? ? Maintainers Checked Pending

User tests: Successful: Unsuccessful:

avatar joomdonation
joomdonation
20 Jun 2022

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

avatar joomdonation joomdonation - open - 20 Jun 2022
avatar joomdonation joomdonation - change - 20 Jun 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 20 Jun 2022
Category Administration com_cpanel com_finder com_languages Front End com_contact com_content
avatar joomdonation joomdonation - change - 20 Jun 2022
Labels Added: ? Maintainers Checked
avatar laoneo
laoneo - comment - 20 Jun 2022

Restarted drone. Wondering if the system test installation is failing because of the pr or just a hickup.

avatar richard67
richard67 - comment - 20 Jun 2022

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.

avatar laoneo
laoneo - comment - 21 Jun 2022

The restart from yesterday and the branch update now were both successfully.

avatar richard67
richard67 - comment - 21 Jun 2022

The restart from yesterday and the branch update now were both successfully.

Maybe it depends on the moon phase 😄 .

avatar brianteeman
brianteeman - comment - 21 Jun 2022

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.

avatar RickR2H RickR2H - test_item - 21 Jun 2022 - Tested successfully
avatar RickR2H
RickR2H - comment - 21 Jun 2022

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.

avatar laoneo
laoneo - comment - 21 Jun 2022

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 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.

avatar viocassel viocassel - test_item - 21 Jun 2022 - Tested successfully
avatar viocassel
viocassel - comment - 21 Jun 2022

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.

avatar richard67 richard67 - change - 21 Jun 2022
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 21 Jun 2022

RTC


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

avatar Quy Quy - change - 22 Jun 2022
Labels Added: ?
avatar HLeithner HLeithner - change - 27 Jun 2022
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2022-06-27 15:49:48
Closed_By HLeithner
avatar HLeithner HLeithner - close - 27 Jun 2022
avatar HLeithner HLeithner - merge - 27 Jun 2022
avatar HLeithner
HLeithner - comment - 27 Jun 2022

Thanks

avatar trananhmanh89
trananhmanh89 - comment - 27 Jun 2022

Could anyone tell me why do we have to make this change? And Factory::getApplication() will be bad practice ?

avatar wilsonge
wilsonge - comment - 30 Jun 2022

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)

Add a Comment

Login with GitHub to post a comment