Make WordPress Core

Opened 2 years ago

Last modified 11 months ago

#55257 new defect (bug)

map_deep() function incompatibility with incomplete objects in PHP 8.0+

Reported by: codex-m's profile codex-m Owned by:
Milestone: Future Release Priority: normal
Severity: major Version: 5.9.1
Component: Formatting Keywords: has-patch php80 has-unit-tests 2nd-opinion dev-feedback
Focuses: Cc:

Description

Incomplete objects (__PHP_Incomplete_Class) used to have no issues with map_deep() before PHP 8.0 because it ignores any incomplete object. With PHP 8.0 - this now returns fatal error and PHP cannot continue processing. This disrupts an otherwise normal processing (before PHP 8.0).

Example of uncaught error in debug.log are as follows:

PHP Fatal error: Uncaught Error: The script tried to modify a property on an incomplete object. Please ensure that the class definition "SuperCustomXyz" of the object you are trying to operate on was loaded _before_ unserialize() gets called or provide an autoloader to load the class definition in /wp/wp-includes/formatting.php:4998

For the map_deep() function to be truly compatible with PHP 8.0 ( as it was with earlier PHP versions) - can we modify this to check for incomplete objects? And if the value to be processed is an incomplete object - we skipped processing. This will make the function to behave like it was being used before PHP 8.0. As a result - the function still returns an array with incomplete objects as it was used to be working.

This issue can be reproduced as follows:

Environment:

  • PHP 8.0
  • WordPress 5.9.1
  • Create a dummy test PHP script as follows in the WP root directory:
<?php
require_once 'wp-load.php';

$test_data = 'a:2:{i:0;O:14:"SuperCustomXyz":1:{s:17:" SuperCustomXyz x";s:4:"test";}s:8:"testdata";a:2:{s:1:"a";i:1;s:1:"b";i:2;}}';
$unserialized = maybe_unserialize($test_data);
    
$res = map_deep($unserialized, 'stripslashes_from_strings_only');
print_r($res);

Using PHP 8.0+ - this now returns fatal error. In PHP 7.4 and below (where most of the WordPress sites are still using), this returns an array with an incomplete object:

Array
(
    [0] => __PHP_Incomplete_Class Object
        (
            [__PHP_Incomplete_Class_Name] => SuperCustomXyz
            [ SuperCustomXyz x] => test
        )

    [testdata] => Array
        (
            [a] => 1
            [b] => 2
        )

)

Attachments (3)

map_deep_check_incomplete_objects.diff (629 bytes) - added by codex-m 2 years ago.
Added checks for incomplete objects in map_deep function
#55257.patch (563 bytes) - added by rehanali 2 years ago.
Added patch
55257.map_deep_check_incomplete_objects.2.diff (627 bytes) - added by costdev 2 years ago.
Patch refresh with WPCS fixes.

Download all attachments as: .zip

Change History (21)

@codex-m
2 years ago

Added checks for incomplete objects in map_deep function

#1 @SergeyBiryukov
2 years ago

  • Keywords has-patch php8 added
  • Milestone changed from Awaiting Review to 6.0

@rehanali
2 years ago

Added patch

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


2 years ago

@costdev
2 years ago

Patch refresh with WPCS fixes.

#3 @costdev
2 years ago

  • Milestone changed from 6.0 to 6.1

Per the discussion in the bug scrub, I'm moving this ticket to the 6.1 milestone.

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


22 months ago

#5 @mukesh27
22 months ago

Per the discussion in the bug scrub, @SergeyBiryukov Is it in your radar for 6.1?

#6 @SergeyBiryukov
22 months ago

  • Milestone changed from 6.1 to 6.2

Hi there, welcome back to WordPress Trac! Thanks for the ticket and the patch.

I think this could use a unit test with the example from the ticket description.

With 6.1 RC1 today, moving to 6.2 for now.

This ticket was mentioned in PR #3841 on WordPress/wordpress-develop by @petitphp.


18 months ago
#7

  • Keywords has-unit-tests added

#8 @petitphp
18 months ago

I've created a PR using the last patch with new unit tests.

Currently, the tests are failing for all PHP versions below 7.2. This come from an old behavior in versions <=7.1 where is_object return false for __PHP_Incomplete_Class (see: https://www.php.net/manual/en/function.is-object.php / https://3v4l.org/iPBeN).

I think it's safe to remove the check for is_object and only keep the instanceof in the condition.

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


17 months ago

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


17 months ago

#11 @azaozz
17 months ago

  • Keywords 2nd-opinion added

Incomplete objects (__PHP_Incomplete_Class) used to have no issues with map_deep() before PHP 8.0 because it ignores any incomplete object. With PHP 8.0 - this now returns fatal error

The question here, imho, is whether the fatal error in PHP 8.0 is a "feature" or should it be considered a bug :)

If it is a feature, perhaps it shouldn't be prevented when WP_DEBUG is true. An incomplete class definitely means something's wrong, right?

#12 @costdev
17 months ago

According to PHP 7.4.x - 8.0.x - Backward Incompatible changes, some warnings and notices got a bump in error level in PHP 8.0, including, for example:

Attempting to write to a property of a non-object. Previously this implicitly created an stdClass object for null, false and empty strings.


The issue reported in this ticket was previously a Notice, as shown here.

Since this Notice, and now Fatal Error, are there to tell you that the class is not defined, I'm not totally sure what the aim of preventing the Fatal Error is.

Sure, it would prevent execution being halted, but it would also perpetuate acting on an object of an undefined class.

So, is there a valid use case for this? For example, a setting whose value includes an object of a class defined in a plugin that's no longer active?

Also pinging @audrasjb and @hellofromTonya as 6.2 Core Tech Leads, as the patch seems close to commit-ready if this change is desired.

#13 @hellofromTonya
17 months ago

  • Milestone changed from 6.2 to 6.3

I think this ticket needs more discussion and consideration. With Beta 4 is today and RC1 is next week, moving this ticket to the next cycle.

#14 @codex-m
17 months ago

Thanks guys for all the inputs. Yes, this can happen when we update settings for plugins / themes that are no longer active. A case example is when a plugin do mass updating user or post meta to update something in dB for maintenance purposes (using core functions).

And then we have these settings in dB for plugins / themes that no longer exist (so their class definition is incomplete). This updating will complete with PHP 7.4 and below but now breaks with PHP 8.0+. The updating will not be completed.

Yes, I agree that with PHP 8.0 handling makes this error more visible (because of fatal error). I added this ticket because I think it would help WordPress become more backward compatible with PHP 8.0. This is just a suggestion :)

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


13 months ago

#16 @oglekler
13 months ago

  • Keywords dev-feedback added

This ticket was discussed in the recent bug scrub and due to lack of activity it was suggested to move it into future release but there is a patch with unit test as well already, and this ticket needs feedback from devs if this expected behaviour or it should be fixes with a patch, so, we are keeping it a bit longer in the current Milestone 6.3 for further consideration.

Props to @Clorith

#17 @oglekler
13 months ago

  • Milestone changed from 6.3 to Future Release

Due to lack of activity I am moving this ticket into future release.

#18 @hellofromTonya
11 months ago

  • Keywords php80 added; php8 removed

Ticket changes:

  • Changed php8 to php80 to denote PHP 8.0 is the version that increased the error severity.

I didn't add php-compatibility focus.

The discussion of whether map_deep() should or shouldn't handle an incomplete object is itself not necessarily an incompatibility with PHP 8.0.

Note: See TracTickets for help on using tickets.