Strict properties and unserialization

php.internals

Pierrick Charron

4 years ago
Hi internals, We (Sergey and I) would like to introduce you to a problem [1] that was reported by Tim Düsterhus and others about the Strict Properties RFC [2] that was implemented in PHP8.2. The RFC missed a part about how PHP should handle unserialization of objects with undefined properties. Example : class Foo {} $foo = unserialize('O:3:"Foo":1:{s:3:"bar";i:42;}'); In this case PHP will not trigger any deprecation warnings but it probably should. It was something that was not thought about while writing the RFC (at least the RFC doesn't mention anything about this). PHP8.2beta3 will be announced this Thursday, and the next scheduled version is supposed to be RC1. So we are pretty late in the pre-release cycle. We have multiple options : First we could just do nothing in PHP8.2 about this and leave it like that and only fix it in PHP8.3. This is probably the way we should do it if we want to stick to the initial PHP8.2 release cycle calendar. The second option would be to fix this in PHP8.2 before RC1. We don't have any idea on how big the patch is and it may influence the final decision. If patching it for 8.2 is the best way to do, we may want to add another Beta (Beta4) and delay the RC1 release by 2 weeks. We would like to hear your thoughts about this issue. Regards Sergey and Pierrick [1] https://github.com/php/php-src021-11-26./issues/9186 <https://github.com/php/php-src/issues/9186> [2] https://wiki.php.net/rfc/deprecate_dynamic_properties

Tim Düsterhus

4 years ago
Hi On 8/16/22 18:33, Pierrick Charron wrote:
> In this case PHP will not trigger any deprecation warnings but it probably > should. It was something that was not thought about while writing the RFC > (at least the RFC doesn't mention anything about this).
Note that for native classes (e.g. the Randomizer from my issue), creating an unknown property is not just a deprecation, but an Error if '@strict-properties' is set:
> Fatal error: Uncaught Error: Cannot create dynamic property Random\Randomizer::$foo in php-src/test.php:4 > Stack trace: > #0 {main} > thrown in php-src/test.php on line 4
---
> First we could just do nothing in PHP8.2 about this and leave it like that > and only fix it in PHP8.3. This is probably the way we should do it if we > want to stick to the initial PHP8.2 release cycle calendar. > The second option would be to fix this in PHP8.2 before RC1. We don't have > any idea on how big the patch is and it may influence the final decision. > If patching it for 8.2 is the best way to do, we may want to add another > Beta (Beta4) and delay the RC1 release by 2 weeks. > > We would like to hear your thoughts about this issue.
The above example (ZEND_ACC_NO_DYNAMIC_PROPERTIES) technically even affects PHP 8.0 [1]. As such this is not a new issue, but I would recommend treating this as an actual bug and fixing this in PHP 8.2, because my understanding is that this is intended to protect internal classes: https://github.com/php/php-src/commit/653e4ea1c57def2d5cd75b7da9e3943a841b7d6c and https://github.com/php/php-src/pull/5533#discussion_r422474377 It might be useful to include nikic in the discussion here? [1] In 8.0 none of the native classes having that flag are unserializable, though. Best regards Tim Düsterhus