CMSDynamicObject to replace CMSObject

[nikosdion]

Pull Request for Issue #38921 .

Tagging @HLeithner

The CMSDynamicObject is a mostly drop-in replacement for CMSObject, with the following differences / b/c breaks to CMSObject:

• It uses concrete properties' visibility to determine which variables are public or not.
• It does not let you modify concrete private and protected properties.
• All dynamically created properties are read/write (public).
• The __toString magic method returns the (public) properties as a JSON-encoded object.
• By default, setError will simply throw a RuntimeException with the message you passed as a parameter instead of using an error message stack.

You CAN make CMSDynamicObject work in a mostly CMSObject-compatible manner by passing TRUE to the second argument of its constructor. If you do that, all of the above changes in behaviour are undone and your object works just like CMSObject, with the following caveats:

• You cannot assign values by reference. For example $object->item = & $item will result in a PHP error. Every instance of this pattern I found seems to be a leftover in view templates, with the value being assigned ($item) always an object — therefore nonsensical to pass by reference anyway. However, this is important enough that we cannot have AbstractView extend from CMSDynamicObject without a b/c strategy for view templates…
• You have magic methods. As a result, if you override any magic method you need to fall back to the parent implementation to avoid problems. This is necessary in BaseDatabaseModel, for example.
• The magic __get returns by reference. This is required for $object->foo['bar'] = 1 to work when your object does not have a concrete $foo array property. If you have overridden the magic __get method you need to adapt it. Again, this is necessary in BaseDatabaseModel.

The PR as-is only includes the CMSDynamicObject and its Unit Tests. It does not touch any other part of the CMS. I have run local tests replacing the Table's base class with CMSDynamicObject and I had no issues. Same for the model, as far as I can tell (we do not have complete units integration and functional tests, nor can I possibly manually test the entire CMS). The view has the problem with the view templates I mentioned, basically I had to remove the ampersands (&) from the view templates but if I were to redo it I'd much rather add concrete properties to the view classes as things should be to begin with!

Based on the above observations, it is possible to do a big push to remove dependency to the legacy CMSObject by 5.0 BUT it may introduce subtle b/c breaks mainly in the various View classes. Once everything is moved to CMSDynamicObject it will take a hard b/c break to get rid of the legacy error handling and add type hints to the CMSDynamicObject methods (I didn't do it because that would be causing a very hard b/c break on the View class if it extends from CMSDynamicObject). It will also take a minor b/c break to get rid of the private property access being enabled by default (we could make it into a Trait).

These are all things that must be taken into account, otherwise we can never get rid of CMSObject. Eventually, PHP will disallow dynamic property creation and we'll be up poop's creek, having to do a supermassive b/c break to implement CMSDynamicObject everywhere with barely any time for doing it right or testing it thoroughly (these RFC's tend to come roughly 6 months before the stable release of a new PHP version, giving us less than a year and a half before poop really hits the fan). Considering that the PHP project has been considering removing dynamic property assignment since 2016 we can only assume that it will happen, with the only unknown being exactly when will it happen.

[richard67]

@nikosdion PHPCS complains about the underscore in your property name: https://ci.joomla.org/joomla/joomla-cms/58421/1/5 . Either you change that name or you add your file to the exceptions in the ruleset.xml here https://github.com/joomla/joomla-cms/blob/4.2-dev/ruleset.xml#L32-L123 .

[nikosdion]

@richard67 I know it complains, and rightly so, but I cannot remove it because that would break Joomla's Table class — the only class in the CMS which tries to set the _errors array directly.

I am not getting in the trouble of making it shut up about the name (there is a way) as I want to discuss whether the overall philosophy is something the project is interested in before investing real time to polish it for production and start converting core classes to use it. This is basically my proof of concept PR, not something ready for merging. I didn't want to make it a draft because Harald might miss it in the pile of stuff he has to look into, like last time I did that (for the events) 😉

[richard67]

@nikosdion Then add that file to the exceptions in ruleset.xml like I wrote, so that PHPCS succeeds and the other tests are run.

[richard67]

@nikosdion You fixed the wrong file, it should have been tests/Unit/Libraries/Cms/Object/CMSDynamicObjectTest.php, and the property name is $_jsonEncode.

[Fedik]

I am sorry, I think it is "to much" :)
Lets just JTable extend stdClass. And later refactor it to use __get/__set (yea it has caveats but fixable).
And forget about CMSObject and Registry for this purpuse, do not merge unmergable :)

