User tests: Successful: Unsuccessful:
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
| Status | New | ⇒ | Pending |
| Category | ⇒ | Libraries |
| Title |
|
||||||
| Labels |
Added:
PR-5.4-dev
|
||
Thanks for the heads-up, @richard67
I’ve fixed all reported code style issues and pushed the updated changes.
The php-cs-fixer and phpcs checks should pass now.
Thanks for the heads-up, @richard67 I’ve fixed all reported code style issues and pushed the updated changes. The php-cs-fixer and phpcs checks should pass now.
@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 libraries/src/User/CachingUserFactory.phpfile. Please check if you can fix that.
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 getInstance() methods in a future release, which can be only a major version due to semantic versioning because it would be a b/c break, so it can happen earliest with 7.0, maybe more likely 8.0.
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.
| Labels |
Added:
Feature
|
||
@sshekhar563 did you use AI code assistant?
Please review the code to fit it in to Joomla code style. There still many issues with the generated code.
| Category | Libraries | ⇒ | Repository Libraries |
Thanks for the feedback.
I’ve manually reviewed all three files and updated them to follow Joomla’s core coding style exactly, using the same headers, method order, and formatting as other files in libraries/src/User, Form, and Service.
I also re-ran php-cs-fixer and phpcs, and both now pass cleanly.
Please let me know if you’d like any further adjustments.
@sshekhar563 Please advise why do you use final classes and private properties?
Nobody will be able to easily extend this code on demand :(
Joomla! is open-source "extendable" software.
| Title |
|
||||||
| Labels |
Added:
PR-6.1-dev
Removed: PR-5.4-dev |
||
@sshekhar563 As I had mentioned in my previous comment, this PR should be made for the 6.1-dev branch. As there was no reply to that, I have now allowed myself to rebase to 6.1-dev.
Thank you @richard67
I appreciate you taking the time to rebase the PR to the 6.1-dev branch.
I now understand that new feature-related work like this should target the 6.1-dev branch instead of 5.4-dev.
Thanks again for your help and guidance throughout the review I’ll make sure to follow this process correctly in future contributions.
I'm not so happy with this approach, we try to avoid super globals when ever possible.
My suggestion is to create a "subform" registry in the Form Object and load the subform subforms^^ into this registry
My understanding is that this is mainly a subformfield issue ymmv.
This apporach has also the benefit that not every module, template override, library can change randomly forms. it means you can load the same subform twice for 2 different forms and manipulate it if needed. for example for comparison where you might load the same form twice.
Thanks for the suggestion @HLeithner that makes sense and I appreciate the direction.
My current PR introduced per-request in-memory caching via decorator factories to restore the request-scoped behavior that getInstance() used to provide, and to do so in a BC-safe way (no singletons / global statics). I agree that adding a focused subform registry on the Form object is a cleaner and less global approach for subform-specific reuse.
I see two ways forward and am happy to follow whichever the team prefers:
1 - Adapt this PR so the Form uses a subform registry internally and the caching factory consults that registry (we keep the caching factory for users and other caches).
2 - Keep this PR as-is to restore request-scoped caching, and follow up with a separate PR to add the Form::subformRegistry() approach and then refactor how subforms are loaded to use it.
If you prefer option 1 I can update this PR to show the registry implementation. If you prefer option 2 I’ll open a follow-up PR that moves subform handling into the Form object and adapts subform loading to use the registry. Which would you prefer? Thanks!
thanks for the feedback, I don't see the CachingFactory at all, since the use case for at least the subform (or any request to a form from outside) should be full fillable with the access to the subform registry in the form.
Beside that it would be interessting if there are more usecases / beside the subform where fields need access to something global which better should be handled in the form object.
@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.