WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 8 months ago

#15030 closed defect (bug) (wontfix)

Unserialize deep when returning arrays of metadata

Reported by: nacin Owned by: boonebgorges
Milestone: Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: revert
Focuses: Cc:

Description (last modified by nacin)

Currently when retrieving an array of metadata values for an object (no meta key specialized), they are retrieved as serialized. Only when meta key is specified do we unserialize.

We should array_map( 'maybe_unserialize' ) on what we plan to return instead of forcing plugins to call that themselves.

Attachments (4)

15030.diff (366 bytes) - added by nacin 5 years ago.
Patches get_metadata()
meta_recursive_unserialize.diff (1.1 KB) - added by mattkeys 9 months ago.
Recursive unserialize meta
meta_recursive_unserialize.2.diff (542 bytes) - added by mattkeys 9 months ago.
Updated version using an anonymous function
meta_recursive_unserialize.3.diff (497 bytes) - added by mattkeys 9 months ago.
replacing array_walk_recursive with simple foreach

Download all attachments as: .zip

Change History (26)

comment:1 @westi5 years ago

  • Keywords needs-patch added

Sounds reasonable.
What is the WP function your going to patch here?

comment:2 @nacin5 years ago

  • Description modified (diff)
  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 3.1

@nacin5 years ago

Patches get_metadata()

comment:3 @nacin5 years ago

  • Keywords needs-patch added; has-patch removed

According to dd32 this patch doesn't go deep enough, and needs to go one level down. Sounds about right.

comment:4 @nacin5 years ago

  • Milestone changed from 3.1 to Future Release

comment:5 @nacin19 months ago

  • Component changed from General to Options and Meta

comment:6 @boonebgorges9 months ago

  • Keywords needs-unit-tests good-first-bug added

@mattkeys9 months ago

Recursive unserialize meta

comment:7 @mattkeys9 months ago

@boonebgorges I heard your talk at SF WordCamp last weekend, very inspiring. I'd like to wet my feet with contributing to core and I have submitted a possible solution to this ticket.

Can you take a look and let me know if I am moving in the right direction here?

comment:8 @mattkeys9 months ago

  • Keywords has-patch added; needs-patch removed

@mattkeys9 months ago

Updated version using an anonymous function

comment:9 follow-up: @mattkeys9 months ago

Looking this over again, I think using an anonymous function here might be more appropriate to avoid needing the new callback function in /wp-includes/functions.php, since that function is not likely to be needed anywhere else.

comment:10 in reply to: ↑ 9 @mattkeys9 months ago

Replying to mattkeys:

Looking this over again, I think using an anonymous function here might be more appropriate to avoid needing the new callback function in /wp-includes/functions.php, since that function is not likely to be needed anywhere else.

It looks like anonymous functions only became available in PHP 5.3, and WP supports PHP 5.2.4, so I guess we cannot use the anonymous function version of my patch.

comment:11 follow-up: @nacin9 months ago

Hey mattkeys, we're still PHP 5.2.4+.

comment:12 in reply to: ↑ 11 @mattkeys9 months ago

Replying to nacin:

Hey mattkeys, we're still PHP 5.2.4+.

Yea I realized right after submitting that second version of the patch that the anonymous function was not going to be available to us. Any thoughts on the first version I posted?

comment:13 follow-up: @boonebgorges9 months ago

  • Milestone changed from Future Release to 4.1
  • Owner set to boonebgorges
  • Status changed from new to accepted

I think we can probably do this without writing a new function. Let's just loop over the values and map maybe_unserialize() on them.

@mattkeys9 months ago

replacing array_walk_recursive with simple foreach

comment:14 in reply to: ↑ 13 @mattkeys9 months ago

Replying to boonebgorges:

I think we can probably do this without writing a new function. Let's just loop over the values and map maybe_unserialize() on them.

Yea I was overcomplicating things here. New patch submitted.

comment:15 @boonebgorges9 months ago

mattkeys - Yup, that's exactly what I had in mind. I'll write a couple unit tests for it and we'll put it in. Thanks for the contribution!

comment:16 @boonebgorges9 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 30115:

Unserialize get_metadata() results when 'key' is omitted.

Props mattkeys, nacin.
Fixes #15030.

comment:17 @dd328 months ago

This is kind of obvious, but this is one thing that's worth noting is no longer backwards compatible (and affects,get_post_custom(), get_post_meta(), and get_metadata()).

If anyone was expecting a serialized string and passing it to unserialize() things will now break. If they were using maybe_unserialize() it will silently still work.

Last edited 8 months ago by dd32 (previous) (diff)

comment:18 @johnbillion8 months ago

  • Keywords revert added; needs-unit-tests good-first-bug has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

We're seeing breakage on WordPress.com VIP due to this change. Reopening to consider a revert.

comment:19 @boonebgorges8 months ago

I'm fine with a revert if this is considered unacceptable breakage.

comment:20 @johnbillion8 months ago

In 30701:

Revert r30115 which was a breaking change for code which interacts with the return value of get_metadata() when no meta key is specified.

See #15030

comment:21 @johnbillion8 months ago

In 30702:

Update the tests for get_metadata() to reflect the revert in r30701.

See #15030

comment:22 @johnbillion8 months ago

  • Milestone 4.1 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

Wontfixing. If anyone comes up with a backwards-compatible way of doing this then by all means please re-open the ticket.

Note: See TracTickets for help on using tickets.