-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[5.4] Add CachingFormFactory and CachingUserFactory with DI provider (BC-safe caching) #46428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 5.4-dev
Are you sure you want to change the base?
Conversation
|
@sshekhar563 Please check the code style errors reported in the CI checks (or see here https://github.com/joomla/joomla-cms/actions/runs/19208200451/job/54906507158?pr=46428 ). Thanks in advance. |
|
Thanks for the heads-up, @richard67 |
@sshekhar563 Code style seems ok now, but PHPstan reports issues. You can see them when checking your changes on GitHub here: https://github.com/joomla/joomla-cms/pull/46428/files and scroll down to the Besides this I see that you place constructor methods before class variables. That's not right, class variables should come first, then the constructor and then other methods. The code style checker might not complain about that, but if you check our other sources you will see it is like that. |
|
Please look at the other files in these folders and update these new files with the same information in the document headers |
|
@sshekhar563 Other thing: Could you rebase this PR to the 6.1-dev branch? We have discussed it in the maintainers Team. The referred issue #46369 is not an actual issue, it is a reminder that we will have an issue when we will remove the So it makes sense to introduce a replacement before that like your PR here does. But as that replacement would be a new feature, it cannot be done with a patch release (also due to semantic versioning), so not with 5.4 or 6.0. That's why it should be rebased to (or re-done for) the 6.1-dev branch. Let me know if you need help with that. Thanks for your understanding. |
| * @package Joomla.CMS | ||
| * @subpackage User | ||
| * | ||
| * @copyright (C) 2005 - 2024 Open Source Matters, Inc. <https://www.joomla.org> | ||
| * @license GNU General Public License version 2 or later; see LICENSE.txt | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct. Please do what I said and look at the other files in the same folder!
| * @package Joomla.CMS | ||
| * @subpackage Form | ||
| * | ||
| * @copyright (C) 2005 - 2024 Open Source Matters, Inc. <https://www.joomla.org> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still not correct - please do what I asked. Its is really not hard. Just look at the copyright headers in the same folder. Or maybe you arent actually doing anything other than asking AI to do it for you
| /** | ||
| * Joomla! Content Management System | ||
| * | ||
| * @copyright (C) 2018 Open Source Matters, Inc. <https://www.joomla.org> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a new file so obviously it is c 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the change.
Thanks for help and guidance
|
@sshekhar563 did you use AI code assistant? |
|
Thanks for the feedback. |
Pull Request for Issue #46369
This PR introduces caching decorator factories for Forms and Users to restore the performance benefits previously provided by getInstance() methods, without re-adding singletons or causing a BC break.
Summary
Adds CachingFormFactory and CachingUserFactory implementing the existing factory interfaces
Adds CachingFactoriesProvider to register caching wrappers via the DI container
Enables per-request in-memory identity maps for Forms and Users
No modifications to existing factories or interfaces (BC-safe)
Why
The removal of Form::getInstance() and User::getInstance() removed request-level caching, causing:
repeated DB queries for the same User object
repeated parsing/loading of Forms
performance regressions especially for large lists, form fields, and plugins executing multiple lookups
This PR restores the original caching behavior without the global static singleton pattern.
Fixes - #46369