Skip to content

Conversation

@laoneo
Copy link
Member

@laoneo laoneo commented Jan 4, 2022

Summary of Changes

The getRouter function is still static in the application classes, but it is used in core on instances. Example is here https://github.com/joomla/joomla-cms/blob/4.1-dev/libraries/src/Router/Route.php#L133.

This pr removes the static call as applications can have different implementations and the router of the application should always be accessed on the instance and not statically.

It became static 13 years ago in 7175d2f. Time to revert it as we do not work anymore on static application access, more on the instance itself.

This can be considered a BC break but in my eyes, it is a wrong implementation.

cc: @wilsonge

Testing Instructions

Open the front end and back end.

Actual result BEFORE applying this Pull Request

All is working.

Expected result AFTER applying this Pull Request

All is working.

@PhilETaylor

This comment was marked as abuse.

@wilsonge
Copy link
Contributor

wilsonge commented Jan 4, 2022

I agree that it's wrong. But it's also a fairly painful b/c break to resolve :( I can't remember the details or discussions - but I remember having to just end up fixing all the current method calls way back #3548 - this was just due to a phpunit upgrade. but i'm sure there was a conversation about making it non-static again too somewhere. https://developer.joomla.org/joomlacode-archive/issue-33688.html?highlight=WzMzNjg4XQ==

I assume we'll have strict errors again after this in the places I changed above (if they still exist - it's possible with the new router lots of that stuff was cleaned up...) but even if we fix them we still have to account for 3rd party plugins doing similar things :( Thinking things like SH404 or other such extensions that overload bits of the router.

Overall completely agree it shouldn't be static. It just might need to be a 5.0 fix :(

@laoneo
Copy link
Member Author

laoneo commented Jan 4, 2022

So we have already strict errors right now. Should we fix it in 4 and then in 5 revert to none static?

@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/app/router/not-static branch January 14, 2022 19:48
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.

4 participants