joomla-cms icon indicating copy to clipboard operation
joomla-cms copied to clipboard

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

Open Hackwar opened this issue 1 year ago • 4 comments

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:

  • [X] No documentation changes for docs.joomla.org needed

  • [ ] Pull Request link for manual.joomla.org:

  • [X] No documentation changes for manual.joomla.org needed

Hackwar avatar May 22 '24 09:05 Hackwar

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 avatar May 22 '24 09:05 Hackwar

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.

HLeithner avatar May 22 '24 14:05 HLeithner

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.

Hackwar avatar May 22 '24 14:05 Hackwar

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: https://github.com/joomla/joomla-cms/blob/05b7573db8a7001ba85c1cbdddc43500d68ad4dd/tests/Unit/Plugin/EditorsXtd/PageBreak/Extension/PageBreakTest.php#L47-L48

Fedik avatar May 22 '24 15:05 Fedik

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

Hackwar avatar Jul 21 '24 07:07 Hackwar