No Code Attached Yet
avatar nikosdion
nikosdion
8 Oct 2022

Steps to reproduce the issue

  • Install Joomla 4.2 on PHP 8.2 (currently in development)
  • Turn error reporting to Maximum

Expected result

No deprecated notices :trollface:

Actual result

(Among other deprecated notices)

Deprecated: Creation of dynamic property Joomla\CMS\Table\User::$id is deprecated in .../libraries/src/Table/Table.php on line 198

System information (as much as possible)

PHP 8.2 is the only relevant information

Additional comments

Context: PHP 9.0 will NOT allow dynamic property declaration anymore (fatal error). From PHP 8.2 and until PHP 9.0 doing that will throw a deprecated warning.

By default, \Joomla\CMS\Table\Table::__construct will try to create properties dynamically for any table fields which do not have a corresponding property in the table class.

Immediate actions required:

  • The \Joomla\CMS\Table\Table and \Joomla\CMS\Object\CMSObject classes need the #[\AllowDynamicProperties] attribute.
  • The Table's constructor needs to catch missing fields and log them as Joomla deprecated notices with a deprecation target of Joomla 6.0 or PHP 9.0, whichever comes first.
  • The CMSObject should be deprecated with a removal target of 6.0 and it should log a deprecated notice on PHP 8.2 or later informing the developer that come PHP 9.0 it will no longer work.

Further actions required:

  • Declare concrete properties for all table fields in all core table classes to make them forwards compatible with PHP 9. Target: Joomla 5.0
  • Communicate to the developers that they need to do the same in their own table classes. DO NOT rely on the deprecated notices alone!
  • CMSObject needs to move to __get / __set / __isset managing a private string-indexed array instead of dynamically declaring properties. Likewise, getProperties needs to provide the union of the concrete properties with the pseudo-dynamic properties in the array. MASSIVE CAVEAT: empty($object->dynamicProperty) will always return TRUE since empty() does not go through magic getter/setters. This will be a hard break for some code come Joomla 6.0.

Tagging @HLeithner @laoneo @nibra because there's code architecture involved and @roland-d @fancyFranci because it's something that needs fixing on 4.2 (PHP 8.2 will be released in less than 2 months).

avatar nikosdion nikosdion - open - 8 Oct 2022
avatar joomla-cms-bot joomla-cms-bot - change - 8 Oct 2022
Labels Added: No Code Attached Yet
avatar joomla-cms-bot joomla-cms-bot - labeled - 8 Oct 2022
avatar HLeithner
HLeithner - comment - 9 Oct 2022

I see it a bit unrealistic to to move our code base to non dynamic properties any time soon (years/decades), on the last sprint I tried to convert the update class to JRegistry because CMSObject is deprecated since ever and I failed, because even for this limited used class the cms uses getter/setter functions and direct access...

So for JTable I would suggest (as you do) to extend from stdClass (or the #[AllowDynamicProperties] attribute) which allows us to keep the dynamic behavior for now (and the future), of course that's not the best way but I think deprecating this for JTable is really hard for 3rd party (and core it self).

CMSObject has to be removed because it should be replaced with Registry or stdClass, this would solve the deprecation too. I think that Registry is full php 8.2 compatible. So till the usage of CMSObject is done I would suggest (as you do) to extend CMSObject from stdClass (or the #[AllowDynamicProperties] attribute) and move the replacement of CMSObject forward.

avatar nikosdion
nikosdion - comment - 9 Oct 2022

@HLeithner Sorry, but you just admitted that Joomla will cease to exist in 2–3 years, when PHP 9.0 comes out and you will just bury your head in the sand until then. This completely beats the entire point of the 2 year major release cycle and all the work all of us are putting into Joomla and its architecture. What the hell?!

Please do read the RFC I linked to and note that it's been voted 52 for – 25 against. PHP 9.0 will NOT allow dynamic properties. Period.

If you do not address dynamic properties until PHP 9 is released Joomla is dead. Period.

The solution is simple. A CMSObject subclass which uses pseudo-dynamic properties and using that for all core objects.

The PHP attribute is an interim solution to make PHP 8.2 shut up about dynamic properties until our code fails completely on PHP 9.0.

I refuse to let the Joomla project die for a silly, easily addressable issue like that. Yes, we can still have “dynamic” properties BUT THEY WILL NOT BE PHP LANGUAGE-LEVEL DYNAMIC PROPERTIES, THEY WILL BE SIMULATED WITH GETTERS AND SETTERS. No, using JRegistry is NOT a drop-in replacement, what I proposed is.

You know what, Harald? I will work on it. I refuse to let everything burn to the ground for 400 or so lines of PHP code I can develop over the course of a week and a planned b/c break for an unlikely to be used construct which y'all can decide in an afternoon.

avatar HLeithner
HLeithner - comment - 9 Oct 2022

@nikosdion please clam down and read the rfc carefully again. I did this several month ago and once again.

Please do read the RFC I linked to and note that it's been voted 52 for – 25 against. PHP 9.0 will NOT allow dynamic properties. Period.

wrong

here the most important parts:

The creation of dynamic properties on classes that aren't marked with the #[AllowDynamicProperties] attribute is deprecated in PHP 8.2 and becomes an Error exception in PHP 9.0. All used properties should be declared in the class declaration.

This says if the class is not marked with the attribute they throw an error with php 9.0, so no this attribute doesn't get removed in php 9.0.

second:

This RFC offers #[AllowDynamicProperties] as a way to opt-in to the use of dynamic properties. A previous version of this proposal instead suggested to extend from stdClass, and make stdClass the only class with first-class dynamic property support.
The difference between these approaches is in the end goal: #[AllowDynamicProperties] requires making classes that rely on dynamic properties explicit and prevents accidental use of dynamic properties. This is a big win for the ecosystem, but it does not have much effect on the overall complexity of the language or implementation, as dynamic properties still need to be supported on arbitrary classes. Requiring an extension of stdClass would allow us to actually remove the “dynamic properties” concept from the language in the future: stdClass would effectively just provide very optimized implementations of __get() and __set().
While completely removing dynamic properties is a worthwhile end goal, we also need to acknowledge that dynamic properties have played an important historical role in PHP, and legacy codebases in particular may be making heavy use of them. While adding an attribute provides a straightforward upgrade path, extending stdClass may not always be easily possible due to lack of multiple inheritance support. For this reason, this RFC pursues the more conservative attribute-based approach.
We may still wish to remove dynamic properties entirely at some later point. Having the #[AllowDynamicProperties] attribute will make it much easier to evaluate such a move, as it will be easier to analyze how much and in what way dynamic properties are used in the ecosystem.

This discussion part says they want to remove dynamic properties at some point but not in the near future.

So nothing is happening the next 5-8 years removing dynamic properties in php. Never the less we should get rid of dynamic properties of course.

I'm not sure what you want, would you like to get CMSObject back? So it's not longer deprecated?

avatar nikosdion
nikosdion - comment - 9 Oct 2022

The way I read the RFC I understand that PHP 8.x starting with 8.2 makes dynamic property creation deprecated and PHP 9 removes it.

More to the point, however, I do not think that removing CMSObject and replacing it with an internal CMSRegistry object is a good idea as it causes a supermassive b/c break, worse than any we've experienced in previous Joomla major versions.

For example, if the table object gets a $fields property which is a Registry object we have two ways to implement its API

  1. Give direct access to the fields property (e.g. with a getter, let's call it fields()). This means that code like this $table->id now becomes $table->fields()->get('id') which is far more verbose, a major b/c break and makes a lot of what we were previously doing in extensions impossible without major rewrites. It also doesn't work with concrete properties at all. It's a bad idea.
  2. We'd create magic getter / setters (__get, __set, __isset) to abstract the access to the Registry object. In this case, congratulations, you've reinvented CMSObject with the difference that you now have to write this code on every class which was formerly descending from CMSObject.

Further to that, using a Registry object for the flat, single level, array structure of dynamically allocated properties and their values adds a massive execution overhead. The Registry is not written for performance, it's written for flexibility. Having magic getters/setters operating on a simple array is far faster and does not lose any flexibility. If anything, it makes it simpler to serialise and unserialise objects which is important for tables and models (which already extend from CMSObject anyway!).

I will prepare a PR with unit tests for the 5.0-dev so you can see what I mean. It will use a different base class name, not CMSObject (I am going for CMSDynamicObject).

While I am at it, I will also deprecate the error handling and add a magic option to upgrade setError to throw RuntimeException instead of setting the ugly internal errors array. By default it will be disabled in 5.0 (since we want 100% b/c with 4.x if possible), we can enable it by default in 6.0 and remove setError completely in 7.0. Basically, have a forwards-compatible way to move from error arrays to exceptions.

avatar joeforjoomla
joeforjoomla - comment - 9 Oct 2022

I also understand that PHP 9.0 will also remove the temporary #[\AllowDynamicProperties]

As of Table class that now extends the class CMSObject, doing a quick test in my opinion the best option would make Table extends a new class e.g. CMSDynamicObject that extends \stdClass.
In this way there is not b/c break and full compatibility with PHP 9 making it possible to continue to use the same code without major changes.

avatar HLeithner
HLeithner - comment - 9 Oct 2022

The way I read the RFC I understand that PHP 8.x starting with 8.2 makes dynamic property creation deprecated and PHP 9 removes it.

I also understand that PHP 9.0 will also remove the temporary #[\AllowDynamicProperties]

Where is the sentence that says this please?

CMSObject is crap because it's more or less an object with a set function which looks odd. Also it has some useless error handling functions deprecated in 3.1.4. But of course JRegistry is much bigger but has everything you would expect from an "data object".

Anyway I'm also happy with a JRegistry light class (CMSDynamicObject).

MASSIVE CAVEAT: empty($object->dynamicProperty) will always return TRUE since empty() does not go through magic getter/setters. This will be a hard break for some code come Joomla 6.0.

this can be easily solved (if the php comment in the documentation is right) with

    public function __isset($key)
    {
        if (isset($this->_items[$key])) {
            return (false === empty($this->_items[$key]));
        } else {
            return null;
        }

https://www.php.net/manual/de/function.empty.php#93117

but extending stdClass for CMSObject seems to be faster (at some time) as mentioned in the RFC because the setter and getter could be optimized at compiler level. (Which maybe never happen, I don't know).

avatar nikosdion
nikosdion - comment - 9 Oct 2022

@HLeithner The sentence you mentioned before “The creation of dynamic properties on classes that aren't marked with the #[AllowDynamicProperties] attribute is deprecated in PHP 8.2 and becomes an Error exception in PHP 9.0. All used properties should be declared in the class declaration.” can be read two ways. Your way and my way. It's unclear if the attribute will allow us to use dynamic property declaration in PHP 9.0. I guess we'll find out.

Regarding empty(), yeah, I have already addressed it a bit more elegantly. I am writing the tests now.

avatar HLeithner
HLeithner - comment - 9 Oct 2022

Ok so since the rest I quoted says it doesn't removed anytime soon I see no reason to get in panic and get aggressive against me.

Happy to see what you came up with.

avatar laoneo
laoneo - comment - 17 Oct 2022

Classes marked with #[AllowDynamicProperties] as well as their children can continue using dynamic properties without deprecation or removal.

Statement from the RFC, so this would stay.

avatar nikosdion nikosdion - change - 22 Jan 2023
Status New Closed
Closed_Date 0000-00-00 00:00:00 2023-01-22 14:43:21
Closed_By nikosdion
avatar nikosdion nikosdion - close - 22 Jan 2023

Add a Comment

Login with GitHub to post a comment