[nikosdion]

@Fedik What you proposed breaks b/c — you do, however, have a valid point.

If you just extend from stdClass you no longer have get, set, getProperties etc, nor do you have setError, getError and getErrors. This is a massive b/c break. The get, set and getProperties can't go away anytime soon; they are used in the core and all extensions as there's not other way to get an array of a table object's data (there's no other way to convert it to an array than calling getProperties).

You are, however, correct that extending from stdClass stops PHP 8.2 from complaining — just like using the PHP attribute. See https://3v4l.org/WCQRP#v8.2rc3 and https://3v4l.org/qmrW4#v8.2rc3 That was totally not what I was reading in that RFC. The wording seems to imply that only a plain stdClass object would not complain. Apparently extending from it and subclassing what extends it also works. Okay. That was… unexpected!

So, maybe a simpler solution is changing CMSObject from

class CMSObject

to

class CMSObject extends stdClass

As for CMSDynamicObject, it would make sense to refactor it to extend from stdClass and just have it as a forwards compatible way where you can disable legacy features (error stack, access to non-public properties, treating properties with leading underscores as private). This way it would be a drop-in replacement to CMSObject and we could start turning off features. I will refactor it to do that. It sounds like a much better solution.

[wilsonge]

You cannot assign values by reference. For example $object->item = & $item will result in a PHP error. Every instance of this pattern I found seems to be a leftover in view templates, with the value being assigned ($item) always an object — therefore nonsensical to pass by reference anyway. However, this is important enough that we cannot have AbstractView extend from CMSDynamicObject without a b/c strategy for view templates…

