[wp-trac] [WordPress Trac] #58087: Fix the 'data' dynamic property in WP_Term

[WordPress Trac]

#58087: Fix the 'data' dynamic property in WP_Term
-------------------------------------+-------------------------------------
 Reporter:  antonvlasenko            |       Owner:  hellofromTonya
     Type:  defect (bug)             |      Status:  reviewing
 Priority:  normal                   |   Milestone:  6.5
Component:  Taxonomy                 |     Version:  6.3
 Severity:  minor                    |  Resolution:
 Keywords:  php82 has-patch has-     |     Focuses:  coding-standards, php-
  unit-tests needs-testing changes-  |  compatibility
  requested                          |
-------------------------------------+-------------------------------------

Comment (by hellofromTonya):

 Continuation of comment:4 's contextual dive to understand its current
 state.

 The `data` property was originally introduced for (see [35032] and [35269]
 in 4.4.0):

 * caching raw database query results in `get_the_terms()`.
    * [37573] removed it, switching the cache to term IDs.
 * saving the updated raw data back to the database in `wp_update_term()`.
    * This still exists today.

 Fast forward to today.
 The `data` property is a container to hold the ''current'' term data /
 column / fields values. But it only gets set ''when'' the dynamic property
 is invoked, which in Core happens in `wp_update_term()`.

 Here's its usage in `wp_update_term()`:

 {{{#!php
 $term_id = (int) $term_id;

 // First, get all of the original args.
 $term = get_term( $term_id, $taxonomy );

 if ( is_wp_error( $term ) ) {
         return $term;
 }

 if ( ! $term ) {
         return new WP_Error( 'invalid_term', __( 'Empty Term.' ) );
 }

 $term = (array) $term->data;

 // Escape data pulled from DB.
 $term = wp_slash( $term );

 // Merge old and new args with new args overwriting old ones.
 $args = array_merge( $term, $args );

 $defaults    = array(
         'alias_of'    => '',
         'description' => '',
         'parent'      => 0,
         'slug'        => '',
 );
 $args        = wp_parse_args( $args, $defaults );
 $args        = sanitize_term( $args, $taxonomy, 'db' );
 $parsed_args = $args;
 }}}


 Notice in the above code:

 * It gets type juggled from `stdClass` to an `array()`.
 Is that necessary? Could `$term->to_array()` be used instead?

 [https://3v4l.org/AQDU3 Comparing the results], the only difference I'm
 seeing is `data` does not include the `filter`, whereas `to_array()` does.

 So other than the `filter` property, why continue using `data`?

 * Notice `sanitize_term()` is used 2x, once in `WP_Term::__get()` and then
 after that in `wp_update_term()`.

 Why do the columns need sanitizing for `'raw'` and then sanitizing for
 `'db'`? The first `'raw'` seems unnecessary in the context of only
 `wp_update_term()`.

 == Is `data` property necessary today?

 In Core, no, as the only usage of it could be replaced.

 What about out in the wild?
 What about BC?

 Honestly, my current thinking:

 I suspect there's no or very very few instances of that dynamic property
 being used. Any instance in Core can be handled internally. As the
 property only existed internally for caching and updating, I don't see a
 use case for using it outside of Core.

 I'm thinking there's little risk for removing it and the `__get()` method,
 similar to removing `proto` in [55580].

 That said, if there's any risk of BC break, the other way to go is to
 declare the property and initialize it in the constructor.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/58087#comment:24>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform