Opened 4 months ago
Closed 4 weeks ago
#47699 closed task (blessed) (fixed)
Remove redundant JSON polyfills for PHP native functionality
Reported by: | jrf | Owned by: | desrosj |
---|---|---|---|
Milestone: | 5.3 | Priority: | normal |
Severity: | normal | Version: | 5.3 |
Component: | General | Keywords: | has-patch has-unit-tests i18n-change has-dev-note commit dev-reviewed |
Focuses: | coding-standards | Cc: | |
PR Number: |
Description
Now the new minimum PHP requirement for WordPress is PHP 5.6.20, the JSON functionality related polyfills for the native PHP functions and related work-arounds can be removed.
I will add a patch for this to this ticket.
Let's talk through it all:
Current implementation
WP polyfills the following PHP native JSON functionality:
- The function `json_encode()` (PHP 5.2.0+)
- The function `json_decode()` (PHP 5.2.0+)
- The function `json_last_error_msg()` (PHP 5.5.0+)
- The constant `JSON_PRETTY_PRINT` (PHP 5.4.0+)
- The constant `JSON_ERROR_NONE` (as part of
json_last_error_msg()
) (PHP 5.3.0+) - The interface `JsonSerializable` (PHP 5.4.0+)
In addition to this, it:
- adds the
_json_decode_object_helper()
function, theWP_JSON_SERIALIZE_COMPATIBLE
constant and the$wp_json
global variable in thewp-includes/compat.php
file. - contains various work-arounds for differences between PHP versions in the
wp_json_encode()
function in thewp-includes/functions.php
file, including the_wp_json_prepare_data()
function which is 100% work-around code. - adds various constants, a
Services_JSON
and aServices_JSON_Error
class in thewp-includes/class-json.php
file
Why all of the above can be safely deprecated/removed
Step 1
As of PHP 5.2.0, the JSON extension is bundled and compiled with PHP by default and can not be disabled.
This means in effect that the following code has not been used since WP 3.3.0 when the minimum PHP requirement went up to PHP 5.2.6:
json_encode()
polyfill, including the dummy copy inwp-admin/includes/noop.php
json_decode()
polyfill_json_decode_object_helper()
function which would only be declared if the PHP nativejson_decode()
function didn't exist and was used exclusively by that polyfill.$wp_json
variable as it would only be assigned a value if the PHP nativejson_encode()
andjson_decode()
functions didn't exist.- All of the code in the
wp-includes/class-json.php
file. - The associated
test_json_encode_decode()
unit test.
With the above in mind, there can be no discussion about removing the first two polyfills - json_encode()
and json_decode()
-.
As for removing the _json_decode_object_helper()
, $wp_json
and the code in the wp-includes/class-json.php
file:
As those were only available conditionally, any userland code referring to this function/variable/these classes/constants, would already have to have been surrounded by appropriate function_exists()
, class_exists()
, defined()
or an isset()
calls.
So, having said that, IMO it is safe to remove all of this.
I'm making one caveat, namely that while the code in the wp-includes/class-json.php
file can be safely removed, the file itself should probably remain (for now) and should generate a deprecated file notice just in case a userland plugin/theme would require it directly, though if they do, they've already had eight years to remove that file-include and they will run into trouble now that the code in the file has been removed, but in my estimation, it would be exceedingly rare, most likely non-existent, for any plugin/theme to include this file directly.
Step 2
Now, as the minimum PHP requirement for WP is now 5.6.20, we can also safely remove:
- the
json_last_error_msg()
polyfill - the
JSON_PRETTY_PRINT
constant polyfill - the
JSON_ERROR_NONE
constant polyfill - the
JsonSerializable
polyfill - any and all work-arounds for differences between PHP versions regarding
json_encode()
- any and all work-arounds for the missing polyfill for
json_last_error()
- see #27799
This includes deprecating the _wp_json_prepare_data()
function which was only needed for PHP 5.2-5.3.
Unit tests
As these functions are used throughout core, removing them can be considered sufficiently unit tested when all the WP Core unit tests pass, and they still do: https://travis-ci.com/WordPress/wordpress-develop/builds/119072132
Other
- I've tried to be comprehensive with this ticket and have searched the codebase for any and all relevant keywords and reviewed all the related code. All the same, as there were quite a lot of different bits and pieces involved in this, I will not guarantee that nothing remains, though I'm fairly confident about the completeness of this patch.
- The patch includes removing the
wp-includes/class-json.php
from the PHPCS exclusions list.
Related tickets
- This ticket is related to #47698 which addresses the other polyfills which can be removed.
- This ticket is related to #27799, #30139.
/cc @pento
Attachments (9)
Change History (48)
#2
@
4 months ago
@Clorith You bring up a valid consideration.
However:
There is no installation needed to use these functions; they are part of the PHP core.
Source: https://www.php.net/manual/en/json.requirements.php
As of PHP 5.2.0, the JSON extension is bundled and compiled into PHP by default.
Source: https://www.php.net/manual/en/json.installation.php
And while it looks like there is an undocumented option to `--disable-json`, any host offering PHP without the JSON extension is willfully breaking PHP and should be burned at the stake (she says in jest).
All comments regarding disabled JSON extensions in the PHP manual are at least five years old, no new comments to that effect have been added since, so I'm hoping this means that hosts have wizened up.
I also don't think that numbers from 8 years ago when 5.3 was the latest & greatest PHP version can in any way compare with the current landscape.
Perhaps a golden middle ground could be taken in this instance, one where we deprecate the polyfills for 1-2 major core releases, where we implement a _doing_it_wrong when the polyfills that should be safe to remove are used. This should give ample warning in scenarios where they are used, while also covering our backs in the process?
In fact, I think a deprecation route such as this would be beneficial for any polyfill slated for removal.
While I agree, that might be a valid route for something like the Services_JSON
class, for anything that is Core PHP, deprecation should be redundant.
All the same, if people are seriously wary of this, I propose the following:
- Add
_deprecated_xxx()
calls in all relevant places in WP 5.3. - Remove work-arounds for differences across PHP versions in WP 5.3. Those already don't take the polyfills into account as they are based on PHP versions, so this should not cause any problems which don't exist already 🙃
- Add the
JSON
extension as a required extension to the list in Site Health in WP 5.3. (Why the heck isn't it listed there now ?!?!?'') - Execute the removal in WP 5.5 at the latest or by the end of the year in the same version as the next PHP version bump, whichever comes sooner.
#3
@
4 months ago
I like the approach you've outlined, and I agree 5.5 at the latest to remove any deprecated features now in supported PHP versions sounds very reasonable.
(and, ironically enough, it was in the Site Health Checker, but I removed it for the same reason you've stated here, that's when the considerations from above were brought to my attention as well, so I thought I'd at least bring it up here for full consideration :) )
@
4 months ago
Agressively deprecate all JSON related polyfills & make the JSON extension a requirement
#4
@
4 months ago
@Clorith I've added a new patch which aggressively deprecates any and all deprecatable JSON related functionality.
Note: Constants, interfaces, (global) variables can not throw deprecation notices, so this still doesn't cover everything, but AFAICS it's as close as it can get.
The patch includes:
- Adding the
JSON
extension as a required module to Site Health. The check is based on thejson_last_error()
function, which is the only function in the JSON extension not polyfilled. - Removing version based checks from
wp_json_encode()
. See my remark about that above. - In a few cases, replacing version based logic with feature based logic.
Passing build: https://travis-ci.com/WordPress/wordpress-develop/builds/119097007
For WP 5.4/5.5, a secondary patch will be needed to remove everything. My original patch can be used for inspiration at that time.
Note: I have not applied this deprecation route to the patches in #47698 as, IMO, it doesn't apply for those patches.
#5
@
4 months ago
- Milestone changed from Awaiting Review to 5.3
Hopefully we can use 47699-remove-json-polyfills-and-work-arounds.patch, but I like the direction of 47699-deprecate-JSON-polyfills.patch as a backup option.
I've asked the VaultPress crew to see how many sites are running PHP 5.6+ without the JSON extension (with breakdown by PHP version, if possible). Once we have some data, we'll hopefully have a better idea of which direction we can go.
#6
follow-up:
↓ 7
@
4 months ago
- Keywords needs-refresh needs-dev-note added; 2nd-opinion removed
The good folks at VaultPress have run some checks across the sites they monitor, and have found approximately 0.01% of PHP 5.6+ sites don't have the JSON extension available.
Giving this some more thought, I'm inclined to remove the polyfills in WP 5.3, except for Services_JSON
, which is still used by plugins/themes (often without even checking if they need to).
We can mark Services_JSON
as deprecated, and put up a dev note to remind devs that they need to check that json_last_error()
doesn't exist before using it.
For the handful of sites that don't have the JSON extension, it'd be good to push out a fix in WP 5.2.3 that would warn them of this. Site Health is part of it, but it should also block updates when WP 5.3 is released, with a relevant error message.
#7
in reply to:
↑ 6
@
4 months ago
Replying to pento:
The good folks at VaultPress have run some checks across the sites they monitor, and have found approximately 0.01% of PHP 5.6+ sites don't have the JSON extension available.
🎉
Thank you good people at VaultPress!
This result is as I expected, but it's great to get confirmation of it.
Giving this some more thought, I'm inclined to remove the polyfills in WP 5.3, except for
Services_JSON
, which is still used by plugins/themes (often without even checking if they need to).
I've run some tests to see which wp.org plugins/themes we are talking about and it looks like it's only really plugins, with the worse offender being JetPack.
- Plugins which appear to include the class-json.php file - 16 plugins with > 0 installs.
- Themes which appear to include the class-json.php file - 1 theme with 50 installs.
- Plugins which instantiate a `new Services_JSON` instance - 210 plugins with > 0 installs, though it looks like this includes some false positives with the class being mentioned in documentation and not actually being instantiated, like in WordFence, where they use their own, differently named copy of the file/class.
- Themes which instantiate a `new Services_JSON` instance - 3 themes with > 0 installs, though it looks like 2 out of 3 are false positives, in that they include their own copy of the
Services_JSON
file which makes them independent of WP Core.
We can mark
Services_JSON
as deprecated, and put up a dev note to remind devs that they need to check thatjson_last_error()
doesn't exist before using it.
👍
For the handful of sites that don't have the JSON extension, it'd be good to push out a fix in WP 5.2.3 that would warn them of this. Site Health is part of it, but it should also block updates when WP 5.3 is released, with a relevant error message.
I've just been looking at this, but I haven't been able to find any such existing check yet for required extensions, though I may be looking in the wrong place.
If it doesn't exist yet, it would need to be added and it would be useful if we could re-use the list of required extensions as contained in the WP_Site_Health::get_test_php_extensions()
method.
For now, I'm going to start with refreshing the WP 5.3 patch and adding a separate patch for WP 5.2 Site Health.
@
4 months ago
WP 5.2.x: make JSON a required extension for Site_Health, patch created against the WP 5.2 branch
#8
@
4 months ago
- Keywords needs-refresh removed
For now, I'm going to start with refreshing the WP 5.3 patch and adding a separate patch for WP 5.2 Site Health.
4 new patches for this have now been uploaded.
I've basically split the original patches based on the separate decision points involved:
- WP 5.2.x: make JSON a required extension for Site Health
- WP 5.3: agressively deprecate the
class-json.php
file, deprecated theServices_JSON
and theServices_JSON_Error
classes - WP 5.3: remove any and all work-arounds in WP Core which were in place for the JSON extension potentially not being available/for JSON in PHP < 5.6.
- WP 5.3: remove the actual poyfills
Passing build for the WP 5.3 patches: https://travis-ci.com/WordPress/wordpress-develop/builds/119772721
What's missing is the WP 5.2.x patch to prevent updating WP Core to the next major when the JSON extension is missing, but I'd like to receive a little more input before I create that patch.
I've just been looking at this, but I haven't been able to find any such existing check yet for required extensions, though I may be looking in the wrong place.
If it doesn't exist yet, it would need to be added and it would be useful if we could re-use the list of required extensions as contained in the WP_Site_Health::get_test_php_extensions() method.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
3 months ago
#14
follow-up:
↓ 24
@
8 weeks ago
- Keywords close added
[46205-46206,46208] contains all of the patches except 47699-WP-5.2.x-site-health-json-required.patch. With beta 1 in less than 3 days, I wanted to ensure these had a bit of soak time before then.
I'm leaving this open for now in case any issues come up, but it may be best to open a separate ticket for making any changes in the 5.2 branch.
#15
@
8 weeks ago
While this shouldn't be a problem for the vast majority of sites, not having JSON compiled into PHP will cause things to break in wonderful ways. See #18015
I think a check in the core upgrader for json functions and a rollback if they are not available could be a warranted addition.
#16
@
8 weeks ago
- Type changed from enhancement to task (blessed)
I'm going to convert this to a task so that the follow up discussion @jorbin called out can continue during the beta period.
#17
@
8 weeks ago
I got pinged about this, so I finally wrote a ticket proposing some stat collection of PHP extension in use: #48116.
In general, I'd like to think that ext/json
is installed on most hosts and available by default, but as I recently found out, it's not always a given - a Docker image I was running for PHP had it disabled for some reason (WordPress worked, the other app I wanted to run didn't).
I think a check in the core upgrader for json functions and a rollback if they are not available could be a warranted addition.
Thankfully the upgrade process can be blocked, and so no rollback functionality is required, a change to wp-admin/includes/update-core.php
like the following should do the job: https://github.com/dd32/wordpress-develop/commit/db7e0e68ead2b58a2fb651b276a3f54380473d5e
#18
follow-up:
↓ 19
@
7 weeks ago
@dd32
I finally wrote a ticket proposing some stat collection of PHP extension in use: #48116.
Excellent! 👍🏻👍🏻 Want me to look over the patch ?
Thankfully the upgrade process can be blocked, and so no rollback functionality is required, a change to wp-admin/includes/update-core.php like the following should do the job: https://github.com/dd32/wordpress-develop/commit/db7e0e68ead2b58a2fb651b276a3f54380473d5e
Wouldn't (a variation of) that change, including a version check with the intended version to upgrade to, need to go into the update-core.php
core file of every single minor which is still supported ? And warrant a patch release for all of them ?
Or is the update-core.php
file from the new version used even before the files are copied over ? (Sorry, never looked into that part of WP that deeply, so I honestly don't know)
#19
in reply to:
↑ 18
@
7 weeks ago
Replying to jrf:
Thankfully the upgrade process can be blocked, and so no rollback functionality is required, a change to wp-admin/includes/update-core.php like the following should do the job: https://github.com/dd32/wordpress-develop/commit/db7e0e68ead2b58a2fb651b276a3f54380473d5e
Wouldn't (a variation of) that change, .... [snip]
Or is the
update-core.php
file from the new version used even before the files are copied over ?
Exactly this, the update-core.php
file in core is not used for the current upgrade process, it gets copied from the new version of WordPress being installed and then executed - taking over the upgrade process from the old version of WordPress; Allowing us to ship fixes/changes to the upgrade process no matter what version they're upgrading from.
As a result, the one that's present on sites at present has run it's course as it was only used for the upgrade TO the version that's currently installed, never again. Hopefully that makes sense?
#20
@
7 weeks ago
@dd32 Makes perfect sense to me and makes life so much easier in this case. Whoever thought this up: 💖
As you already drafted that last patch which is needed for this ticket, would you like to upload a patch or would you prefer I work this out further ?
Should this be set up in a way that both Site Health as well as the update-core.php
file can use the same list of required extensions ?
I'm in two minds about this as Site Health does a check after install/upgrade, while update-core
will block upgrades before they are rolled out.
Maybe an inline comment in Site Health to update the list in update-core
whenever an extension gets set to required
and visa versa would suffice ?
@
7 weeks ago
Services_Json: add @deprecated tags - I'd neglected to do so in the previous patch as committed in [46205]
#22
follow-up:
↓ 28
@
6 weeks ago
- Keywords close removed
47699-update-warning.diff is a patch for the changes @dd32 linked to in his test branch above.
Are there any objections to landing this change before beta 3?
#23
@
6 weeks ago
@desrosj No objections, but if we land it like this, a separate ticket should probably be opened to address the concerns I raised in https://core.trac.wordpress.org/ticket/47699#comment:20
#24
in reply to:
↑ 14
@
6 weeks ago
Replying to desrosj:
[46205-46206,46208] contains all of the patches except 47699-WP-5.2.x-site-health-json-required.patch.
Just noting that this patch was included separately in [46268] (since I was wondering for a bit why it was excluded here).
This ticket was mentioned in Slack in #core by marybaum. View the logs.
5 weeks ago
This ticket was mentioned in Slack in #core by desrosj. View the logs.
5 weeks ago
#27
@
5 weeks ago
- Keywords i18n-change added
Marking this as a string change since we are pasts soft freeze.
#28
in reply to:
↑ 22
@
5 weeks ago
- Keywords commit added
Replying to desrosj:
47699-update-warning.diff is a patch for the changes @dd32 linked to in his test branch above.
Are there any objections to landing this change before beta 3?
This looks good to me. Thanks @dd32 and @desrosj!
#30
@
5 weeks ago
- Keywords commit removed
Thanks, everyone! I am drafting a dev note for this now. The one thing remaining we could do for this is to specify ext-json
as a requirement in Composer. But, I think that can be explored in a separate ticket where all dependencies can be audited.
#31
@
5 weeks ago
The one thing remaining we could do for this is to specify ext-json as a requirement in Composer. But, I think that can be explored in a separate ticket where all dependencies can be audited.
@desrosj There's already a ticket open for that: #48086
#33
@
4 weeks ago
- Keywords has-dev-note added; needs-dev-note removed
- Resolution fixed deleted
- Status changed from closed to reopened
Dev note posted: https://make.wordpress.org/core/2019/10/15/php-native-json-extension-now-required/
I am reopening to explore changing the error code slightly. The error code for this specific failure should be different than the one for incompatible versions of PHP (PHP < 5.6.20). This will allow more accurate tracking of exactly how many sites cannot upgrade because the native JSON extension is disabled.
Incoming patch with the suggested php_not_compatible_json
code change.
This ticket was mentioned in Slack in #core by desrosj. View the logs.
4 weeks ago
#36
@
4 weeks ago
Posting the related discussion for this change here for reference: https://make.wordpress.org/core/2019/10/15/php-native-json-extension-now-required/#comment-37091
I think we may need data to back this, as a discussion in this took place in March in the #core-php channel, and although it's compiled into PHP by default, many choose to define extensions manually when building PHP, and as such may not be including it.
@SergeyBiryukov brought up the point that hosts did (and some still might) compile without the json extension. Given how important json is in the majority of interactions on the internet today, it would be beneficial to have some metric behind removing various polyfills, jsut so we don't "breaking the internet" by removing one (at least that was the outcome of the discussions back then).
I see we've previously reached out to VaultPress for these kind of metrics, is this a viable approach still, or are there better ways to get anonymous data of that variety (I know there's been talk of it being useful in other scenarios, but we don't have anything like this our selves at times time last I heard).
Related: #18015, #19524 (the removal of the polyfill when the minimum PHP requirement was bumped to 5.2 for WordPress 3.2)
The percentages are likely different now (I would hope), but it would be nice to have confirmation on that if at all possible.
---
Perhaps a golden middle ground could be taken in this instance, one where we deprecate the polyfills for 1-2 major core releases, where we implement a
_doing_it_wrong
when the polyfills that should be safe to remove are used. This should give ample warning in scenarios where they are used, while also covering our backs in the process?In fact, I think a deprecation route such as this would be beneficial for any polyfill slated for removal.