Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#36358 closed defect (bug) (fixed)

Return of _wp_json_prepare_data() in wp_json_encode() should be used.

Reported by: jrf's profile jrf Owned by: rmccue's profile rmccue
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.4
Component: General Keywords: has-patch needs-unit-tests 4.6-early
Focuses: Cc:

Description (last modified by swissspidy)

I was looking at the wp_json_encode() function for unrelated reasons and noticed a code oddity where the return of a function call to _wp_json_prepare_data() which was intentionally introduced was being disregarded.

The code was introduced in [34926] by @rmccue.

I'm including three different patches which each solve the issue in a slightly different way.

  • Patch 1 presumes that the call should only influence the $data for the first json_encode() call.
  • Patch 2 presumes that the call should influence the $data for both json_encode() calls.
  • Patch 3 presumes that the call should only influence the $data for the potential second json_encode() call.

As I'm not 100% sure about the intended influence reach of the code, I'd like to ask @rmccue to review the patches and decide which one should be merged.

Attachments (3)

36358-patch1-_wp_json_prepare_data.patch (814 bytes) - added by jrf 9 years ago.
36358-patch2-_wp_json_prepare_data.patch (1.3 KB) - added by jrf 9 years ago.
36358-patch3-_wp_json_prepare_data.patch (1.1 KB) - added by jrf 9 years ago.

Download all attachments as: .zip

Change History (19)

#1 @swissspidy
9 years ago

  • Description modified (diff)

#2 @swissspidy
9 years ago

  • Keywords has-patch needs-unit-tests added

This ticket was mentioned in Slack in #core-restapi by jrf. View the logs.


9 years ago

#4 @rachelbaker
9 years ago

@rmccue @swissspidy or @pento Can you take a look? I cannot tell from [34926] if the intent was for the result of _wp_json_prepare_data to be passed to json_encode always OR only if json_encode() fails.

This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.


9 years ago

#6 follow-up: @rmccue
9 years ago

  • Keywords 4.6-early added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to rmccue
  • Status changed from new to reviewing

Sorry, I had meant to comment here! 36358-patch1-_wp_json_prepare_data.patch is good-to-go, but needs to wait until we've branched to 4.6.

#7 @jrf
9 years ago

@rmccue @rachelbaker Thank you both for your response. Glad to see this will make it in soon.

Just putting it out there: should this also be ported to 4.5.1 (and maybe 4.4.3) ?

#8 @rachelbaker
9 years ago

  • Milestone changed from Future Release to 4.5.1

#9 @swissspidy
9 years ago

Not (yet) familiar with _wp_json_prepare_data(), so maybe anyone else can write a test highlighting the bug and how the patch(es) fixes it?

#10 @jrf
9 years ago

@swissspidy Re: unit tests - while I totally agree it would be good to have unit tests for these functions, IMHO that's outside of the scope of this issue.

This is simply about fixing a 'typo' in a previous commit.

#11 @swissspidy
9 years ago

@jrf Yeah, but there's no test at all for wp_json_encode() showing the usage of _wp_json_prepare_data(). That's why I was wondering.

Since _wp_json_prepare_data() was introduced in 4.4 as part of the REST API infrastructure to support the JsonSerializable interface for PHP 5.2 and 5.3, this is unlikely to affect the majority of users.

That's why it doesn't really feel like 4.5.1 material to me, unless it would also be fixed in 4.4.x. What about putting this in 4.6 only?

#12 @jrf
9 years ago

That's why it doesn't really feel like 4.5.1 material to me, unless it would also be fixed in 4.4.x. What about putting this in 4.6 only?

@rmccue would be the best person to judge this IMHO.

#13 @rmccue
9 years ago

I am +1 on 4.6-only. I don't think it's a major bug, it's more like an enhancement (i.e. enabling JsonSerializable on PHP <5.4).

#14 @swissspidy
9 years ago

  • Milestone changed from 4.5.1 to 4.6

#15 in reply to: ↑ 6 @ocean90
9 years ago

Replying to rmccue:

Sorry, I had meant to comment here! 36358-patch1-_wp_json_prepare_data.patch is good-to-go, but needs to wait until we've branched to 4.6.

@rmccue 4.6 was branched a few weeks ago. What is needed to get this in?

#16 @rmccue
9 years ago

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

In 37444:

REST API: Use prepared JSON data correctly.

This was modifying a variable that was never used. Oops.

Fixes #36358.
Props jrf.

Note: See TracTickets for help on using tickets.