Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#47699 closed task (blessed) (fixed)

Remove redundant JSON polyfills for PHP native functionality

Reported by: jrf's profile jrf Owned by: desrosj's profile 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:

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:

In addition to this, it:

  • adds the _json_decode_object_helper()function, the WP_JSON_SERIALIZE_COMPATIBLE constant and the $wp_json global variable in the wp-includes/compat.php file.
  • contains various work-arounds for differences between PHP versions in the wp_json_encode() function in the wp-includes/functions.php file, including the _wp_json_prepare_data() function which is 100% work-around code.
  • adds various constants, a Services_JSON and a Services_JSON_Error class in the wp-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 in wp-admin/includes/noop.php
  • json_decode() polyfill
  • _json_decode_object_helper() function which would only be declared if the PHP native json_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 native json_encode() and json_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)

47699-remove-json-polyfills-and-work-arounds.patch (57.3 KB) - added by jrf 5 years ago.
47699-deprecate-JSON-polyfills.patch (14.6 KB) - added by jrf 5 years ago.
Agressively deprecate all JSON related polyfills & make the JSON extension a requirement
47699-WP-5.2.x-site-health-json-required.patch (886 bytes) - added by jrf 5 years ago.
WP 5.2.x: make JSON a required extension for Site_Health, patch created against the WP 5.2 branch
47699-WP-5.3-deprecate-services-json.patch (7.6 KB) - added by jrf 5 years ago.
WP 5.3: Agressively deprecate the Services_JSON and the Services_JSON_Error classes
47699-WP-5.3-remove-JSON-related-work-arounds.patch (10.5 KB) - added by jrf 5 years ago.
WP 5.3: Remove JSON extension related work-arounds
47699-WP-5.3-remove-JSON-related-polyfills.patch (4.6 KB) - added by jrf 5 years ago.
WP 5.3: Remove JSON extension related polyfills
47699-add-deprecated-docblock-tags.patch (7.4 KB) - added by jrf 4 years ago.
Services_Json: add @deprecated tags - I'd neglected to do so in the previous patch as committed in [46205]
47699-update-warning.diff (826 bytes) - added by desrosj 4 years ago.
47699-specific-code.diff (632 bytes) - added by desrosj 4 years ago.

Download all attachments as: .zip

Change History (48)

#1 @Clorith
5 years ago

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.

#2 @jrf
5 years 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.
Last edited 5 years ago by jrf (previous) (diff)

#3 @Clorith
5 years 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 :) )

@jrf
5 years ago

Agressively deprecate all JSON related polyfills & make the JSON extension a requirement

#4 @jrf
5 years 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 the json_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.

Last edited 5 years ago by jrf (previous) (diff)

#5 @pento
5 years 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: @pento
5 years 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 @jrf
5 years 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.

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.

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.

@jrf
5 years ago

WP 5.2.x: make JSON a required extension for Site_Health, patch created against the WP 5.2 branch

@jrf
5 years ago

WP 5.3: Agressively deprecate the Services_JSON and the Services_JSON_Error classes

@jrf
5 years ago

WP 5.3: Remove JSON extension related work-arounds

@jrf
5 years ago

WP 5.3: Remove JSON extension related polyfills

#8 @jrf
5 years 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 the Services_JSON and the Services_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.


5 years ago

#10 @desrosj
5 years ago

  • Owner set to desrosj
  • Status changed from new to reviewing

#11 @desrosj
5 years ago

In 46205:

Code Modernization: Deprecate the Services_JSON and Services_JSON_Error classes.

