Re: [PHP-DEV] [RFC] Deprecate dynamic properties

This is only part of a thread. view whole thread
  116360
November 15, 2021 10:26 nicolas.grekas+php@gmail.com (Nicolas Grekas)
Hi Nikita, hi everybody,

Le mer. 25 août 2021 à 12:03, Nikita Popov ppv@gmail.com> a écrit :

> Hi internals, > > I'd like to propose the deprecation of "dynamic properties", that is > properties that have not been declared in the class (stdClass and > __get/__set excluded, of course): > > https://wiki.php.net/rfc/deprecate_dynamic_properties > > This has been discussed in various forms in the past, e.g. in > https://wiki.php.net/rfc/locked-classes as a class modifier and > https://wiki.php.net/rfc/namespace_scoped_declares / > > https://github.com/nikic/php-rfcs/blob/language-evolution/rfcs/0000-language-evolution.md > as a declare directive. > > This RFC takes the more direct route of deprecating this functionality > entirely. I expect that this will have relatively little impact on modern > code (e.g. in Symfony I could fix the vast majority of deprecation warnings > with a three-line diff), but may have a big impact on legacy code that > doesn't declare properties at all. >
Thanks for the RFC, it makes sense to me and I support the move. Since Symfony is mentioned in the RFC, I thought you might want to know about this PR, that removes dynamic properties from the Symfony codebase: https://github.com/symfony/symfony/pull/44037/files What Nikita describes in the RFC is correct: declaring+unsetting the "groups" property works. There's just one more thing I had to do; I also had to replace two calls to property_exists: - if (!property_exists($this, 'groups')) { + if (!isset(((array) $this)['groups'])) { The rest are test cases where we've been lazily accepting fixtures with undeclared properties. No big deal, and I'm happy the engine might soon help us get a bit stricter in this regard. I read that some think that this PR is not required because static analysers (SA) can report when a dynamic property is used. Although that's correct, I think it would be detrimental to PHP as a language if SA tools (or any tools actually) were a requirement to code in the safe-n-modern way. You should not have to install any complex safeguarding tooling infrastructure to start coding; both for newcomers, but also for no-so-new-comers. About the discussion related to deprecations. I've yet to see a better reporting system than the current one. It's true that too many userland error handlers are throwing instead of collecting/logging/skipping deprecations. But these can be fixed (and many are being fixed these days, which is nice!) Cheers, Nicolas
  116362
November 15, 2021 11:23 kontakt@beberlei.de (Benjamin Eberlei)
On Mon, Nov 15, 2021 at 11:26 AM Nicolas Grekas <
nicolas.grekas+php@gmail.com> wrote:

> Hi Nikita, hi everybody, > > Le mer. 25 août 2021 à 12:03, Nikita Popov ppv@gmail.com> a écrit > : > > > Hi internals, > > > > I'd like to propose the deprecation of "dynamic properties", that is > > properties that have not been declared in the class (stdClass and > > __get/__set excluded, of course): > > > > https://wiki.php.net/rfc/deprecate_dynamic_properties > > > > This has been discussed in various forms in the past, e.g. in > > https://wiki.php.net/rfc/locked-classes as a class modifier and > > https://wiki.php.net/rfc/namespace_scoped_declares / > > > > > https://github.com/nikic/php-rfcs/blob/language-evolution/rfcs/0000-language-evolution.md > > as a declare directive. > > > > This RFC takes the more direct route of deprecating this functionality > > entirely. I expect that this will have relatively little impact on modern > > code (e.g. in Symfony I could fix the vast majority of deprecation > warnings > > with a three-line diff), but may have a big impact on legacy code that > > doesn't declare properties at all. > > > > Thanks for the RFC, it makes sense to me and I support the move. > > Since Symfony is mentioned in the RFC, I thought you might want to know > about this PR, that removes dynamic properties from the Symfony codebase: > https://github.com/symfony/symfony/pull/44037/files > > What Nikita describes in the RFC is correct: declaring+unsetting the > "groups" property works. > There's just one more thing I had to do; I also had to replace two calls to > property_exists: > > - if (!property_exists($this, 'groups')) { > + if (!isset(((array) $this)['groups'])) { > > The rest are test cases where we've been lazily accepting fixtures with > undeclared properties. No big deal, and I'm happy the engine might soon > help us get a bit stricter in this regard. > > I read that some think that this PR is not required because static > analysers (SA) can report when a dynamic property is used. Although that's > correct, I think it would be detrimental to PHP as a language if SA tools > (or any tools actually) were a requirement to code in the safe-n-modern > way. You should not have to install any complex safeguarding tooling > infrastructure to start coding; both for newcomers, but also for > no-so-new-comers. >
Its not so true from my POV that static analysis can avoid having this deprecation: 1. static analysis does not work for dynamic assignments, $object = new SomeDataObject(); $row = $pdo->fetch(); foreach ($row as $column => $value) { $object->$column = $value; } arguably this is one of the important use cases this deprecation fixes. A second example of this is when doing deserialization into an object from JSON or XML: $object = new SomeDataObject(); $objectPayload = json_decode($input, true); foreach ($objectPayload as $prop => $value) { $object->$prop = $value; } This doesn't apply sole to user input where maybe more validation of input is necessary, but also for mapping config files to an object. All this kind of generic code cannot be statically analysed, but this deprecation and removal has the most value in exactly that use-case.
> > About the discussion related to deprecations. I've yet to see a better > reporting system than the current one. > It's true that too many userland error handlers are throwing instead of > collecting/logging/skipping deprecations. > But these can be fixed (and many are being fixed these days, which is > nice!) > > Cheers, > Nicolas >
  116366
November 15, 2021 12:29 tekiela246@gmail.com (Kamil Tekiela)
I would be very sad to see this RFC not go through. I have voted Yes as I
believe this "feature":is a bug that needs to be fixed. There is also an
opt-in proposed for people who really do consider it a feature.

I don't see why it would cause much trouble for maintainers of OSS. At the
moment it is proposed to make this change in PHP 9.0, which is a couple
years away. That is a lot of time to fix the code. The deprecation message
will inform us about the number of uses, whether accidental or intentional.
If we decide that removal of this feature would cause too much disruption,
could we not have RFC in PHP 8.3 to remove the deprecation?

Deprecated or not, I still believe that OSS should avoid dynamic
properties. They are really difficult to identify, even with static
analysers. Having the deprecation message would at least help us identify
where dynamic properties were used, so that we can fix it.