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

This is only part of a thread. view whole thread
  115848
August 26, 2021 08:34 nikita.ppv@gmail.com (Nikita Popov)
On Wed, Aug 25, 2021 at 5:28 PM Sara Golemon <pollita@php.net> wrote:

> On Wed, Aug 25, 2021 at 5:03 AM Nikita Popov ppv@gmail.com> wrote: > > 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 RFC offers `extends stdClass` as a way to opt-in to the use of > dynamic properties. > > > This makes the assumption that existing uses of dynamic properties are all > root classes. I don't think that assumption can be made. > > > Some people have suggested that we could use a magic marker > > interface (implements SupportsDynamicProperties), > > an attribute (#[SupportsDynamicProperties]) > > or a trait (use DynamicProperties;) instead. > > > My gut-check says an Attribute works well enough. This will unlock the > class (disable deprecation warning) in 8.2+ and be ignored in earlier > releases (8.0 and 8.1 would need an auto-loaded polyfill userspace > attribute obviously, but that's a tractable problem). > > #[SupportsDynamicProperties] > class Foo { ... } >
The crux of the issue is what our end goal is: 1. Require users to explicitly annotate classes that use dynamic properties, but otherwise keep dynamic properties as a fully supported part of the core language. 2. Remove support for dynamic properties entirely. The core language only has declared properties and __get/__set, and nothing else. stdClass is just a very efficient implementation of __get/__set. The RFC is currently written under the assumption that the end goal is (2). To support that, I believe that "extends stdClass" and "use DynamicProperties" are the only viable approaches. The former, because it inherits stdClass behavior, the latter because it's literally __get/__set. An attribute would require retaining the capability to have arbitrary classes with dynamic properties. Now, if the actual goal is (1) instead, then I fully agree that using a #[SupportsDynamicProperties] attribute is the ideal solution. It can be easily applied anywhere and has no BC concerns. Heck, someone who just doesn't care could easily slap it on their whole codebase without spending brain cycles on more appropriate solutions. I do think that just (1) by itself would already be very worthwhile. If that's all we can reasonably target, then I'd be fine with the #[SupportsDynamicProperties] solution. Though if (2) is viable without too many complications, then I think that would be the better final state to reach.
> > The Locked classes RFC took an alternative approach to this problem > space: > > Rather than deprecating/removing dynamic properties and providing an > opt-in > > for specific classes, it instead allowed marking specific classes as > locked in > > order to forbid creation of dynamic properties on them. > > > > I don't believe that this is the right strategy, because in contemporary > code, > > classes being “locked” is the default state, while classes that require > dynamic > > properties are a rare exception. Additionally, this requires that class > owners > > (which may be 3rd party packages) consistently add the “locked” keyword > > to be effective. > > > I struggle here, because the opt-in approach is the most BC, but I > actually think you're right. I think[citation needed] that dynamic props > are probably rare enough that as long as the escape hatch is clear, we can > be a little more bold about nudging the ecosystem forward. I will counter > however, that the same issue with 3rd party libraries applies to opt-out as > opt-in. A third party library that's undermaintained (and uses dynamic > props) won't be able to be used out of the box once we apply this. I don't > think that's enough to scare us off, it just means that the opt-in side of > that argument cancels out. >
I think the argument isn't quite symmetric: A 3rd-party library in maintenance-only mode has essentially no incentive to go out of their way and add a "locked" keyword/attribute to their classes. Adding some missing property declarations to keep working on new PHP versions is a different matter. Regards, Nikita
  115871
August 26, 2021 22:10 smalyshev@gmail.com (Stanislav Malyshev)
Hi!

> The crux of the issue is what our end goal is: > > 1. Require users to explicitly annotate classes that use dynamic > properties, but otherwise keep dynamic properties as a fully supported part > of the core language. > 2. Remove support for dynamic properties entirely. The core language only > has declared properties and __get/__set, and nothing else. stdClass is just > a very efficient implementation of __get/__set.
I think goal (1) is reasonable for the long run, and goal (2) is taking it a bit too far for PHP. -- Stas Malyshev smalyshev@gmail.com
  115874
August 27, 2021 03:02 pierre.php@gmail.com (Pierre Joye)
Hi Stan,

