1. I left mutual dependency for now as I have to make sure about the goal. Therefore LanguageConverterFactory::createFromCode

has got weird signature:

public function createFromCode($code, \Language $language)

2. I use a naming convention to wire $code with Converter. Like in LanguageFactory.

3. To be sure that the Factory creates the same convertor with the same setting as newConverter(), I introduced LanguageCovertorFactoryTest which extensively verifies results for all supported languages.

4. I introduced static function LanguageConverter::createDefault() function and left newConverter() for backward compatibility. I suppose to use constructor instead of LanguageConverter::createDefault() and remove newConverter() in the future.

Below is a tentative roadmap:

Release 1

1. Mark newConverter() as deprecated.

2. Add fallback if language has no ::createDefault() implemented (legacy extensions etc)

3. Introduce ILanguageConverter and rename FakeConverter by DefaultLanguageConverter. Implement ILanguageConverter in both LanguageConverter and DefaultLanguageConverter.

Release 2

1. Remove $mLangObj property from LanguageConverter. It might be a little bit tricky as it executes Language::ucfirst.

3. Change signature of LanguageConverterFactory::createFromCode

4. Remove newConverter and replace ::createDefault() within a constructor.

In T226833#5803628, @Peter.ovchyn wrote:

1. I left mutual dependency for now as I have to make sure about the goal. Therefore LanguageConverterFactory::createFromCode

has got weird signature:

public function createFromCode($code, \Language $language)

Conceptually, it makes more sense to me for LanguageConverter to depend on language than the other way around. We can't get rid of the mutual dependency for now, but ideally, it would be confined to *deprecated* methods in Language using LanguageConverter.

I noticed that the factory method in your patch doesn't cache instances. The factory should do instance caching. the "create" method should be private, and there should be a public "get" method that would re-use any existing instance.

The signature I'd expect to see is getLanguageConverter( Language $language ).

2. I use a naming convention to wire $code with Converter. Like in LanguageFactory.

That's ok, but I think we should go for an explicit map and use ObjectFactory, like we do in SpecialPageFactory.

3. To be sure that the Factory creates the same convertor with the same setting as newConverter(), I introduced LanguageCovertorFactoryTest which extensively verifies results for all supported languages.

Excellent, thank you!

4. I introduced static function LanguageConverter::createDefault() function and left newConverter() for backward compatibility. I suppose to use constructor instead of LanguageConverter::createDefault() and remove newConverter() in the future.

newConverter doesn't need to be backwards compatible, since it's not public. We can just drop it. I think we can use the constructor right away (though via ObjectFactory). I see no need for createDefault().

By the way: the detour via ObjectFactory allows us to register converters in a declarative way, while still allowing DI to function.

Below is a tentative roadmap:

Release 1

1. Mark newConverter() as deprecated.

Since it's not public, it doesn't need deprecation, we can just drop it. This was implicite in the old deprecation policy, and is explicit in the new stable interface policy.

2. Add fallback if language has no ::createDefault() implemented (legacy extensions etc)

Why not just provide a default implementation in the base class?
But then, I don't think this methods is needed at all.

By the way - our deployment cycle is weekly, but the release cycle is twice a year. The next release would be some time in the summer.

3. Introduce ILanguageConverter and rename FakeConverter by DefaultLanguageConverter. Implement ILanguageConverter in both LanguageConverter and DefaultLanguageConverter.

The do-nothing FakeConverter could just be the base class.
We can also use an interface, but that interface would need to expose a *lot* of methods...

Release 2

1. Remove $mLangObj property from LanguageConverter. It might be a little bit tricky as it executes Language::ucfirst.

It would be nice to remove that dependency, but not entirely necessary. It's the dependency in the other direction that really needs to go (after deprecating the methods in Language that delegate to LanguageConverter).

If we want to remove the need for $mLangObj in LanguageConverter, we have to provide a way to inject implementations for ucfirst and also for getNsText. A narrow interface for each, implemented by Language, would work. That would remove the static dependency.