The PHP native JSON extension has been bundled and compiled with PHP by default since version 5.2.0. Because the minimum version of PHP required by WordPress is now 5.6.20 (see #46594 and [45058]), JSON extension related polyfills and backwards compatibility code can now be removed.

Since there are still some plugins and themes that utilize the Services_JSON class, these classes will remain for the time being, but including the wp-includes/class-json.php file and creating Services_JSON instances will now throw deprecated notices.

See #47699.
Props jrf, Clorith, pento.

#12 @desrosj
5 years ago

In 46206:

Code Modernization: Remove JSON extension workarounds for PHP < 5.6.

The PHP native JSON extension has been bundled and compiled with PHP by default since version 5.2.0. Because the minimum version of PHP required by WordPress is now 5.6.20 (see #46594 and [45058]), JSON extension related polyfills and backwards compatibility code can now be removed.

This change removes code that supported JSON related functionality on older versions of PHP. This includes (but is not limited to) checks that json_last_error() exists, checking and setting the JSON_UNESCAPED_SLASHES and JSON_PRETTY_PRINT constants if not previously defined, and deprecating the _wp_json_prepare_data() function (which was 100% workaround code).

Follow up of [46205].

See #47699.
Props jrf, Clorith, pento.

#13 @desrosj
5 years ago

In 46208:

Code Modernization: Remove JSON related polyfills.

The PHP native JSON extension has been bundled and compiled with PHP by default since version 5.2.0. Because the minimum version of PHP required by WordPress is now 5.6.20 (see #46594 and [45058]), JSON extension related polyfills and backwards compatibility code can now be removed.

This change removes the json_last_error_msg() and JsonSerializable polyfills included in WordPress for full JSON extension support in PHP < 5.6.

Follow up of [46205-46206].

See #47699.
Props jrf, Clorith, pento.

#14 follow-up: @desrosj
5 years 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 @jorbin
5 years 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 @desrosj
5 years 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 @dd32
5 years 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

Last edited 5 years ago by dd32 (previous) (diff)

#18 follow-up: @jrf
5 years 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 @dd32
5 years 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 @jrf
5 years 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 ?

@jrf
4 years ago

Services_Json: add @deprecated tags - I'd neglected to do so in the previous patch as committed in [46205]

#21 @desrosj
4 years ago

In 46377:

Docs: Add missing @deprecated tags.

Follow up to [46205-46206,46208].

Props jrf.
See #47699.

#22 follow-up: @desrosj
4 years 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 @jrf
4 years 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 @SergeyBiryukov
4 years 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.


4 years ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 years ago

#27 @desrosj
4 years ago

  • Keywords i18n-change added

Marking this as a string change since we are pasts soft freeze.

#28 in reply to: ↑ 22 @jorbin
4 years 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!

#29 @desrosj
4 years ago

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

In 46455:

Upgrade/Install: Detect the presence of the native PHP JSON extension before updating.

The PHP native JSON extension has been bundled and compiled with PHP by default since version 5.2.0. Because the minimum version of PHP required by WordPress is now 5.6.20 (see #46594 and [45058]), the related polyfills and workarounds have been removed (see [46205-46206,46208]).

However, even though the JSON extension is now included in PHP by default, it is still possible to disable the extension in a custom configuration. This change will prevent sites from upgrading if the JSON extension is disabled to prevent compatibility issues.

Props jrf, jorbin, dd32, desrosj.
Fixes #47699.

#30 @desrosj
4 years 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 @jrf
4 years 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

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#32 @SergeyBiryukov
4 years ago

In 46464:

I18N: Don't capitalize "Extension" in update_core(), for consistency with other strings.

Follow-up to [46455].

See #47699.

#33 @desrosj
4 years 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.

#34 @jorbin
4 years ago

  • Keywords dev-feedback commit added

Change looks good to me.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 years ago

#37 @johnbillion
4 years ago

+1 from me too

#38 @desrosj
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

#39 @desrosj
4 years ago

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

In 46560:

Upgrade/Install: Use a unique error code when an update fails due to the PHP JSON extension missing.

This allows update failures caused when the native PHP JSON extension is missing to be distinguished from updates that fail because the site does not meet the minimum PHP requirements.

Follow up of [46455].

Reviewed by desrosj, jorbin, johnbillion.
Fixes #47699.

Note: See TracTickets for help on using tickets.