On Fri, Aug 27, 2021 at 5:10 AM Stanislav Malyshev <smalyshev@gmail.com> wrote:
> > Hi! > > > The crux of the issue is what our end goal is: > > > > 1. Require users to explicitly annotate classes that use dynamic > > properties, but otherwise keep dynamic properties as a fully supported part > > of the core language. > > 2. Remove support for dynamic properties entirely. The core language only > > has declared properties and __get/__set, and nothing else. stdClass is just > > a very efficient implementation of __get/__set. > > I think goal (1) is reasonable for the long run, and goal (2) is taking > it a bit too far for PHP.
I feel the same way. However my gut feeling tells me that we will end up with (2) sooner or later. Having StdClass behaving differently (even if it is technically not, I don't think the "it is a more efficient get/set implementation" will make it through our users base) looks inconsistent. I don't know how others feel about it, I think most want to go the path to (2) and a more strict, if not almost fully strict PHP. Core devs to simplify the engine, users to allow them easier checks and safety in their frameworks or libs. End user don't really bother as far as I can see. We may have to decide once the long term vision for PHP and then do the changes accordingly, and more importantly, with consistency. Best, -- Pierre @pierrejoye | http://www.libgd.org
  115875
August 27, 2021 03:56 tobias.nyholm@gmail.com (Tobias Nyholm)
Just giving my 2 cents: 

> 2. Remove support for dynamic properties entirely.
I support this RFC and I like the end goal to be to remove the dynamic properties entirely. Dynamic properties are just confusing for beginners. “This is how you declare properties, the scope, the type, name etc.. but you don’t have too…” I also remember that I’ve fixed more than one bug because I misspelled a property name. I understand there will be an impact on legacy code but there is help to find dynamic properties (static analysers) and there is also a clear upgrade path. // Tobias
  115876
August 27, 2021 05:19 mike@newclarity.net (Mike Schinkel)
> On Aug 26, 2021, at 11:02 PM, Pierre Joye php@gmail.com> wrote: > I don't know how others feel about it, I think most want to go the > path to (2) and a more strict, if not almost fully strict PHP. Core > devs to simplify the engine, users to allow them easier checks and > safety in their frameworks or libs. End user don't really bother as > far as I can see. We may have to decide once the long term vision for > PHP and then do the changes accordingly, and more importantly, with > consistency.
I tend to concur that path (2) would provide the better outcome. However, eliminating dynamic variables would be eliminating the ability to simulate a(t least one) language feature that some (many?) people have been using dynamic variables to simulate. Maybe as part of deprecation of dynamic variables we should also consider adding this(ese) feature(s) to the PHP language? There are two approaches I can envision, Both would better enable the S.O.L.I.D. "open-closed" principle in PHP. 1.) Allow PHP class declaration across multiple files — A use-case for this is what I think is effectively the command dispatcher pattern where properties contain instances of classes that might be contributed by 3rd party code to allow those developers to access the instances by named property: e.g. $cms->map->display() where $map is added by a 3rd party library to the class for the $cms instance before $cms is instantiated. See: https://gist.github.com/mikeschinkel/ed4f9594c105dae7371bfce413399948 Some might view this as "monkey patching" but I view it as a more structured approach, like C# partial classes. 2.) Allow trait usage for a class from another file — This is just a slightly different spin on #1. See: https://gist.github.com/mikeschinkel/5b433532dc54adf53f5b239c8086fd63 Each approach has its own pros and cons but it probably depends on which would be easier and more performant to implement. -Mike
  115872
August 27, 2021 01:19 pollita@php.net (Sara Golemon)
On Thu, Aug 26, 2021 at 3:34 AM Nikita Popov ppv@gmail.com> wrote:
> > The crux of the issue is what our end goal is: > > 1. Require users to explicitly annotate classes that use dynamic properties,
> but otherwise keep dynamic properties as a fully supported part of the core language.
> That's how I initially read your RFC, what I'm hearing here is that your
quite explicit goal is:
> 2. Remove support for dynamic properties entirely. The core language only has
> declared properties and __get/__set, and nothing else. stdClass is just a very
> efficient implementation of __get/__set. > If that's your position, then the `extends stdClass` approach makes sense,
because the dynamic properties HashTable can live in the object's extra data offset block and the get_prop/set_prop implementations in stdClass' handlers are the *only* things which know about dynamic properties. That represents a significant advantage to the engine for the "happy path" of declared properties only. Every (non-dynamic) object is a word (8 bytes) leaner, and the branch points for dynamic property access are limited to the only place they can be used, and where the typical result is positive. Starting from that argument, your RFC begins to look more compelling. I was definitely not starting there on an initial read. I do still have reservations about enforcing stdClass as a root for any class which wants to use dynamic properties. It's messy, but... to paraphrase what you said about going with an attribute approach... yeah, someone could make every current root class in their application explicitly inherit from stdClass without any ill effects in PHP < 8.2, with only the exception of external libraries being a problem. So that's the spectrum: - Opt-in to strict behavior: No BC impact, least benefit to the ecosystem or the engine - Attribute/Interface opt-out (goal#1): Moderate BC impact, ecosystem benefits well, engine benefits a little, but not much. - Extend stdClass opt-out (goal#2): Highest BC impact, ecosystem and engine benefit most. Goal#2 is bold. Possibly not-too-bold, but it's not clear to me where dynamic props are being used today and how many of those places can't "just extend stdClass" for one reason or another. Goal#1 might be a stepping stone along the way to Goal#2, as it will give us something clear to scan for across major frameworks at the very least, but that is going to push us from removal in 9.0 all the way out to removal in 10.0. If you're keeping track, that's gonna be circa 2030 give or take a year or two. That might be dangling the carrot out a little too far... The purist in me wants to be bold. It also wants some hard data on existing usages. A simple grep isn't going to cut it this time. We're going to need to run some static analyzers on some frameworks and libraries. Who's got it in them to do the research? -Sara
  115873
August 27, 2021 02:02 matthewmatthew@gmail.com (Matthew Brown)
On Thu, 26 Aug 2021 at 21:20, Sara Golemon <pollita@php.net> wrote:

> We're > going to need to run some static analyzers on some frameworks and > libraries. Who's got it in them to do the research? > > -Sara >
I'm not volunteering, but I forked Nikita's package analysis to add Psalm scanning a while ago: https://github.com/muglug/popular-package-analysis It's not trivial (downloading the requisite files takes up a lot of HDD space) but Psalm's output can be parsed and analysed looking for UndefinedPropertyFetch and UndefinedThisPropertyFetch issues.