Move CurrentLanguageContext to Core\Language\ContextProvider

+1 to the change.

The bug here is inaccessibility of current language context without language module because "current language context" lives not in core.

This makes it at least major bug. I let @Gábor Hojtsy RTBC it cuz we might need tests for this.

Drupal is lingual without language.module but hardly multi unless someone writes an alternative implementation of language.module.

This is fine as long as we don't change the service ID because that's part of the data structure now.

It is technically true that Drupal is multilingual by default because the default languages Not specified and Not applicable are present even without language module (additionally to the default language). See LanguageManager::getDefaultLockedLanguages().

I think the move is fine. Do we have existing tests to ensure this works?

@Gabor: Fine, you win :) But those languages are never the current language. Content can have that language but getCurrentLanguage($type) will never return them ( I hope, that would be scary.. ;))

@anydpost: yeah I reacted on "But core is multilingual without language module". The language types themselves do not guarantee multilingual behavior, and those will never select any other language but the site default unless some custom language module replacement is used. I can see that for that use case, this patch is helpful. That is a pretty edge case though as it seems.

Anyway, where is existing test coverage for this one? So we made sure tests are fine. :)

I think we should be fine here. RTBC - Current language should not change even with new language module implementations.

Thanks everyone. Couple questions about the beta evaluation:

How is renamespacing a class and moving a service definition "no disruption"?

What is the impact on code that used or extended the old class? What is the impact on an existing site that uses the service? We should evaluate whether we need a CR or upgrade path here, even if we think the BC break is warranted.

#11: In this case the service is named the same, hence no disruption.

Given that the upgrade path has not even been finished, yet for the issue and this was just moved by a critical issue, there is indeed zero disruption.

We could not even write an upgrade path if we wanted to, the only thing we could do if we really wanted to keep BC is to leave a class in language.module, subclass the other class and not register it as a service, so that someone hypothetical could still subclass it from language module.

However chances of some actually subclassing that are really slim ...

Lets add a quick CR though ...

Adding a CR for this is a lot of noise for nothing IMHO. Instead, lets make sure the original issue is updated and lists the renamed services and their (final) class name.

#13: Yes, sounds good.

Alright, thanks.

So were the class and service only just added in #2354889: Make block context faster by removing onBlock event and replace it with loading from a ContextManager? If so let's at least document that in the beta eval here. :) Wasn't clear to me.

Well these are also examples for those classes which could be marked as final, as they are the end of any inheritance chain. When you would like to change the behaviour of one of them, there is no reason why you not just exchange the service and be done.

In other words, I don't see a usecase in extending those classes in contrib.

@dawehner: Not convinced by that at all. If you're going to replace the node route context service for example, it's quite likely that you want to extend.. there's very likely no reason at all to duplicate the getAvailableContexts() method, you would just extend getRuntimeContexts().

Why would you want to extend such a tiny little class?

Committed c78f1da and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

Let's update the CRs.