[5.0] Transition from CMSObject to Registry for model state

[laoneo]

Summary of Changes

Replaces the state holder for model with a transition class which extends the Registry. Like that we benefit from the features of the Registry which is used across the whole core. It removes the dependency for the deprecated CMSObject class.

Testing Instructions

• Create an article in the back end with the state published
• Filter for trashed articles

Actual result BEFORE applying this Pull Request

No article is shown.

Expected result AFTER applying this Pull Request

No article is shown.

Link to documentations

Please select:

• Documentation link for docs.joomla.org:

• No documentation changes for docs.joomla.org needed

• Pull Request link for manual.joomla.org: Document the new State class Manual#78 and Add deprecation for State object in favor of Registry Manual#77

• No documentation changes for manual.joomla.org needed

[laoneo]

@HLeithner what do we do here?

[HLeithner]

@laoneo tests are failing I restarted the tests and I think we will go with this solution. I added it to my personal todo list and hope to merge it this week after giving some thought again.

[laoneo]

I guess the tests are failing because there is actually an issue there:

https://ci.joomla.org/artifacts/joomla/joomla-cms/5.0-dev/39663/system-tests/64369/screenshots/site/

[HLeithner]

Yes I thought the same I just wanted to be sure ;-) can you have a look?

[laoneo]

It is actually a break when we switch to the registry. Because the registry returns the default value when the value in the internal data store is an empty string:

https://github.com/joomla-framework/registry/blob/2.0-dev/src/Registry.php#L207

While the CMSObject checks only if the property exists:

https://github.com/joomla/joomla-cms/blob/5.0-dev/libraries/src/Object/LegacyPropertyManagementTrait.php#L64

[HLeithner]

Ok can we make this b/c and trigger a deprecation warning before we are switching to registry?

[laoneo]

Not sure if we really want to do 4e7ca12 😏

[HLeithner]

https://github.com/joomla-framework/registry/blob/2.0-dev/src/Registry.php#L207 looks completely wrong...

Why in the word do we return the default value if value is an empty string? or NULL

we have a remove() function, shouldn't we only check for isset() ?
https://github.com/joomla-framework/registry/blob/2.0-dev/src/Registry.php#L607

[laoneo]

I guess the issue is that this applies only when the separator is empty, otherwise it uses isset further down. If I read the code correctly.

[HLeithner]

Hmm, your work around works different because 0 empty too... can you point me to the code that had failed in the tests and why?

[laoneo]

Here is direction set to an empty string https://github.com/joomla/joomla-cms/blob/5.0-dev/components/com_content/src/Model/FeaturedModel.php#L108 and here it gets the default value https://github.com/joomla/joomla-cms/blob/5.0-dev/components/com_content/src/Model/ArticlesModel.php#L634

The home site will imediately crash when you remove the fix, so you can debug it quickly as it will make then an ordering like a.ordering DESC ASC in the query.

[HLeithner]

ok then I think we need to keep the workaround and add documentation (and fix the cms) to use $state->remove() or is there anything that speaks against this?

[Fedik]

Why in the word do we return the default value if value is an empty string? or NULL

As I understood, this is made for Form Fields default values, because Registry heavily used in Params, and form validation, filtering etc.
Kind of.

[HLeithner]

As I understood, this is made for Form Fields default values, because Registry heavily used in Params, and form validation, filtering etc. Kind of.

ok sounds wrong anyway

[laoneo]

Where do you want to document what?

[HLeithner]

Where do you want to document what?

good question, one point would be the deprecation b/c part for joomla upgrades. On the other side we should mention this in the documentation of the model in the wiki? and the manual (doesn't exist yet?)

[HLeithner]

I merge this for now so we may get hopefully real world feedback in alpha 1.