PR-5.2-dev Pending

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
22 May 2024

Summary of Changes

Factory::getLanguage() is deprecated and should not be used anymore. This PR replaces all occurences in our codebase with Factory::getApplication()->getLanguage(). I did this with simple search+replace, so this will need a carefull review (which I'm going to do, too, but more people have to take a look at this.)

Testing Instructions

Codereview.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

avatar Hackwar Hackwar - open - 22 May 2024
avatar Hackwar Hackwar - change - 22 May 2024
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 22 May 2024
Category Administration com_admin com_categories com_joomlaupdate com_login com_menus com_templates Modules Templates (admin) Repository Front End com_ajax com_banners com_config com_contact com_content com_finder com_users Installation
avatar Hackwar
Hackwar - comment - 22 May 2024

Ok, so, our tests fail with this change, since it requires an application to be started now and that fails. @laoneo do you have an idea for this? Would you have someone who could fix this?

avatar Hackwar Hackwar - change - 22 May 2024
Labels Added: PR-5.2-dev
avatar joomla-cms-bot joomla-cms-bot - change - 22 May 2024
Category Administration com_admin com_categories com_joomlaupdate com_login com_menus com_templates Modules Templates (admin) Repository Front End com_ajax com_banners com_config com_contact com_content com_finder com_users Installation Administration com_admin com_associations com_categories com_config com_contact com_content com_contenthistory com_fields com_finder com_installer com_joomlaupdate
avatar Hackwar Hackwar - change - 22 May 2024
Title
Replacing Fact::getLang() with Fact::getApp()->getLang()
[5.2] Replacing Fact::getLang() with Fact::getApp()->getLang()
avatar Hackwar Hackwar - edited - 22 May 2024
avatar HLeithner
HLeithner - comment - 22 May 2024

Doesn't like it for many cases, you swap just the language dependency into an application dependency which is worse. Trying to add languageawareinterface where possible should be done before falling back to the application object.

avatar Hackwar
Hackwar - comment - 22 May 2024

I understand and pretty much agree with you, however that is one of the two proposals in the deprecation notice how to replace this. I prefer this solution over the DI container since the DI container solution doesn't contain automatic type hinting. Basically I prefer the solution which PHPStorm automatically understands. But I'm open for any solution. I just want our core to not contain deprecated code usage anymore until 6.0.

avatar Fedik
Fedik - comment - 22 May 2024

Doesn't like it for many cases, you swap just the language dependency into an application dependency which is worse.

The Language can't live without Application (since 4.x?).
It is what it is.

@Hackwar Be aware that language available only after the application is initialised.
On quick check I do not see such use in PR,
however If there any instace of such call (before App initialisation), then the code will fail.

About failing test, it probably need something like this:

$app = $this->createStub(CMSWebApplicationInterface::class);
$app->method('getLanguage')->willReturn($this->createStub(Language::class));

avatar Hackwar
Hackwar - comment - 21 Jul 2024

We all agree that this is wrong, so I'm closing this PR again. :-)

avatar Hackwar Hackwar - close - 21 Jul 2024
avatar Hackwar Hackwar - change - 21 Jul 2024
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2024-07-21 07:16:05
Closed_By Hackwar

Add a Comment

Login with GitHub to post a comment