Make WordPress Core

Opened 3 months ago

Last modified 3 weeks ago

#61950 new defect (bug)

add_*_meta()/get_*_meta() undocumented type juggling for some meta values

Reported by: rodrigosprimo's profile rodrigosprimo Owned by:
Milestone: 6.8 Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-patch
Focuses: docs Cc:

Description

While working on a new WPCS sniff related to the get_*_meta() functions (https://github.com/WordPress/WordPress-Coding-Standards/pull/2465), I noticed there is some undocumented type juggling for some meta values.

For example, WP returns an empty string for an object meta created as false.

Here are all the cases that I found:

Value added via add_*_meta()Value returned by get_*_meta()
false ''
true '1'
0 '0'
1.5 '1.5'

I haven't checked, but I believe this behavior was not introduced recently, so changing it would be a breaking change and not really an option. That being said, I suggest that the DocBlock for those functions is updated to document this behavior explicitly. It is probably a good idea to add some tests as well to prevent unexpected changes in the future.

The description of the $meta_value parameter of those functions is "Metadata value. Must be serializable if non-scalar." An initial suggestion is to change it to mention the special cases listed above.

Change History (5)

#1 @jrf
3 months ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 6.7

#2 @azaozz
3 months ago

there is some undocumented type juggling for some meta values

Yep, this is pretty much the same behavior when saving and retrieving options. As the meta_value columns are set to LONGTEXT, all values seem to be converted to strings on saving to the DB. Then non-serialized values are returned as read from the DB.

I believe this behavior was not introduced recently

Right. This has been like that "forever" afaik.

I suggest that the DocBlock for those functions is updated to document this behavior explicitly

Yea, don't think anything else can be done at this point. I actually tried to make options saving and retrieving to respect data types, but at the end there were too many edge cases that it became risky and not practical.

A good docblock update for the different meta retrieval functions (post, comment, user, site, etc.) would include the full description about what's going on, and probably mention that the unexpected types can be avoided at the cost of a tiny bit more processing if the data is serialized. JSON encoding would also work in most cases, but may behave a bit unexpectedly when differentiating between objects and associative arrays.

Version 1, edited 3 months ago by azaozz (previous) (next) (diff)

#3 @peterwilsoncc
2 months ago

How about something like this?

Arrays and objects are stored in the database as serialized data and will be returned as the type used when saving the data.

Other data types will be cast to strings for storing in the database and should be type cast after calling this function for use in their original form.

#4 @peterwilsoncc
7 weeks ago

  • Keywords needs-unit-tests removed
  • Milestone changed from 6.7 to 6.8

I'm moving this off the milestone, as I am not really happy with my suggestion above. I've removed the needs tests keyword as this is a docs change.

This ticket was mentioned in PR #7829 on WordPress/wordpress-develop by @sukhendu2002.


3 weeks ago
#5

  • Keywords has-patch added; needs-patch removed
Note: See TracTickets for help on using tickets.