? ? Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
1 Jul 2022

Summary of Changes

Renames the translate function to something less generic to avoid potential conflicts with existing plugins. If there are better names feel free to comment, because for me is translate the right name.

Ping @roland-d

Testing Instructions

Create a GET request task and run it manually in the back end from the web interface.

Actual result BEFORE applying this Pull Request

All works.

Expected result AFTER applying this Pull Request

All works.

avatar laoneo laoneo - open - 1 Jul 2022
avatar laoneo laoneo - change - 1 Jul 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 1 Jul 2022
Category Libraries Front End Plugins Unit Tests
avatar wilsonge
wilsonge - comment - 1 Jul 2022

Please can we just keep to _. It's also similar to wordpress/kohana/cakephp (which use double underscore) which gives us some parity with other similar platforms

avatar wilsonge
wilsonge - comment - 1 Jul 2022

Also whilst we're reviewing this function is there a reason this method uses PHP's sprintf instead of the \Joomla\CMS\Language\Text::sprintf function

avatar brianteeman
brianteeman - comment - 1 Jul 2022

How about just doing it correctly. Revert the - untested change which was clearly wrong and then you can address the issue at leisure

avatar laoneo
laoneo - comment - 2 Jul 2022

Please can we just keep to _. It's also similar to wordpress/kohana/cakephp (which use double underscore) which gives us some parity with other similar platforms

I was also thinking about this, but we are using also ::_ in Route, so my thought was that in a plugin it would be a bit strange to use underscore for translation. But I'm open here really.

Also whilst we're reviewing this function is there a reason this method uses PHP's sprintf instead of the \Joomla\CMS\Language\Text::sprintf function

What we can also do is to move sprintf to the language. The target I want is to be able to avoid Text::_ for the sake of testing. When the language is directly used for translation, then we can properly unit testing it without mocking Factory::$application.

avatar HLeithner
HLeithner - comment - 2 Jul 2022

Can we please remove this function, I see 0 benefit. It only provides a subset of the functions from jlanguage. And pollutes the object.

avatar laoneo
laoneo - comment - 2 Jul 2022

As said in my comment before. It allows us to mock the application and then to write correct unit tests for plugins, which I don't think I have to explain here the benefits. Additionally it lowers the dependency to the Factory class. As I said, another solution would be to add the sprintf function to the language class, then the translate function can be removed.

When you have a look on the plugin unit tests for example here https://github.com/joomla/joomla-cms/blob/4.2-dev/tests/Unit/Plugin/Task/Requests/Extension/RequestsPluginTest.php#L99 or in #38072, then you will see the translate function allows us to write unit tests for plugins. Because a plugin should be testable as they should contain atomic functionality.

avatar HLeithner
HLeithner - comment - 2 Jul 2022

The please inject the language object if needed by the Plugin. I think the service provider should detect all interfaces implemented inject it.

avatar laoneo
laoneo - comment - 2 Jul 2022

There is no interface for the language and the language is available already through the application. Either way we keep this function or do copy the sprintf function from the Text class to the language object.

avatar HLeithner
HLeithner - comment - 2 Jul 2022

My understanding is that application should not be available to the plugin at some point in time

avatar laoneo
laoneo - comment - 2 Jul 2022

I think the plugin will always have a reference to the application as they act globally in the core. But they should not load it by itself, instead of, it should be injected. There we use for transition the app from the Factory class.

I made an alternative pr #38203 which reverts the translate function and goes directly to the application object.

avatar laoneo laoneo - change - 2 Jul 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-07-02 18:19:18
Closed_By laoneo
Labels Added: ? ?
avatar laoneo laoneo - close - 2 Jul 2022

Add a Comment

Login with GitHub to post a comment