[wp-trac] [WordPress Trac] #43392: Support 'object' and 'array' types in register_meta()

[WordPress Trac]

#43392: Support 'object' and 'array' types in register_meta()
-------------------------------------------------+-------------------------
 Reporter:  diegoliv                             |       Owner:
                                                 |  TimothyBlynJacobs
     Type:  enhancement                          |      Status:  assigned
 Priority:  normal                               |   Milestone:  5.3
Component:  Options, Meta APIs                   |     Version:  4.9.4
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch has-unit-tests dev-        |     Focuses:  rest-api
  feedback 2nd-opinion                           |
-------------------------------------------------+-------------------------

Comment (by TimothyBlynJacobs):

 Thanks for the review @birgire!

 Replying to [comment:30 birgire]:
 > Hi, I noticed a comment here:
 >
 > https://stackoverflow.com/a/11735933/2078474
 >
 > about the {{{SORT_REGULAR}}} option of {{{array_unique()}}} instead of
 the default {{{SORT_STRING}}}.
 >
 > I've not tested it, but wonder if it's relevant here.

 It seems like `SORT_REGULAR` does a loose comparison which has some
 gotchas: https://stackoverflow.com/questions/14802680/array-unique-sort-
 regular-flag

 > In any case I wonder if it would be beneficial to add an inline comment
 with a little bit of explanation for this part:
 >
 > {{{
 > $to_remove = array_map( 'maybe_unserialize', array_unique( array_map(
 static function( $value ) {
 >     return is_scalar( $value ) ? $value : maybe_serialize( $value );
 > }, $to_remove ) ) );
 >
 > }}}
 >
 > ... this just came to mind, as this part stopped my quick code skimming
 :-)

 That makes sense, I've added a comment explaining.

 >
 > I also wonder if:
 >
 > {{{
 > $to_remove = array_map( 'maybe_unserialize', array_unique( array_map(
 'maybe_serialize', $to_remove ) ) );
 >
 > }}}
 >
 > would have a noticeable performance difference.

 I'm not sure. But I think it is also the correct way of doing it so double
 serialization is handled properly. Nice catch.

 > ps: Just some minor notes regarding the inline docs:
 >
 > - There are missing {{{@since 43392}}} on two new test methods.
 >
 > - Missing translation comments.
 >
 > - I also think the {{{"This is need"}}} should be {{{"This is needed"}}}
 in the description for the {{{default_additional_properties_to_false()}}}
 method.

 Fixed!

 -----

 While looking at the `array_unique` issue, I looked a bit at property
 order as well. JSON objects have no order, however PHP associative arrays
 do. Which means that the `array_keys( $to_remove, $value, true )` line
 doesn't catch the existing value because when PHP does strict associative
 array comparisons it cares about the order of the elements.

 I added a test to demonstrate this
 `test_update_multi_meta_value_object_property_order_is_ignored`. It only
 fails due to the last assertion that the meta id didn't change. WordPress
 is deleting the old value and then re-adding it with the new ( differently
 ordered ) value.

 Should we account for this?

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