WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 2 years ago

Last modified 7 weeks 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 6 years ago.
Patches get_metadata()
meta_recursive_unserialize.diff (1.1 KB) - added by mattkeys 2 years ago.
Recursive unserialize meta
meta_recursive_unserialize.2.diff (542 bytes) - added by mattkeys 2 years ago.
Updated version using an anonymous function
meta_recursive_unserialize.3.diff (497 bytes) - added by mattkeys 2 years ago.
replacing array_walk_recursive with simple foreach

Download all attachments as: .zip

Change History (27)

#1 @westi
6 years ago

  • Keywords needs-patch added

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

#2 @nacin
6 years ago

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

@nacin
6 years ago

Patches get_metadata()

#3 @nacin
6 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
6 years ago

  • Milestone changed from 3.1 to Future Release

#5 @nacin
3 years ago

  • Component changed from General to Options and Meta

#6 @boonebgorges
2 years ago

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

@mattkeys
2 years ago

Recursive unserialize meta

#7 @mattkeys
2 years 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
2 years ago

  • Keywords has-patch added; needs-patch removed

@mattkeys
2 years ago

Updated version using an anonymous function

#9 follow-up: @mattkeys
2 years 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
2 years 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
2 years ago

Hey mattkeys, we're still PHP 5.2.4+.

#12 in reply to: ↑ 11 @mattkeys
2 years 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
2 years 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
2 years ago

replacing array_walk_recursive with simple foreach

#14 in reply to: ↑ 13 @mattkeys
2 years 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
2 years 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
2 years 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
2 years 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 2 years ago by dd32 (previous) (diff)

#18 @johnbillion
2 years 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
2 years ago

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

#20 @johnbillion
2 years 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
2 years ago

In 30702:

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

See #15030

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

#23 @mbelchev
7 weeks ago

#38352 was marked as a duplicate.

Note: See TracTickets for help on using tickets.