Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115817
MIME-Version: 1.0
References: <CAF+90c9_qu=PiTS81J2mesPfJdz7n5nSi3qoLNLjhEGN-aTytA@mail.gmail.com>
 <DM6PR07MB6618AF81A6985C5E31C1E4CBF9C69@DM6PR07MB6618.namprd07.prod.outlook.com>
In-Reply-To: <DM6PR07MB6618AF81A6985C5E31C1E4CBF9C69@DM6PR07MB6618.namprd07.prod.outlook.com>
Date: Wed, 25 Aug 2021 16:49:10 +0200
Message-ID: <CAF+90c_LF+SuXWBKF6m6wsyEaCveGAVvFOa7KaOJbJTOPSKFtA@mail.gmail.com>
To: tyson andre <tysonandre775@hotmail.com>
Cc: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="00000000000053955d05ca635b29"
Subject: Re: [PHP-DEV] [RFC] Deprecate dynamic properties
From: nikita.ppv@gmail.com (Nikita Popov)

--00000000000053955d05ca635b29
Content-Type: text/plain; charset="UTF-8"

On Wed, Aug 25, 2021 at 4:26 PM tyson andre <tysonandre775@hotmail.com>
wrote:

> Hi Nikita Popov,
>
> > 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.
>
> I'd had some concerns.
>
> It might be too soon after your addition of WeakMap.
> https://www.php.net/weakmap was introduced in PHP 8.0 (and WeakReference
> in 7.4),
> so applications/libraries fixing the deprecation may need to drop support
> for php 7.x early.
> Applications attempting to polyfill a WeakMap in earlier PHP versions
> would potentially leak a lot of memory in php 7.x.
>
> - I don't know how many minor versions to expect before 9.0 is out
> - Is it feasible for a developer to create a native PECL polyfill for
> WeakMap for earlier PHP versions that has a
>   subset of the functionality the native weak reference counting does?
>   (e.g. to only free polyfilled weak references when cyclic garbage
> collection is triggered and the reference count is 1).
>

For compatibility with < PHP 7.4 in this use case I'd suggest to either
suppress the deprecation for a particular assignment, or to pick the used
mechanism depending on PHP version.

We don't have an official schedule for major versions, but going by the
last few releases, I'd guess that it happens after PHP 8.4, or around that
time :) I think an approximate PECL polyfill (that tries to free on GC)
would be possible, but I'm not sure it would really be useful relative to
having version-dependent behavior.


> Additionally, it makes it less efficient (but still feasible) to associate
> additional fields
> with libraries or native classes/PECLs you don't own (especially for
> circular data structures), especially if they need to be serialized later.
>

Efficiency probably depends on the case here -- while I'd expect the
dynamic property to be more efficient on access than the WeakMap, it will
likely also use more memory, sometimes much more.


> (it isn't possible to serialize WeakMap, and the WeakMap would have fields
> unrelated to the data being serialized)
> I guess you can have a wrapper class that iterates over a WeakMap to
> capture and serialize the values that still exist in SplObjectStorage,
> though.
> (Though other languages do just fine without this functionality)
>

Right, the WeakMap approach will not impact the serialization of the
object, as well as other property based behaviors, such as comparison
semantics. I would usually count that as an advantage (what you do
shouldn't impact the behavior of 3rd-party classes), but there's probably
use cases for everything :)


> I'm not sure if a library owner would want to change their class hierarchy
> to extend stdClass (to avoid changing the behavior of anything using `$x
> instanceof stdClass`) and the attribute/trait approach might be more
> acceptable to library owners.
> E.g.
> https://github.com/vimeo/psalm/blob/master/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php
> would set a dynamic property `$stmt->pure` in
> `PhpParser\Node\Expr\FuncCall $stmt` in a vendor dependency on php-parser.
>

I wouldn't want library authors to change their class hierarchy for
3rd-party use cases, unless those use cases are actually fundamental to the
library in some way. For PHP-Parser this is actually the case, which is why
it offers the Node::setAttribute() API. The intended way to do this would
be $stmt->setAttribute('pure', true) in this case.

Regards,
Nikita

--00000000000053955d05ca635b29--