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: |
|
Owned by: |
|
---|---|---|---|
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 )
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 firstjson_encode()
call.
- Patch 2 presumes that the call should influence the
$data
for bothjson_encode()
calls.
- Patch 3 presumes that the call should only influence the
$data
for the potential secondjson_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)
Change History (19)
This ticket was mentioned in Slack in #core-restapi by jrf. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.
9 years ago
#6
follow-up:
↓ 15
@
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
@
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) ?
#9
@
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
@
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
@
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
@
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
@
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).
@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 tojson_encode
always OR only ifjson_encode()
fails.