Skip to content

Conversation

@laoneo
Copy link
Member

@laoneo laoneo commented Jan 14, 2022

Pull Request for pr's #36562, #36629, #36631.

Summary of Changes

As discussed in #36631. It would be better to get the router from a service. Additionally the getRouter function in applications do become deprecated.

A second step would be to turn the language plugin into a service provider plugin and inject the router there. The same goes for the finder extension and the applications.

ping @nibra and @wilsonge

Testing Instructions

  • Create an article
  • Create a blog menu item
  • Enable the language filter plugin
  • Open the front end

Actual result BEFORE applying this Pull Request

All is working.

Expected result AFTER applying this Pull Request

All is working.

@laoneo laoneo force-pushed the j4/router/deprecate-app branch 4 times, most recently from d6f933a to 5545d3d Compare January 15, 2022 14:53
@laoneo laoneo force-pushed the j4/router/deprecate-app branch from 5545d3d to c83bff8 Compare January 15, 2022 15:26
@Fedik
Copy link
Member

Fedik commented Jan 16, 2022

I would not deprecate it, Web APP still should have it, like it have a Document.
Just add ServiceProvider, and if someone need a specific Router he will pick it from there (like in Route::link()).

Another thing, introducing SiteRouterInterface looks a bit not-intuitive to me, what purpose of it?
I would expect then AdministratorRouterInterface, ApiRouterInterface, CliRouterInterface etc

@joeforjoomla
Copy link
Contributor

joeforjoomla commented Jan 16, 2022

I would not deprecate it, Web APP still should have it, like it have a Document. Just add ServiceProvider, and if someone need a specific Router he will pick it from there (like in Route::link()).

I agree. Just like Document, Session, etc for consistency it should not be deprecated. Otherwise everything should be deprecated. App still should have everything as a shortcut to the container.

@laoneo laoneo changed the base branch from 4.0-dev to 4.2-dev January 25, 2022 06:58
@laoneo laoneo changed the title [4] Deprecate getting the router by the app and introduce a service [4.2] Deprecate getting the router by the app and introduce a service Jan 25, 2022
@laoneo laoneo removed the PR-4.0-dev label Jan 25, 2022
@laoneo laoneo marked this pull request as ready for review February 9, 2022 08:34
@BPBlueprint
Copy link

I have tested this item ✅ successfully on 51cfca4


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36688.

@dgrammatiko
Copy link
Contributor

I have tested this item ✅ successfully on f6908c0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36688.

@laoneo
Copy link
Member Author

laoneo commented Mar 23, 2022

rtc


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36688.

@laoneo laoneo added the RTC This Pull Request is Ready To Commit label Mar 23, 2022
@roland-d roland-d removed the RTC This Pull Request is Ready To Commit label Mar 24, 2022
@roland-d
Copy link
Contributor

@laoneo This requires another test after the new commit.

@BPBlueprint Could you redo your test again?

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 24, 2022
@laoneo
Copy link
Member Author

laoneo commented Mar 24, 2022

@roland-d all I did was adding a try/catch block around, nothing more f6908c0

@roland-d roland-d merged commit 31c9dd1 into joomla:4.2-dev Mar 24, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 24, 2022
@roland-d
Copy link
Contributor

Thanks everybody.

@roland-d roland-d added this to the Joomla 4.2.0 milestone Mar 24, 2022
@laoneo laoneo deleted the j4/router/deprecate-app branch March 24, 2022 18:58
@joeforjoomla
Copy link
Contributor

joeforjoomla commented Mar 25, 2022

Pinging @laoneo

Actually this PR is not fully b/c and reliable, given than:

Factory::getContainer()->get('SiteRouter'):

is not the same than:

$this->app::getRouter('Site');

I've noticed that they are 2 separate and different instances of the SiteRouter class.

So given for example that the Joomla core uses the container: Factory::getContainer()->get('SiteRouter'):
but a system plugin still uses $this->app::getRouter('Site'); to attach some build/parse rules:

$router->attachParseRule ( array (
		$this,
		'preProcessParseRule'
), \Joomla\CMS\Router\Router::PROCESS_BEFORE );

The result will be that the preProcessParseRule function of this plugin will never be called.

This implication has potential drawbacks, not sure how it can be solved other than update the code of all extensions to the new container to work on the same instance of the router class.

@laoneo
Copy link
Member Author

laoneo commented Mar 26, 2022

There is an alias missing for the old class names. Can you give #37381 test?

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.

8 participants