Skip to content
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

Use CMSWebApplicationInterface in controllers #28508

Merged

Conversation

wilsonge
Copy link
Contributor

@wilsonge wilsonge commented Mar 29, 2020

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\Factory to 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

@wilsonge
Copy link
Contributor Author

Removing beta blocker actually. Loosening the constraints can be done in any minor version

@laoneo
Copy link
Member

laoneo commented Apr 1, 2020

Is it not a BC break when the signature of the constructor changes?

@wilsonge
Copy link
Contributor Author

wilsonge commented Apr 1, 2020

No. As long as it's compatible with the old typehint (which it is - it's a higher level) it's totally b/c

@wilsonge
Copy link
Contributor Author

@mbabker thanks for the review. Do you have any opinion on the Logging part?

@wilsonge wilsonge force-pushed the feature/application-interface-controllers branch from ae52752 to 5018868 Compare January 4, 2022 01:35
@wilsonge wilsonge marked this pull request as ready for review January 4, 2022 01:35
@wilsonge wilsonge force-pushed the feature/application-interface-controllers branch 2 times, most recently from c35ef9b to d1eb72b Compare January 4, 2022 01:41
@wilsonge wilsonge closed this Jan 4, 2022
@wilsonge wilsonge reopened this Jan 4, 2022
@wilsonge
Copy link
Contributor Author

wilsonge commented Jan 4, 2022

@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 😴

@PhilETaylor

This comment was marked as abuse.

@wilsonge wilsonge force-pushed the feature/application-interface-controllers branch from d1eb72b to 6d1889e Compare January 4, 2022 19:16
@laoneo
Copy link
Member

laoneo commented Jan 5, 2022

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.

@wilsonge
Copy link
Contributor Author

wilsonge commented Jan 5, 2022

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

@laoneo
Copy link
Member

laoneo commented Jan 5, 2022

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.

@wilsonge
Copy link
Contributor Author

wilsonge commented Jan 5, 2022

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).

@laoneo
Copy link
Member

laoneo commented Jan 5, 2022

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.

@bembelimen bembelimen changed the base branch from 4.0-dev to 4.1-dev January 25, 2022 15:46
@laoneo laoneo self-assigned this Apr 20, 2022
@laoneo
Copy link
Member

laoneo commented Apr 20, 2022

Can you fix the conflicts here and make it ready for review? Would like to bring this one forward.

@laoneo laoneo changed the base branch from 4.1-dev to 4.2-dev June 15, 2022 14:14
@joomla-bot
Copy link
Contributor

This pull requests has been automatically converted to the PSR-12 coding standard.

@Hackwar Hackwar changed the base branch from 4.2-dev to 4.3-dev October 21, 2022 08:32
@Hackwar
Copy link
Member

Hackwar commented Oct 21, 2022

I've changed the basebranch to 4.3-dev, since we wont be merging this into 4.2.

@SharkyKZ
Copy link
Contributor

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.

@wilsonge
Copy link
Contributor Author

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.

@Hackwar Hackwar added the Feature label Apr 6, 2023
@HLeithner HLeithner changed the base branch from 4.3-dev to 5.0-dev May 8, 2023 15:03
@HLeithner
Copy link
Member

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.

@HLeithner HLeithner merged commit 67a4e57 into joomla:5.0-dev Sep 3, 2023
3 checks passed
@HLeithner
Copy link
Member

Thanks, can you please write a manual migration documentation?

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.

None yet