WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 15 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 16 months ago.
Recursive unserialize meta
meta_recursive_unserialize.2.diff (542 bytes) - added by mattkeys 16 months ago.
Updated version using an anonymous function
meta_recursive_unserialize.3.diff (497 bytes) - added by mattkeys 16 months ago.
replacing array_walk_recursive with simple foreach

Download all attachments as: .zip

Change History (26)

#1 @westi
5 years ago

  • Keywords needs-patch added

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

#2 @nacin
5 years ago

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

@nacin
5 years ago

Patches get_metadata()

#3 @nacin
5 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.

#4 @nacin
5 years ago

  • Milestone changed from 3.1 to Future Release

#5 @nacin
2 years ago

  • Component changed from General to Options and Meta

#6 @boonebgorges
16 months ago

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

@mattkeys
16 months ago

Recursive unserialize meta

#7 @mattkeys
16 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?

#8 @mattkeys
16 months ago

  • Keywords has-patch added; needs-patch removed

@mattkeys
16 months ago

Updated version using an anonymous function

#9 follow-up: @mattkeys
16 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.

#10 in reply to: ↑ 9 @mattkeys
16 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.

#11 follow-up: @nacin
16 months ago

Hey mattkeys, we're still PHP 5.2.4+.

#12 in reply to: ↑ 11 @mattkeys
16 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?

#13 follow-up: @boonebgorges
16 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.

@mattkeys
16 months ago

replacing array_walk_recursive with simple foreach

#14 in reply to: ↑ 13 @mattkeys
16 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.

#15 @boonebgorges
16 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!

#16 @boonebgorges
16 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.

#17 @dd32
15 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 15 months ago by dd32 (previous) (diff)

#18 @johnbillion
15 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.

#19 @boonebgorges
15 months ago

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

#20 @johnbillion
15 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

#21 @johnbillion
15 months ago

In 30702:

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

See #15030

#22 @johnbillion
15 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.