Feature ? PR-5.0-dev Success

User tests: Successful: Unsuccessful:

avatar wilsonge
wilsonge
29 Mar 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

avatar wilsonge wilsonge - open - 29 Mar 2020
avatar wilsonge wilsonge - change - 29 Mar 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 29 Mar 2020
Category Libraries
avatar wilsonge
wilsonge - comment - 29 Mar 2020

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

avatar laoneo
laoneo - comment - 1 Apr 2020

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

avatar wilsonge wilsonge - change - 1 Apr 2020
The description was changed
avatar wilsonge wilsonge - edited - 1 Apr 2020
avatar wilsonge
wilsonge - comment - 1 Apr 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

avatar wilsonge wilsonge - change - 2 Apr 2020
Labels Added: ?
avatar wilsonge
wilsonge - comment - 10 Apr 2020

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

avatar wilsonge wilsonge - change - 4 Jan 2022
Labels Added: ?
Removed: ?
avatar wilsonge wilsonge - change - 4 Jan 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-01-04 01:53:55
Closed_By wilsonge
avatar wilsonge wilsonge - close - 4 Jan 2022
avatar wilsonge wilsonge - change - 4 Jan 2022
Status Closed New
Closed_Date 2022-01-04 01:53:55
Closed_By wilsonge
avatar wilsonge wilsonge - change - 4 Jan 2022
Status New Pending
avatar wilsonge wilsonge - reopen - 4 Jan 2022
avatar wilsonge
wilsonge - comment - 4 Jan 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 😴

avatar PhilETaylor
PhilETaylor - comment - 4 Jan 2022

added to my list, but first, bedtime :) Thanks.

avatar laoneo
laoneo - comment - 5 Jan 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.

avatar wilsonge wilsonge - change - 5 Jan 2022
The description was changed
avatar wilsonge wilsonge - edited - 5 Jan 2022
avatar wilsonge
wilsonge - comment - 5 Jan 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

avatar laoneo
laoneo - comment - 5 Jan 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.

avatar wilsonge
wilsonge - comment - 5 Jan 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).

avatar laoneo
laoneo - comment - 5 Jan 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.

avatar laoneo
laoneo - comment - 20 Apr 2022

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

avatar laoneo
laoneo - comment - 15 Jun 2022

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

avatar wilsonge wilsonge - change - 18 Jun 2022
Labels Added: ?
Removed: ?
avatar wilsonge
wilsonge - comment - 18 Jun 2022

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

avatar joomla-cms-bot joomla-cms-bot - change - 18 Jun 2022
Category Libraries Libraries Unit Tests
avatar wilsonge wilsonge - change - 19 Jun 2022
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 19 Jun 2022
Category Libraries Unit Tests Installation Libraries Unit Tests
avatar joomla-bot
joomla-bot - comment - 27 Jun 2022

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

avatar wilsonge wilsonge - change - 28 Jun 2022
Labels Added: ?
avatar Hackwar
Hackwar - comment - 21 Oct 2022

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

avatar wilsonge wilsonge - change - 13 Dec 2022
Labels Added: PR-4.3-dev
Removed: ? ?
avatar SharkyKZ
SharkyKZ - comment - 13 Dec 2022

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.

avatar wilsonge
wilsonge - comment - 13 Dec 2022

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.

avatar HLeithner
HLeithner - comment - 8 May 2023

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.

avatar wilsonge wilsonge - change - 2 Sep 2023
Labels Added: Feature PR-5.0-dev
Removed: PR-4.3-dev
4162b73 2 Sep 2023 avatar wilsonge CS
3e16249 2 Sep 2023 avatar wilsonge CS
avatar HLeithner HLeithner - close - 3 Sep 2023
avatar HLeithner HLeithner - merge - 3 Sep 2023
avatar HLeithner HLeithner - change - 3 Sep 2023
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2023-09-03 08:51:16
Closed_By HLeithner
avatar HLeithner
HLeithner - comment - 3 Sep 2023

Thanks, can you please write a manual migration documentation?

Add a Comment

Login with GitHub to post a comment