Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.2] Replacing Fact::getLang() with Fact::getApp()->getLang() #43509

Open
wants to merge 2 commits into
base: 5.2-dev
Choose a base branch
from

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented May 22, 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

@Hackwar
Copy link
Member Author

Hackwar commented May 22, 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?

@Hackwar Hackwar changed the title Replacing Fact::getLang() with Fact::getApp()->getLang() [5.2] Replacing Fact::getLang() with Fact::getApp()->getLang() May 22, 2024
@HLeithner
Copy link
Member

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.

@Hackwar
Copy link
Member Author

Hackwar commented May 22, 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.

@Fedik
Copy link
Member

Fedik commented May 22, 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));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants