[#36562] - [4.1] Gettting the router should not be static
- Closed
- 14 Jan 2022
- Medium
- Build: 4.1-dev
- # 36562
- Diff
- Digital-Peak:j4/app/router/not-static
User tests: Successful: Unsuccessful:
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.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
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 :(
So we have already strict errors right now. Should we fix it in 4 and then in 5 revert to none static?
| Labels |
Added:
?
|
||
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-01-14 19:48:33 |
| Closed_By | ⇒ | laoneo |
Probably not possible in 4.1.x then, needs to be done in 5...
We dont yet have a branch for 5.0-dev :)