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

[4.1] Move getRouter to CMSApplicationInterface #36631

Closed
wants to merge 5 commits into from

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented Jan 10, 2022

Pull Request for discussion #36332 (comment).

Summary of Changes

Moves the getRouter function from the CMSWebApplicationInterface to CMSApplicationInterface as it is available in all apps now since #36332 and #36630.

Testing Instructions

Open front and back end and run a task in the console.

Actual result BEFORE applying this Pull Request

All works.

Expected result AFTER applying this Pull Request

All works.

@HLeithner
Copy link
Member

This a b/c break and have to be rebased to 5.0 as soon as the branch is available.

Another question is, is it really required in all CMSApplicationInterface ? I mean till now this was not a problem it seems.

@nibra
Copy link
Member

nibra commented Jan 10, 2022

As I understand it, the router is web specific, so it belongs in the CMSWebApplicationInterface. If you really think you need the router in a CLI applications, that CLI application should just implement the CMSWebApplicationInterface.

@laoneo
Copy link
Member Author

laoneo commented Jan 10, 2022

Creating an url is not web specific, only the url itself. With #36332 it is possible to generate site links in console applications. When you force a cli application to be a web application then you have to deal with menu stuff.

@nibra
Copy link
Member

nibra commented Jan 10, 2022

Creating an url is not web specific, only the url itself. With #36332 it is possible to generate site links in console applications. When you force a cli application to be a web application then you have to deal with menu stuff.

However, it still does not make sense to "pollute" all CMS applications with the router (and wait for 5.0 to solve the issue). Is there no possibility to a) instantiate the router in the application itself or better b) use the DI container for it?

@laoneo
Copy link
Member Author

laoneo commented Jan 10, 2022

I guess this is really a relict from the beginning. As for now the router is very much disconnected from the application, so it indeed makes sense to make the router available independent of the application. I would start with a router factory which can be injected to get a router instance from. What do you think? This can be even done in 4 and we can deprecate the whole getRouter stuff.

@nibra
Copy link
Member

nibra commented Jan 10, 2022

I would start with a router factory which can be injected to get a router instance from. What do you think? This can be even done in 4 and we can deprecate the whole getRouter stuff.

That actually sounds like a clean and sustainable solution to me.

@laoneo
Copy link
Member Author

laoneo commented Jan 10, 2022

@wilsonge what do you think, should we start deprecated getRouter in favor of a factory?

@wilsonge
Copy link
Contributor

I don’t really see what value a factory is giving you. If you wanna go that way just store it in the container directly and be done with it?

@laoneo
Copy link
Member Author

laoneo commented Jan 11, 2022

Perhaps factory was the wrong word, better would be a registry. But yeah could also be to inject the router directly instead of a registry(factory). I need to play with it.

@laoneo
Copy link
Member Author

laoneo commented Jan 14, 2022

Closing in favor of #36688.

@laoneo laoneo closed this Jan 14, 2022
@laoneo laoneo deleted the j4/router/app-interface branch January 14, 2022 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants