Make WordPress Core

Opened 23 months ago

Last modified 6 weeks ago

#60355 new enhancement

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

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

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 23 months ago.

Download all attachments as: .zip

Change History (8)

#1 @azaozz
23 months ago

  • Keywords needs-docs added
  • Milestone changed from Awaiting Review to 6.5
  • Severity changed from major to minor
  • Type changed from defect (bug) to 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 23 months ago by azaozz (previous) (diff)

#2 @azaozz
23 months ago

  • Component changed from Formatting to General

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

@abditsori
23 months ago

#3 @abditsori
23 months 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 23 months ago by abditsori (previous) (diff)

#4 @swissspidy
22 months 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
22 months ago

  • Milestone changed from 6.5 to Future 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
22 months ago

  • Focuses docs added

#7 @johnbillion
6 weeks ago

  • Focuses docs removed
  • Keywords has-patch needs-refresh needs-unit-tests added
  • Priority changed from normal to low
  • Severity changed from minor to major
Note: See TracTickets for help on using tickets.