joomla-cms
joomla-cms copied to clipboard
joomla
Use CMSWebApplicationInterface in controllers
Summary of Changes
Moves Joomla MVC Controllers over to use CMSWebApplicationInterface instead of CMSApplication. This may be of use in the future to console applications who wish to use the Controller system.
- Changes type hints in constructors to
CMSWebApplicationInterface - Changes some instances in controllers where we currently use
\Joomla\CMS\Factoryto use the app property (especially FormController)
~Note this is a draft PR because I need to figure out how to resolve the calls to getLogger in BaseController - as the FIG LoggerInterface is not contained in the CMSApplicationInterface as things stand.~
Testing Instructions
No changes expected in the web interface. Code review of changes.
Documentation Changes Required
None
Removing beta blocker actually. Loosening the constraints can be done in any minor version
No. As long as it's compatible with the old typehint (which it is - it's a higher level) it's totally b/c
@mbabker thanks for the review. Do you have any opinion on the Logging part?
@PhilETaylor can you cast a more experienced pair of eyes over this? I’ve done some more refactoring and still not totally sure about the approach here.
obviously need to change it to target 4.1 too but will sort that another day. Time for some 😴
This may be of use in the future to console applications who wish to use the Controller system.
I do not see the point that the Controllers and the whole MVC behind it can then be used, as a Console application can never implement getMenu.
I do not see the point that the Controllers and the whole MVC behind it can then be used, as a Console application can never implement getMenu.
I'm not sure I really see your point here? My entire driver here is that we are going to have scenarios if console if a first class application where you might have controller tasks which are specifically for a console and shouldn't be exposed to a web application ever and obviously we need to take into account even on shared tasks there might not be an active menu item in those scenarios
Ok, if you write controllers specifically for console app, then will work. What I did understand was, that all controllers (site/admin) will then work in console apps.
So the controller has to be aware of the request format (because, for example, the input object is going to be different and likely will have different naming conventions) - your output format is also likely to be different (obviously you rarely want to redirect which is web application specific or display a html view inside the console). Obviously we need to minimise differences as much as practically possible - but there's always going to be some differences. The end goal should be where the state is injected into the Model so that the model eventually becomes largely unaware of application (it's a grand goal that's going to take a long time to reach).
Totally agree
The end goal should be where the state is injected into the Model so that the model eventually becomes largely unaware of application (it's a grand goal that's going to take a long time to reach).
Something we should consider version 5.
Can you fix the conflicts here and make it ready for review? Would like to bring this one forward.
Can you fix the conflicts here and make it ready for review? Would like to bring this one forward.
Done but needs a full test rework now to go with it because the mock application objects don’t return the logger even though the real one always will. Will work on it over next few days
This pull requests has been automatically converted to the PSR-12 coding standard.
I've changed the basebranch to 4.3-dev, since we wont be merging this into 4.2.
Wouldn't it possible to introduce a more abstract class that accepts any application and leave BaseController as web-only? Or does something prevent that at this point (like legacy loader thing)? Otherwise, document the new exceptions.
Wouldn't it possible to introduce a more abstract class that accepts any application and leave BaseController as web-only? Or does something prevent that at this point (like legacy loader thing)? Otherwise, document the new exceptions.
My aim was to do it a bit differently - most controllers only have a display controller so I wanted to move display into it's own subcontroller of base controller in a future major version (use an empty class in the current version for b/c)
Then that leaves the hold/release/check edit ids - they are common to all the child classes - so I was thinking of maybe putting them in a trait???? (less sure but they are all group'd together)
Finally there's redirect and checkToken which are basically both proxy methods. I'm not that fussed about just the exceptions for them given they are so small?
I mean with all the benefits of hindsight I should have made a BaseWebController or something. But it's still not that clear if it's a good idea for like 2-3 methods.
I'm pretty sure I want display controller to be it's own thing. Less sure about how to rework the others tho - happy to take ideas.
This pull request has been automatically rebased to 5.0-dev. No new features will be merged into Joomla! 4.3 series. Joomla! 4.4 series is a bridge release to make migration from Joomla! 4 to 5 as smooth as possible.
