I don't get why you want to get rid of the static JText calls. What is wrong with them except that they may be a bit tricky to unit test?
This to me looks like just a change for the sake of change and it will generate a lot of code changes (replacing all JText calls) for no gain (or at least I don't see one yet).

What is wrong with them except that they may be a bit tricky to unit test?

Exactly that. Every part of our infrastructure that is static cannot be tested without a massive asston of black hat magic (if you're lucky, you can mock stuff in JFactory, but if the services are instantiating new classes or calling into any other of our singleton factories, you're now in deep rooted dependency hell).

So yes it may look like change for the sake of change (namespacing code is too, as well as changing UI frameworks), but it will have long term benefits if handled correctly (as in introduce it now and don't drop the static stuff in 4.0, wait on that).

Is this better than what we are doing in the framework v2 language package where me and michael already did this with full DIC container?

On phone so hard to review so it might be. Just raising it now :)

@Bakual it is comparable to the situation with JRequest, we need to get rid of that static stuff.

@mbabker that's exactly my intention.

@wilsonge had a look on the framework, it is also using static calls like Text::_().

If we agree on that approach, then I will polish it by injecting the logger and writing some unit tests.

Just did a grep. We have in staging 6932 instances of JText

What exactly would replaceJText::_(WHATEVER)

On a first step we will inject it into the MVC layer, that we can get rid of it there. On a long term it should be injected into form fields, layouts and JHTML.

You did not reply to my question

What exactly would replace JText::_(WHATEVER) ?

I thought you was asking about the procedure, because it's obvious when you have a look on the JText class from the pr https://github.com/Digital-Peak/joomla-cms/blob/0cc70080cbf61140b1fcf7fdf4164cc70718c45d/libraries/joomla/language/text.php.

JFactory::getApplication()->getTranslator()->_($string, $jsSafe, $interpretBackSlashes, $script); is replacing it.

I meant in a php file when we switch form JText to the new code.

Example:
return JText::_($val ? 'JON' : 'JOFF');

Would we have to use:
return JFactory::getApplication()->getTranslator()->_($val ? 'JON' : 'JOFF');

In most places yes, when we are finished with the MVC improvements, then you need to do in the view $this->translator->_();, it's similar to the transition from JRequest to JFactory::getApplication()->input. What do you think @infograf768?

had a look on the framework, it is also using static calls like Text::_().

Actually, JText::_() is deprecated in the framework package as well.

JFactory::getApplication()->getTranslator()->_($string, $jsSafe, $interpretBackSlashes, $script);

So you want to replace all ~7000 JText:: calls with that ugly JFactory::getApplication()->getTranslator()->?
Of course one should instantiate the class once per class and use from there. Eg

$this->translator = JFactory::getApplication()->getTranslator();
$this->translator->_(string)

But good luck changing that for all the 7000 calls. It will not be a simple search/replace.

I'm not a huge fan of replacing those static calls. Imho it's overengineering. JText is a rather simple class. I doubt we had many bugs related to it in the past. Most likely, unit tests are useless for that class anyway since any error would jump into your eyes right away (untranslated strings are hard to oversee when they happen all over the place).

If we refactor this to an object approach, why not directly use the JLanguage class? Why do we even need the translator? The real work is done by JLanguage anyway. I always saw JText as a convenient (read "short and simple") way to translate strings without having to worry about instantianting a class. Imho the whole point of JText is moot when you take away the static behavior from it.

I agree with @Bakual
This looks overkill to me, likely to cause errors because more complex. Simple is better.

It's not about replacing it at all in one step. We can do it during the whole lifecycle of Joomla 3/4/5 or whenever we feel comfortable.

Unit testing the Translator by itself is one thing. Where I see a bigger benefit is that we are one step closer to actually make the MVC layer unit testable without to set up a whole freaking infrastructure around every test case.

If we refactor this to an object approach, why not directly use the JLanguage class

Good question, I had a look on it. The problem is that the _function in JText is not the same as in JLanguage. It would mean that we introduce the JS text string management in JLanguage which is IMO not good. I think it would be not bad when we separate the ini file management and the translation functionality.

Actually, JText::_() is deprecated in the framework package as well.

The main reason for that actually has nothing to do with the code. Text::_() is a terrible function name, we opted for something more descriptive, hence Text::translate() was born.

If we refactor this to an object approach, why not directly use the JLanguage class?

JText in the CMS is also the interface for the JavaScript language store, handles various merge types (sprintf, printf), supports lookups for alternate keys, and handles key pluralization. JLanguage is basically primarily a metadata store but also holds the message catalog but doesn't actually do "real" translation work other than checking if a key is registered and returning its value. If proper separation of concerns were followed, there would actually be at least 3 classes in the language package; JLanguage itself which would be stripped down to basically the metadata handling and probably the localise class stuff, a JLanguageCatalog which is the actual message catalog and is where all the file loading and key lookup happens, and JText which is the translation interface. The latter two would be accessible through JLanguage (if you look in the Framework 2.0 branch there's actually a method in there to get a Text instance from the Language class, and since we aren't supporting the JavaScript store at that level anymore we actually don't have to have a singleton instance for that class).

