#49810 closed enhancement (fixed)
Remove workaround for $HTTP_RAW_POST_DATA bug present in PHP < 5.2.2
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.5 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch has-unit-tests has-dev-note |
Focuses: | rest-api | Cc: |
Description
This version of PHP is no longer officially supported, so the workaround need not be present in core.
Attachments (2)
Change History (39)
This ticket was mentioned in PR #213 on WordPress/wordpress-develop by skoskie.
5 years ago
#1
This ticket was mentioned in PR #214 on WordPress/wordpress-develop by skoskie.
5 years ago
#3
Removed workaround for $HTTP_RAW_POST_DATA bug present in PHP < 5.2.2, since that version is no longer supported.
Trac ticket: https://core.trac.wordpress.org/ticket/49810
5 years ago
#4
Now the workaround is always used which is the wrong way to "fix" this. See also https://www.php.net/manual/en/reserved.variables.httprawpostdata.php
#5
@
5 years ago
- Milestone changed from Awaiting Review to 5.5
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
Hi there, welcome to WordPress Trac! Thanks for the patch.
As noted in the PHP manual, $HTTP_RAW_POST_DATA
was deprecated in PHP 5.6.0, and removed as of PHP 7.0.0.
The code relying on it should be updated to always use php://input
instead.
#6
@
5 years ago
- Resolution set to invalid
- Status changed from reviewing to closed
Apologies as this is my first contribution.
Since WP still supports PHP 5.6, and therefore the $HTTP_RAW_POST_DATA
variable, should we not ensure it remains set for those still depending on it?
Anyway, this ticket can be closed. Thank you.
#7
@
5 years ago
- Resolution invalid deleted
- Status changed from closed to reopened
Thanks for the contribution, no need for apologies :)
I think this ticket is valid, and some changes should be made here, just not exactly the currently proposed patch.
#8
@
5 years ago
Found this searching Trac before creating my own ticket for the same issue.
While working on the currently flagged PHPCompatibility issues in #49922, this came up.
I think this one is tricky to fix. A plugin directory search yields ~1,500 instances of HTTP_RAW_POST_DATA
in the directory. Any plugin or custom code that relies on this variable being available could potentially experience problems when it is removed. There's even a test within the REST API tests making sure content in that global variable is properly unslashed.
This ticket was mentioned in PR #305 on WordPress/wordpress-develop by jrfnl.
5 years ago
#9
See extensive comment in the related Trac ticket: https://core.trac.wordpress.org/ticket/49810
#10
@
5 years ago
I've had a look at this and all other occurrences of $HTTP_RAW_POST_DATA
in WP.
First off, this is a PHP Core removed global variable. Any plugin which relies on WP core backfilling that variable is most definitely doing it wrong™.
In reality, WP doesn't actually backfill the variable except for the XML-RPC entry point and select REST requests, which aren't the most common entry point for WP anyway, and on top of that, XML-RPC is very often disabled.
So, let's look at the three different files in which the issue occurs:
src/wp-includes/IXR/class-IXR-server.php
This is a file from an external dependency. I'm not sure by heart if this dependency is still maintained externally or abandoned and now maintained in WP Core only.
This file doesn't actually backfill the variable, it just uses it if available.
Conclusion: This should just be switched out for php://input
.
src/wp-includes/rest-api/class-wp-rest-server.php
While this function - WP_REST_Server::get_raw_data()
- does actually backfill the global, this looks more like an oversight than an architectural choice.
The important thing here is that the static WP_REST_Server::get_raw_data()
method is in place to handle the potential non-availability of the $HTTP_RAW_POST_DATA
variable and that any REST related code should already use this method instead of relying on the availability of the $HTTP_RAW_POST_DATA
variable.
Conclusion: This should just be switched out for php://input
.
src/xmlrpc.php
This is the only one where a case could be made for letting the variable remain as-is. The variable gets backfilled in the global scope for a very select entry point and is subsequently never used in WP Core, though the class-IXR-server.php
file may have used it, but as it already had its own BC-code in place that was never an issue.
Conclusion: As it is never used in Core, this might be an entry point for which plugins expect the variable to exist and leaving it in place doesn't do any actual harm.
Patch
I've attached a new patch via GitHub to address this issue based on the above analysis.
PHPCompatibility report
FILE: src\wp-includes\IXR\class-IXR-server.php ---------------------------------------------------------------------------------------------------------------------------------------------------- FOUND 3 ERRORS AFFECTING 3 LINES ---------------------------------------------------------------------------------------------------------------------------------------------------- 50 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead 51 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead 55 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead ---------------------------------------------------------------------------------------------------------------------------------------------------- FILE: src\wp-includes\rest-api\class-wp-rest-server.php ---------------------------------------------------------------------------------------------------------------------------------------------------- FOUND 4 ERRORS AFFECTING 4 LINES ---------------------------------------------------------------------------------------------------------------------------------------------------- 1374 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead 1380 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead 1381 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead 1384 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead ---------------------------------------------------------------------------------------------------------------------------------------------------- FILE: src\xmlrpc.php ---------------------------------------------------------------------------------------------------------------------------------------------------- FOUND 5 ERRORS AFFECTING 4 LINES ---------------------------------------------------------------------------------------------------------------------------------------------------- 20 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead 21 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead 25 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead 26 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead 26 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead ----------------------------------------------------------------------------------------------------------------------------------------------------
5 years ago
#11
For some weird reason the second commit I added to this branch does not show up. Opening a new ticket.
This ticket was mentioned in PR #306 on WordPress/wordpress-develop by jrfnl.
5 years ago
#12
- Keywords has-unit-tests added
See extensive comment in the related Trac ticket: https://core.trac.wordpress.org/ticket/49810
Replaces #305
#13
@
5 years ago
- Focuses rest-api added
I don't see how the XML-RPC logic is any different than the REST API logic. The request object isn't available immediately, notably for authentication. They may be relying on the raw post data to do a signature check.
WooCommerce does something similar to this, but they use WP_REST_Server::get_raw_data()
so they wouldn't be effected: https://wpdirectory.net/search/01E9W6WA98EVCS5AFW5KKW89F5 But I think the logic still holds for someone wanting to get the raw body before the REST request is available or they intentionally want to get data for the global request.
5 years ago
#14
@jrfnl For future PRs: Please use a fork of this repo as branches may get deleted due to the git mirror syncing.
5 years ago
#15
@ocean90 Thanks for the heads-up. I must have pushed to the wrong remote. Will keep it mind for the future.
#17
@
5 years ago
But I think the logic still holds for someone wanting to get the raw body before the REST request is available or they intentionally want to get data for the global request.
@TimothyBlynJacobs They can without any problem by using file_get_contents('php://input')
which is the recommended alternative for the removed PHP global.
#18
@
5 years ago
They can without any problem by using file_get_contents('php://input') which is the recommended alternative for the removed PHP global.
Yes, of course they can update their code to do that. Developers using the XML-RPC based one could as well. It is still a BC break that costs us very little to maintain.
#19
@
5 years ago
@TimothyBlynJacobs This is not a WP BC-break though, but a PHP Core one, which most developers will have mitigated against already anyway.
#20
@
5 years ago
This is not a WP BC-break though, but a PHP Core one,
What is the distinction? If you were expecting that global to be backfilled, your code will break.
Conclusion: As it is never used in Core, this might be an entry point for which plugins expect the variable to exist and leaving it in place doesn't do any actual harm.
Do you no longer think that then?
#21
@
5 years ago
If you were expecting that global to be backfilled, your code will break.
Noone should ever have expected that though. The backfilling by WP in select places was only WP's way to mitigate the PHP BC-break when PHP 5.2 still needed to be supported.
Do you no longer think that then?
That's the only place where the variable is explicitely declared in the global namespace, which is why I made an exception.
Removing the mitigation there would involve renaming the variable and introducing a new variable in the global namespace which is risky as well, which is why I left it as is, but yes, realistically, renaming that variable is on the table.
#22
@
5 years ago
- Owner changed from SergeyBiryukov to jrf
- Status changed from reopened to assigned
The whole thing with the variable in the src/xmlrpc.php
file is, is that the variable is unused in WP.
To move forward there are three options:
- Remove the variable all together as it is unused.
- Rename the variable.
- Leave as is.
As the variable is unused in WP core, both option 1 and 2 feel like a pretty big break.
This is different from the REST API situation where there was a function in place already which people should have used instead from the beginning.
#23
@
5 years ago
- Owner changed from jrf to SergeyBiryukov
Sorry, my browser is being awkward and changing things I never intended to change. Hoping I've set things back to how they were before correctly now.
#24
@
5 years ago
The variable was deprecated in PHP 5.6 and removed from PHP in version 7.0.
What about the following plan:
- Leave as is for now (Core still supports 5.6).
- Publish a post on Make/Core and cross post to the Make/Plugins blog detailing that this variable should not be used, and
php://input
should be used instead. - Detailing in that post that it would be removed from core when the minimum required version of PHP is bumped to 7.0.
- Removing the variable in Core when the version requirement is bumped to 7.0.
If by chance someone is utilizing the variable, it will still work as expected for now, but there is documentation around how to stop using it, and they are given some time to make an adjustment. The comments of that post on Make/Core may also give us some BC guidance if someone is using it unconventionally.
#25
@
5 years ago
@desrosj I can live with that.
Note: as the change to src/wp-includes/IXR/class-IXR-server.php
does not involve a BC break in any way, that change should be made already. The other two can then wait for the next WP version.
This ticket was mentioned in PR #313 on WordPress/wordpress-develop by desrosj.
5 years ago
#26
Addresses occurrences of $HTTP_RAW_POST_DATA
in the codebase.
Trac ticket: https://core.trac.wordpress.org/ticket/49810
#27
@
5 years ago
@jrf What about an approach like in my linked PR (unit tests need to get fixed).
It it follows the plan I outlined above, but if the variable is set in those two situations the variable would remain, a _doing_it_wrong()
is triggered to notify that the variable was removed in PHP 7.0.
#28
@
5 years ago
@desrosj Sorry, but no, that doesn't seem like a good idea as that will throw an error on all PHP 5.6 installs without users being able to do anything to mitigate it.
While the variable was deprecated in PHP by that time, it still existed.
#29
@
5 years ago
Ah, yes. I was trying to be too clever there.
I updated my PR above to what reflect what I believe to be the current desired approach. Tests pass locally, there is just a huge backlog of builds in Travis due to WCEU contributor day.
@TimothyBlynJacobs does the plan I detailed make sense and feel reasonable to you?
#30
@
5 years ago
does the plan I detailed make sense and feel reasonable to you?
I'm more comfortable with that plan.
#31
@
5 years ago
- Component changed from XML-RPC to General
- Keywords needs-dev-note added
- Severity changed from trivial to normal
@TimothyBlynJacobs Great! @jrf was kind enough to give another review to the attached PR. This is good to go for now.
I'm also going to move this to General because it affects both XML-RPC and the REST API.
#33
@
5 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
- Version trunk deleted
Reopening and assigning to myself for a developer note.
5 years ago
#35
Closing in favour of merged patch: https://core.trac.wordpress.org/changeset/47926
#36
@
5 years ago
- Resolution set to fixed
- Status changed from reopened to closed
I am in the process of drafting the dev note and just confirmed it is in the list of notes being tracked by the release squad. Going to close this out to help clear the milestone in advance of beta 1 tomorrow.
#37
@
5 years ago
- Keywords has-dev-note added; needs-testing needs-dev-note removed
Referenced in the following dev note: https://make.wordpress.org/core/2020/07/14/php-related-improvements-changes-wordpress-5-5-edition/.
See Ticket : https://core.trac.wordpress.org/ticket/49810