WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 4 days ago

#47699 new enhancement

Remove redundant JSON polyfills for PHP native functionality

Reported by: jrf Owned by:
Milestone: 5.3 Priority: normal
Severity: normal Version: trunk
Component: General Keywords: has-patch has-unit-tests needs-dev-note
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 (6)

47699-remove-json-polyfills-and-work-arounds.patch (57.3 KB) - added by jrf 5 weeks ago.
47699-deprecate-JSON-polyfills.patch (14.6 KB) - added by jrf 5 weeks 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 4 weeks 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 4 weeks 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 4 weeks 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 4 weeks ago.
WP 5.3: Remove JSON extension related polyfills

Download all attachments as: .zip

Change History (15)

#1 @Clorith
5 weeks 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 weeks 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 weeks ago by jrf (previous) (diff)

#3 @Clorith
5 weeks 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 weeks ago

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

#4 @jrf
5 weeks 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 weeks ago by jrf (previous) (diff)

#5 @pento
5 weeks 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
4 weeks 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
4 weeks 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
4 weeks ago

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

@jrf
4 weeks ago

WP 5.3: Agressively deprecate the Services_JSON and the Services_JSON_Error classes

@jrf
4 weeks ago

WP 5.3: Remove JSON extension related work-arounds

@jrf
4 weeks ago

WP 5.3: Remove JSON extension related polyfills

#8 @jrf
4 weeks 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.


4 days ago

Note: See TracTickets for help on using tickets.