Most likely, unit tests are useless for that class anyway since any error would jump into your eyes right away (untranslated strings are hard to oversee when they happen all over the place).

Wrong. The default rule when writing APIs should be to write code that can be tested AND replaced. JText is testable, but requires a real JLanguage instance to do it basically. JText cannot be replaced because it is static, so any code that is under test and calls it has to in essence cover testing JText as well. You can't properly unit test that code. With that tangent over, JText actually does have unique responsibilities and should be tested on its own, every test case in https://github.com/joomla-framework/language/blob/2.0-dev/tests/TextTest.php is covering that functionality.

Architecturally, this is the right move to make. Yes it will be a painful transition, and it will be a slow transition, but it is one that really needs to happen at some point.

@mbabker Ok, thanks for the deeper explanation. It's appreciated!

Yes it will be a painful transition, and it will be a slow transition, but it is one that really needs to happen at some point.

Can we then make sure we at least only have to touch those calls once during J4? Eg get it right from the beginning.
Also we should reconsider the framework package imho and if it can't be used for some reason (the removed JS stuf?) follow the conventions done there. This would make things smoother in future I think.

Text::_() is a terrible function name,

I certainly agree with that ? , if we change the class and have to touch each call, it would be a good move to use Text::translate() going forward ?

Also we should reconsider the framework package imho and if it can't be used for some reason (the removed JS stuf?) follow the conventions done there.

We should be able to have JLanguage extends \Joomla\Language\Language at least while keeping the CMS' specific features in the subclasses in 4.0. I don't know if we'd be able to have this Translator class extend the Framework Text class though, we took a different approach for named parameter support (explicit injection of a $parameters argument over the approach added in https://developer.joomla.org/joomlacode-archive/issue-31587.html results in an incompatible method signature). It's just all a matter of time and effort.

Most of the language package is still compatible with the CMS, we just dropped some app specific conventions (all the callbacks dealing with search, the JavaScript language store, and the JLanguage constructor loading stuff for both the site and admin apps at the same time).

Can we then make sure we at least only have to touch those calls once during J4

I would not do that. Because the plan is to inject the translator into the MVC stack. So you would do then in a view just $this->translator->_(); instead of calling the app. Another point is that we will backport this to 3.8/3.9 as well.

it would be a good move to use Text::translate() going forward

I was thinking about this, but I tought it would be a too big shock. Good we are on the same page here.

In this commit is illustrated how the Translator can be used to inject it into JHtmlNumber. Now this class is properly unit testable.

I would not do that. Because the plan is to inject the translator into the MVC stack. So you would do then in a view just $this->translator->_(); instead of calling the app. Another point is that we will backport this to 3.8/3.9 as well.

Then do that from the start (before J4 is released). so we only have to touch the JText calls once to adjust to the new way.

Another point is that we will backport this to 3.8/3.9 as well.

Honestly? There is no point backporting this into 3.x if you keep JText in J4 anyway. Especially if extensions can't use the translator (or Text) in the final new way (from the MVC). No sane extension developer will adjust those calls if we had to do it a year later again (and nothing breaks between).

Fixed the CS comments.

Thanks @laoneo

Then do that from the start (before J4 is released). so we only have to touch the JText calls once to adjust to the new way.

This will affect only the calls in the views, models, layouts, controllers and tables.

There is no point backporting this into 3.x if you keep JText in J4 anyway

We are backporting all the functionality into 3.8/3.9 that we as extension devs can then use the namespaced extension in J3 as well. So we need to backport it.

Where do we stay here? Any more feedback?

As JText has been namespaced do we still need this?

Where this PR is going should be an end state we reach eventually. Just as noted in the comment thread though, there's a lot to sort out to get to the ideal place.

OK I've spent some time having a think about this. What I'd like to do is backport the named parameter support from the framework language package into 3.9. Then use the framework version 2 language package in CMS 4 (deprecating the current named parameter support in 3.9). Adding framework 2 into CMS 4 will require us to add some extra legacy code to support the old Text::_ method - but I'm ok with that.

Seems a fair plan.