Make WordPress Core

Opened 3 years ago

Last modified 5 months ago

#60355 new enhancement

Can't save object to metadata with `readonly` properties

Reported by: Cawa-93 Owned by:
Priority: low Milestone: Future Release
Component: General Version:
Severity: major Keywords: needs-docs has-patch needs-unit-tests
Cc: Focuses:

Description

If I have class with readonly property

<?php
class Foo {
  __constructor(public readonly int $prop) {}

It throws an exception if you try to save it to medatata:

update_metadata('post', 1, 'key', new Foo(1));
Error:
Cannot modify readonly property App\Namespace\Foo::$prop

  at /var/www/html/web/wp/wp-includes/formatting.php:5148
  at map_deep(object(Foo), 'stripslashes_from_strings_only')
     (/var/www/html/web/wp/wp-includes/formatting.php:5148)
  at map_deep(object(Foo), 'stripslashes_from_strings_only')
     (/var/www/html/web/wp/wp-includes/formatting.php:5143)
  at map_deep(array(object(Foo), object(Foo), object(Foo)), 'stripslashes_from_strings_only')
     (/var/www/html/web/wp/wp-includes/formatting.php:2855)
  at stripslashes_deep(array(object(Foo), object(Foo), object(Foo)))
     (/var/www/html/web/wp/wp-includes/formatting.php:5804)
  at wp_unslash(array(object(Foo), object(Foo), object(Foo)))
     (/var/www/html/web/wp/wp-includes/meta.php:208)
  at update_metadata('post', 1, 'key', array(object(Foo), object(Foo), object(Foo)), '')
     (/var/www/html/web/wp/wp-includes/post.php:2558)

The issue cause in map_deep function since it tries to overwrite properties

        $object_vars = get_object_vars( $value );
        foreach ( $object_vars as $property_name => $property_value ) {
            $value->$property_name = map_deep( $property_value, $callback ); // <---------
        }

Attachments (1)

60355.diff (737 bytes ) - added by abditsori 3 years ago.

Download all attachments as: .zip

Change History (10)

#1 @azaozz
3 years ago

  • Keywords needs-docs added
  • Milestone Awaiting Review6.5
  • Severity majorminor
  • Type defect (bug)enhancement

This sounds like PHP limitation, maybe check https://www.php.net/manual/en/language.oop5.serialization.php.

Think this is a documentation problem in WP. Imho the docblocks for adding options and meta should state clearly that saving class instances or PHP objects to the DB is not recommended and that only simple objects are supported (like plain name->value pairs?). Also that the preferable format to save to the DB are strings or associative arrays, not objects.

Setting this as a "documentation enhancement" for 6.5.

Last edited 3 years ago by azaozz (previous) (diff)

#2 @azaozz
3 years ago

  • Component FormattingGeneral

Setting this to "General" as there is no "Documentation" component...

@abditsori
3 years ago

#3 @abditsori
3 years ago

I think this issue is caused by PHP 8.1's readonly class properties. (https://php.watch/versions/8.1/readonly)

It is pretty straight forward to fix it as in the patch I attached or this PR (https://github.com/WordPress/wordpress-develop/pull/5956/files)

Last edited 3 years ago by abditsori (previous) (diff)

#4 @swissspidy
3 years ago

Think this is a documentation problem in WP. Imho the docblocks for adding options and meta should state clearly that saving class instances or PHP objects to the DB is not recommended and that only simple objects are supported (like plain name->value pairs?). Also that the preferable format to save to the DB are strings or associative arrays, not objects.

Big +1 to this. Storing class instances in database usually causes more harm than good. Not only can it cause issues like this, but it can also inflate the database size and cause potential security issues.

As for the patch, it would need a slight update to not crash on PHP 7.0+, as isReadOnly() is only available since 8.1.

@abditsori FYI, no need to upload a patch file _and_ submit a PR. A PR is enough.

#5 @audrasjb
3 years ago

  • Milestone 6.5Future Release

As we are close to WP 6.5 beta 1 and still waiting for a patch, let's move this ticket to Future Release milestone.

#6 @audrasjb
3 years ago

  • Focuses docs added

#7 @johnbillion
10 months ago

  • Focuses docs removed
  • Keywords has-patch needs-refresh needs-unit-tests added
  • Priority normallow
  • Severity minormajor

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


5 months ago
#8

  • Keywords needs-refresh removed

Trac ticket: https://core.trac.wordpress.org/ticket/60355

On PHP 8.1+, passing an object with readonly properties through map_deep() causes a fatal error because the function attempts to reassign every property after applying the callback.

This affects any code path that stores objects in metadata, since update_metadata()wp_unslash()stripslashes_deep() calls map_deep() internally.

This PR adds a PHP_VERSION_ID >= 80100 guard inside map_deep() that uses ReflectionProperty::isReadOnly() to detect and skip readonly properties.

WIP: Will be adding unit tests for this shortly.

#9 @abcd95
5 months ago

I've been working on this and attempted to refresh the patch and tested on PHP 8.3.30 / WP 7.0-beta6.

The original patch was almost right, but was missing a php version guard since ReflectionProperty::isReadOnly() only exists on PHP 8.1+, so calling it on lower versions would itself cause a fatal.

The fix skips readonly properties in map_deep() via continue. Since readonly values are immutable after construction, the callback could never have a lasting effect on them anyway and skipping would be semantically correct.

Confirmed that:

  • The unpatched map_deep() crashes with "Cannot modify readonly property" on mixed, all-readonly, and nested-array-of-readonly objects.
  • The patched version passes all cases: readonly properties are preserved, mutable properties are still correctly transformed, and plain stdClass objects (regression check) work exactly as before.

discussions are welcomed.

Note: See TracTickets for help on using tickets.