Skip to content

[5.0] Replaces the dependency for model state with a State object#39024

Closed
laoneo wants to merge 9 commits intojoomla:5.0-devfrom
Digital-Peak:cmsobject/state
Closed

[5.0] Replaces the dependency for model state with a State object#39024
laoneo wants to merge 9 commits intojoomla:5.0-devfrom
Digital-Peak:cmsobject/state

Conversation

@laoneo
Copy link
Copy Markdown
Member

@laoneo laoneo commented Oct 22, 2022

Summary of Changes

Replaces the state holder for model with an array based class. It removes the dependency for the deprecated CMSObject class.

Like that he performance can be improved by a third against using the 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:

@laoneo laoneo changed the title [5.0 Replaces the dependency for model state with a State object [5.0] Replaces the dependency for model state with a State object Oct 22, 2022
@Abernyte-Git
Copy link
Copy Markdown

I have tested this item ✅ successfully on a9aa13d


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39024.

@laoneo laoneo marked this pull request as draft October 27, 2022 09:10
@wilsonge
Copy link
Copy Markdown
Contributor

Is there a reason not to use Registry? Given it has similar methods as JObject for b/c purposes but also implements arrayAccess as an interface which could be situationally useful

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Oct 28, 2022

I tried to use registry but it doesn't support field access. So it would be a hard BC break. It is also massive slower than CMSObject or this new State class.

@laoneo laoneo marked this pull request as ready for review January 16, 2023 07:19
@laoneo laoneo requested a review from HLeithner January 16, 2023 07:19
@Fedik
Copy link
Copy Markdown
Member

Fedik commented Jan 18, 2023

Registry have some benefits also. In future you probably will be need serialisation, cloning.
Can try extend ModelState from Registry, and implement magick get/set.

Performance issue can overcome by disabling $separator, and make it always "flattened".
But that probably require changes in Registry class itself.
Much more work.
Hm, seems a simple State that you made here also fine :)

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Jan 18, 2023

But then I would rather extend the registry class with the magic methods and not introduce a new class at all.

@HLeithner
Copy link
Copy Markdown
Member

But then I would rather extend the registry class with the magic methods and not introduce a new class at all.

This would be against the argumentation in joomla-framework/registry#58

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Jan 18, 2023

I made an alternative which is using the registry in #39663.

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Feb 23, 2023

Closing in favor of #39663. When joomla-framework/registry#66 got merged, then we have the same speed without a new class which does more or less the same as the registry.

@laoneo laoneo closed this Feb 23, 2023
@laoneo laoneo deleted the cmsobject/state branch February 23, 2023 13:01
HLeithner added a commit to joomla/Manual that referenced this pull request Jun 26, 2023
* joomla/joomla-cms#39024

* Update new-features.md

* Update new-features.md

* Update new-features.md

* registry

---------

Co-authored-by: Harald Leithner <leithner@itronic.at>
HLeithner pushed a commit to joomla/Manual that referenced this pull request Jun 26, 2023
* Add deprecation for joomla/joomla-cms#39024

* Update new-deprecations.md

* Update new-deprecations.md

* Update new-deprecations.md

* Update new-deprecations.md

* Better text

* cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants