@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 .

@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) ?

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

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

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 :)

@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.

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

(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?

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.

Closing in favour of #39020