3. Change signature of LanguageConverterFactory::createFromCode
4. Remove newConverter and replace ::createDefault() within a constructor.

I think we can do both right away.

By the way - our deployment cycle is weekly, but the release cycle is twice a year. The next release would be some time in the summer.

Can we remove these lines then:

if ( !func_num_args() ) {
      // Old calling convention, deprecated
      if (static::class === 'Language') {
        $this->mCode = 'en';
      } else {
        $this->mCode = str_replace('_', '-', strtolower(substr(static::class, 8)));
      }

      $services = MediaWikiServices::getInstance();
      $this->mConverter = $services->getLanguageConverterFactory()->createFromCode($this->mCode, $this);
			$this->localisationCache = $services->getLocalisationCache();
			$this->langNameUtils = $services->getLanguageNameUtils();
			$this->langFallback = $services->getLanguageFallback();
			return;
		}

In T226833#5805009, @Peter.ovchyn wrote:

By the way - our deployment cycle is weekly, but the release cycle is twice a year. The next release would be some time in the summer.

Can we remove these lines then:

Not yet. The deprecation is for 1.35 (documented on the $code parameter), so we can only remove them when 1.35 has been released and we start working on 1.36. I think the branch is due to be cut in May, Cindy has the details.

->getLanguageConverter( $services->getContentLanguage() )

is quite often lines, maybe it worth to add getContentLanguageConverter() shorthand?

Injection of LanguageConverterFactory through constructor looks good according to OOP. How about introducing new arguments policy?

 	public function __construct(
 		$code = null,
 		LocalisationCache $localisationCache = null,
 		LanguageNameUtils $langNameUtils = null,
		LanguageFallback $langFallback = null,
		LanguageConverterFactory $converterFactory = null
 	) {

• Replace and remove Language::newConverter() - Done!
• Move the knowledge encoded in the various implementations of newConverter() into the respective LanguageConverter subclasses - Done!
• Remove $variants, $variantFallback, and similar parameters from the constructor signature of LanguageConverter, hard-coding the values insead. - Done!
• Create LanguageConverterFactory, managed by MediaWikiServices. - Done!
• Hard-code a mapping from language code to LanguageConverter subclass in * LanguageConverterFactory. - Working On Right Now!
• Make the LanguageConverter base class abstract. - Done!

In T226833#5822347, @Peter.ovchyn wrote:

• Replace and remove Language::newConverter() - Done!
• Move the knowledge encoded in the various implementations of newConverter() into the respective LanguageConverter subclasses - Done!
• Remove $variants, $variantFallback, and similar parameters from the constructor signature of LanguageConverter, hard-coding the values insead. - Done!
• Create LanguageConverterFactory, managed by MediaWikiServices. - Done!
• Hard-code a mapping from language code to LanguageConverter subclass in * LanguageConverterFactory. - Working On Right Now!
• Make the LanguageConverter base class abstract. - Done!

This all looks good in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/564789. There are only minor issues left to be resolved before it can be merged.

Change 567479 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/core@master] languages: Move Converter and tests to respective files

https://gerrit.wikimedia.org/r/567479

Change 567509 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/core@master] languages: Add @group Language to all tests related to Language for easier navigation through tests

https://gerrit.wikimedia.org/r/567509

Change 564789 merged by jenkins-bot:
[mediawiki/core@master] languages: Introduce LanguageConverterFactory

https://gerrit.wikimedia.org/r/564789

Change 567479 merged by jenkins-bot:
[mediawiki/core@master] languages: Move Converter and tests to respective files

https://gerrit.wikimedia.org/r/567479

Change 567509 merged by jenkins-bot:
[mediawiki/core@master] languages: Add @group Language to all tests related to Language for easier navigation through tests

https://gerrit.wikimedia.org/r/567509

Change 608977 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] language: Move LanguageConverterFactory and TrivialLanguageConverter to includes/

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /608977

Change 608977 merged by jenkins-bot:
[mediawiki/core@master] language: Move LanguageConverterFactory and TrivialLanguageConverter to includes/

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /608977