[4.2] Replace Factory::getApplication() by $this->app in component's controllers#38103
Conversation
administrator/components/com_scheduler/src/Controller/TasksController.php
Show resolved
Hide resolved
administrator/components/com_cpanel/src/Controller/DisplayController.php
Show resolved
Hide resolved
administrator/components/com_finder/src/Controller/IndexerController.php
Show resolved
Hide resolved
|
Restarted drone. Wondering if the system test installation is failing because of the pr or just a hickup. |
Wondering about the same. But I haven't found anything in the log or the screenshot from the test results telling what it could be. |
|
The restart from yesterday and the branch update now were both successfully. |
Maybe it depends on the moon phase 😄 . |
|
I really appreciate all the work you are and @laoneo are doing but I really think that it is far too late in the release process of 4.2 to be making these changes. |
|
I have tested this item ✅ successfully on 83e9eae This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38103. |
I think this is up to the release lead to decide. And nobody says that this needs to go into 4.2. But thats the branch where this kind of changes should be done to atm. |
|
I have tested this item ✅ successfully on 83e9eae This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38103. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38103. |
|
Thanks |
|
Could anyone tell me why do we have to make this change? And |
|
So inside the controller we want to take better advantage of dependency injection. This allows us to create significantly better unit tests over the controllers and therefore improve both the quantity and quality of tests by creating mocks. Additionally injecting like this allows us to take better advantage of the Dependency injection container. The static factory settings conversely are more difficult to mock across multiple tests, mean its harder to utilise the DIC. Furthermore by making this dynamic it improves our ability to use the same controller in different applications (as API starts to become more of a first class citizen this will become more and more clear I think) |
Pull Request for Issue # .
Summary of Changes
In none static methods of component controllers, we should use $this->app to get application object instead of using Factory::getApplication(). This PR just makes that change.
Testing Instructions
I hope code review would be enough. Otherwise, I will have to split this simple PR to multiple small PRs for real user testing.
Actual result BEFORE applying this Pull Request
Works !
Expected result AFTER applying this Pull Request
Works !
Documentation Changes Required
None