I thought (happy to be told I'm an idiot in this case) that actually we utilised pretty minimal amounts of CMSObject in the View classes in 4.0 - we've moved the core error handing to exceptions (fully accept 3PD's may not have done and that's something that would need work - but not necessarily library changes) and the majority of the get's were the proxies to the model from here https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/MVC/View/AbstractView.php#L131-L148

It does not touch any other part of the CMS. I have run local tests replacing the Table's base class with CMSDynamicObject and I had no issues

So strategically for table I assumed that we'd move to a more FOF style approach with the whole recordData array and proxy get's and sets through that - because effectively we rely on all the dynamic properties there mapping to data records. I'm not a massive fan of Table extending stdClass for something like this - although obviously it's better for some of the more data properties like here https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/Helper/ContentHelper.php#L168

@richard67 I know it complains, and rightly so, but I cannot remove it because that would break Joomla's Table class — the only class in the CMS which tries to set the _errors array directly.

https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/Table/Table.php#L582 If it's just this can we add a reset errors method to the class and be done with it - and try and pretend we were never that stupid to start with :D

[nikosdion]

(fully accept 3PD's may not have done and that's something that would need work - but not necessarily library changes)

That's basically what I am mostly worried about. I have not used CMSObject features in my software but I can't vouch for third party code, mostly because there is so little of it which is Joomla 4 native as opposed to Joomla 3 legacy MVC with minimal modifications to make it work in Joomla 4.

If we can avoid using CMSObject at all in views I am all for it. I do, however, feel that this is something which needs to be communicated now (and again every six months) with a target removal of 6.0.

So strategically for table I assumed that we'd move to a more FOF style approach with the whole recordData array and proxy get's and sets through that - because effectively we rely on all the dynamic properties there mapping to data records.

There are pros and cons to using the recordData array. The biggest problem is that you cannot have table fields which are decoded to arrays if you want to modify them through magic __get. This fails:

$myTable->foo[] = 1

If foo can only be accessed over __get unless you defined the magic getter as &__get (return by reference) which raises a lot of questions as to whether it's a sustainable architecture :/ See a couple of commits ago in my PR, you'll see me doing that.

This was a major PITA with DataModel in FOF. You had to use getFieldValue and setFieldValue with fields of this kind.

I agree that the Table should ideally have database data to record data and vice-versa converters so we don't have to do any kind of conversion that's NOT JSON data (already supported in the Table) with custom code in load and store.

I'm not a massive fan of Table extending stdClass for something like this

We will always need to add arbitrary stuff to the table if we want to have features like Fields which adds a nice jcfields property to items. But, wait, you say, this only happens in Models which return dumb stdObject objects! Yeah, sure, but I have modified my Models which return a 1:1 representation of raw table data to map these dumb objects to the respective Table objects so I can implement relations (which are methods in my Table objects). So don't discount the need for arbitrary, dynamic properties in table objects.

If it's just this can we add a reset errors method to the class and be done with it - and try and pretend we were never that stupid to start with :D

Obviously yes, but also notify developers and give them an entire major version to change their code. And by “notify developers” I do not mean writing a docblock and forgetting about it. I am talking about an organised outreach campaign.

At the very least, developers who have JED extensions should be asked to opt-into a developers newsletter detailing b/c breaks being introduced. Open that newsletter to anyone who wants to subscribe to it. Every time something is being merged which can cause a b/c break or requires developers to do something an article needs to be written and a newsletter sent. PUSH the information instead of expecting it to be PULLED. I am very active in this repo and I still miss things because I didn't monitor a badly named issue or PR which introduces a b/c break, you know?

[wilsonge]

That's basically what I am mostly worried about. I have not used CMSObject features in my software but I can't vouch for third party code, mostly because there is so little of it which is Joomla 4 native as opposed to Joomla 3 legacy MVC with minimal modifications to make it work in Joomla 4.
If we can avoid using CMSObject at all in views I am all for it. I do, however, feel that this is something which needs to be communicated now (and again every six months) with a target removal of 6.0.

I believe in my experience that this would be fairly safe? I think there are a small number of people who use setError in a single place (when getting an item or list of items returns false because of an error and we bubbled back up the model exception) - I would imagine for 2 major versions time this would be a message we could fairly easily push back out.

EDIT: Did a git history check and actually we used to call JError directly which we removed (ea71f82) so actually this is even safer than I thought

If foo can only be accessed over __get unless you defined the magic getter as &__get (return by reference) which raises a lot of questions as to whether it's a sustainable architecture :/ See a couple of commits ago in my PR, you'll see me doing that.
This was a major PITA with DataModel in FOF. You had to use getFieldValue and setFieldValue with fields of this kind.
I agree that the Table should ideally have database data to record data and vice-versa converters so we don't have to do any kind of conversion that's NOT JSON data (already supported in the Table) with custom code in load and store.

OK that's interesting. I guess my issue with the current system is we kinda assume that everything is a database field and send all public fields to the database even some that we know are not database columns and just rely on the fact that db's won't match them - jcfields being a really good example of that so I agree we need dynamic setters - but we also need a way to separate the two was more my point - and class properties aren't really good enough as a result. We need different blocks for 'metadata' and 'database fields' and I saw that sort of thing as the best method to distinguish the two.

At the very least, developers who have JED extensions should be asked to opt-into a developers newsletter detailing b/c breaks being introduced. Open that newsletter to anyone who wants to subscribe to it. Every time something is being merged which can cause a b/c break or requires developers to do something an article needs to be written and a newsletter sent. PUSH the information instead of expecting it to be PULLED. I am very active in this repo and I still miss things because I didn't monitor a badly named issue or PR which introduces a b/c break, you know?

I'm down with this. But I'm not a member of the JED team (or any leadership team anymore) so easy for me to say :D Either way fixing what we have in core by adding the reset method seems like a good start even beyond getting the message out to the wider 3PD ecosystem and actively demonstrating what we are preaching.

[nikosdion]

Closing in favour of #39020