Problem/Motivation

Since only the parts strictly needed to objectify lower-level subsystems were converted to OOP, the language system is currently an unsettling mixture of object-oriented and procedural code, best exemplified by the following snippet from the LanguageManager:

is not autoloadable: additionally, since parts of it live in language.inc and language.negotiation.inc to avoid loading unnecessary code on monolingual sites, those need to be manually included wherever a language API function or constant is needed;

makes code hardly unit-testable, whenever the language system is involved;

is highly inconsistent and hard to integrate with most of the other D8 subsystems;

introduces a circular dependency with the configuration system:

we need configuration to know which language negotiation methods should be used to determine the values of the current language types;

if interface translation is enabled, LocaleConfigSubscriber will try to set overridden values for any loaded config object with data matching the current interface language.

Proposed resolution

Objectify the language system by moving most procedural code and constants to regular classes. Remove language.inc, language.negotiation.inc and the related explicit includes, together with the code living in bootstrap.inc.

Details

Define a basic core language manager to be used on language-unaware sites.

Introduce a separate language negotiation service, provided by the Language module, which is provided to the language manager through setter injection. This can rely on the regular config.factory service as the latter does not need the language negotiation service to be instantiated.

Register the Language module request subscriber and path processor only on multilingual sites. This allows to avoid loading most of the language system code on monolingual sites, as the negotiator is instantiated and injected while setting the request onto the language manager.

Convert language negotiation methods to plugins handled by the language negotiation service. Path processing and language switching code is moved on the related plugins, since the logic implemented by those is strictly tied to the language detection logic implemented by the plugin.

Resolve a circular dependency with the string translation service through setter injection.

Move the browser language detection logic to a reusable standalone component, which can then be used by the related language negotiation plugin and by the installer code, without introducing a dependency on the Language module.

Notes

Procedural BC wrappers are still in place for the most used functions to limit the size of the patch.

Some functions used mainly by the Language admin UI were not converted to methods yet to limit the size of the patch.

The language entity CRUD API has been left out to limit the patch size. Since these functions are rarely used (at least outside tests), we can afford to wait and introduce a dedicated language storage manager in a follow-up.

Remaining tasks

Decide: "Do we want to commit this and leave the rest to follow-up or quickly discuss the remaining non-controversial bullets?" (See #353)

(Maybe) another round of follow-up issues need to be made

User interface changes

None

API changes

hook_language_negotiation_info() and the related procedural callbacks are converted to plugins.

The language_count state entry was removed, as getting the language list from config and counting them has the same performance of getting the language count from state. This allows to remove all the synchronization code.

Most functions in the language system are moved to the Language manager or the Language negotiation services:

Well, it is probably unsettling because the request was converted to OOP without language being converted :) I agree having them using the same approach would be good, language was built out way earlier than request and OOP was not at all a universal approach back then. Looking forward to ideas as to how to best refactor this :)

To recap: language, when on, might want to override every CMI object we load so we have a circular dependency where you need to read various CMI objects to determine the language. To avoid this, I am converting the language negotiation callbacks into plugins and then copy the language negotiation settings into the container on compile time. To do the latter, I will change DrupalKernel::buildContainer to add the configStorage as a synthetic service and then LanguageBundle can use that to read whatever CMI objects are necessary and store them neatly in a container parameter. Thus, when we have a kernel, we have the negotiation services and we have the negotiation settings so as soon as we have a request to extract things from it we have the language and can override. Hurray!

Let me recap your plan myself, just to be sure I understood it correctly :)

Problem

Atm we have a circular dependency between language and configuration:

we need configuration to know which language negotiation methods should be used to determine the values of the current language types;

if Locale is enabled, LocaleConfigSubscriber will try to set overridden values for any loaded config object with data matching the current interface language.

Proposed solution

Language negotiation method are becoming plugins. I guess this is not part of the actual solution but it's the proper way to do this in D8.

Language negotiation settings are being "cached" into the container, so when processing "regular" requests we are pulling those straight from there and avoid config loading.

During a container-building request we are using config storage as a synthetic service. If I'm not mistaken this means that it will be shared among all container's scopes, actually being available to any bundle needing it to register further services and the related configuration. What is not clear to me is how this is preventing LocaleConfigSubscriber to hook-in here too. Is this related to #1868028: Raw (original) config data is not accessible?

@sun:

Hm. #7 scares me a bit. We need to be able to change the language mid-request (language is context, not config).

If the scenario above is what @chx has in mind I don't see particular problems to switch languages during a request: actually we would use language negotiation settings to determine language values for the topmost scope. Then in any point of the execution flow we can set whichever value for the languages we want, based on any required logic (e.g. user's preferred language) just before entering a new scope. Am I missing anything?

Edit: I didn't see plach's comments only sun's. I do not think there will be any functional changes. It's just moving things around -- and practically moving the CMI object caching for these objects into the container. No big deal IMO.

#9 the bootstrap config reader is using the storage directly (by default, YAML files), there is no caching, there are no events firing, nothing. It's just bare metal reading. We can't have anything else because we do not have a container to read the config factory, event dispatcher, subscribers from. While slow this is fine because it only happens on container rebuild.

This namespace does not really make sense to me, what about Drupal\language\Negotiation?

Moreover, as we start converting stuff in bootstrap.inc and language.inc, we will have classes in Drupal\Core\Language referencing stuff in Drupal\language\Plugin, hence I'd move at least the interfaces in the Drupal\Core\Language\Negotiation namespace.

Not sure about the current general approach for plugins, but it would be nice if we could inject only the configuration concerning this particular plugin. It could still live in a single language negotiation config object, but plugins here would get just their specific stuff. This would force better separation.

Currently the only reason we pass the $languages array to each negotiation method is avoid a call to language_list() for each one. We may want to move this to the constructor and inject it like the the other global-scope info.

I can't help thinking that our decision to make the language manager dependent on the request was misguided. It seems we are constantly tying ourselves in knots because of that. So I'm wondering if getting rid of this dependency and having an event subscriber that listens to the request and sets the request object on the language manager would be a sensible first step for this issue? Here's a first pass at doing this - I ran the language and path tests locally and got some fails but I think they'd be easy enough to work out, want to see what else explodes and find out what people think of this general idea? For starters it allows us to effectively get rid of the horrible language() function.

It is still not entirely clear to me why Drupal\Core\Language\LanguageManager has a hard dependency on the Request. It looks like if that request parameter would be optional, then LanguageManager could be re-used and shared as initialization and sorta factory for both the request-based and non-request container, eliminating the new procedural language_manager() helper. What am I missing?

Furthermore, I've the impression that these changes could really use some more detailed reviews from language subsystem maintainers. (erm, so consider this the first ;) ...not sure whether @plach is up to speed with the new kernel stuff, but we should ping him)

Ha! :-) Anyway at the time we had had this big long discussion in irc with one of the symfony guys and he did in fact mention the possibility of using a listener to set the request, but for one reason or another we decided a scoped service was the way to go. And now I am thinking maybe sun was right all along :-P

So once the test completes, the bot attempts to send the results to qa.d.o. Something in the response (maybe an individual assertion, maybe in the log file, maybe just an attribute of the response ... to big?) causes qa.d.o to reject the report. As a result, the test fails, is sent back to the queue ... and assigned back to the testbot which just finished testing it since it's the first one in line waiting for a new test.

Ah, so as it turns out, it wasn't testbot, it was the patch. Basically we can't assume we'll always have a language_manager in the container because unit tests use a totally bare container with no services and then they call the t() function for assertions. Badness.
So, yeah - I guess there would have been a huge number of fails resulting from this. Sorry testbot! Thanks @jthorson for the info.

To clarify my plan of attack here - I'm going to try and get those tests passing locally and then move on to marrying this to the work chx started in #17.

One implication of not having the language manager be dependent on the request scope is that we can only set the request on it for the master request, not subrequests (as we'd have no way of setting it back after the subrequest was handled) - I think this should be totally fine unless someone can think of a scenario where a subrequest would have different implications for language negotiation than the master request.

@katbailey: I feel like the problems you are trying to solve are very important but that kind of stumped on @chx's patch which went to convert negotiation methods to OO and plugins/annotations. I think we'll need that too eventually, it would be good if we don't loose that. We'll probably need yet another issue to open for that work now that this is taken to a whole different direction.

@Gabor I'm not quite following :-/ By stumped, did you mean I stomped on chx's patch? My intention was to back up a bit before any more work was done in that direction because I felt we'd be tying ourselves in knots if we continued with a language manager that's tied to the request scope.

As I mentioned in my previous comment, I was going to merge this in with chx's work (after fixing the 5 fails). If you feel the two issues need to be separated out then I can create a separate issue for the change I'm proposing and block this one on that..?

I hope I am not stomping again but I really want to make progress on this issue, and it was unclear to me how to make this work with plugins - specifically, how the plugins could be wired up to get services injected into them from the DIC. So here's an alternative approach that uses a compiler pass to register negotiators to the language manager, which then calls them directly, depending on the negotiation settings.

I've only run the tests in the Language group locally and am getting 15 fails in the UI Language Negotiation test, but that entire test would need to be rewritten (including getting rid of the LanguageTestManager that messes with $_SERVER['HTTP_HOST']) - in fact, I'm wondering if a lot of these couldn't be converted to unit tests that use a mock $request object...

Nit: $method_id makes me think it's identifying the ID of a method, like a language method, not a language-deriving method. $negotiator_id?

That said, why isn't the negotiator ID a value retrieved from the object?

Also, needs docblock.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -111,6 +112,65 @@ public function getLanguage($type) {
+ // Execute the language negotiation methods in the order they were set up and
+ // return the first valid language found.

Since order apparently matters, we should include a priority flag in the DIC configuration like we do for other order-sensitive objects.

I just reviewed #43, sorry for taking so long. TBH I don't like very much the proposed approach: registering every negotiation method into the DIC feels a bit cumbersome. Moreover it doesn't address (yet) the circular dependencies cited in the OP. I think @chx's proposal makes sense and we should start incorporating in the current work.

If you don't mind I'd like to try and merge #43 and #17, taking #9 and following into account. I should be able to work on this tomorrow.

plach: Registering every negotiator in the DIC is a pattern we're using in a number of places. ParamConverters are doing that. Route Enhancers. Route Filters. Access Checkers, although they have an extra lazy-load layer involved. I don't think that's an issue.

My understanding of #43 was that @kat went this way because she was not sure how to make negotiaton methods plugins. IMHO plugins would be way better for DX and they fit very well the concept of pluggable negotiation methods already implemented in D7 through procedural callbacks. Unless we have a strong reason (mainly performance, I guess) to go the other way, I'd really wish to try and use plugins as originally planned with @chx.

Did you guys have any look to #17? Aside from it being a raw proof-of-concept and thus missing lots of DI stuff, it looks promising to me.

To be clear, what I was unclear about was "how the plugins could be wired up to get services injected into them from the DIC" ;-) (from #43 above). I had looked at #17 before starting on this.

Sorry, my sentence was unfortunate, in no way I meant to diminish your knowledge on this matter. To clarify: my understanding was you were unsure about the best approach to make negotiation methods plugins ;)

Starting to work on this right now, I ain't sure I'll be able to post a new patch tonight, but I'll try.

The attached patch merges #43 and #17 and should address most of Crell's review. I left out the terminology stuff since right now we have such a messy mix between the old terminology and the one introduced in this patch that this could use some discussion before addressing it.

I think we should probably complete the conversion of the configuration stuff to CMI in this patch to have a clean end result.

Aside from that what still needs work is making language types plugins, converting the rest of language.inc in OO code, not sure whether everything in there belongs to the language manager. AAMOF I was thinking that a we could provide a very minimal version of the language manager and use it in monolingual sites, Language module would swap in the full implementation when installed.

We also need to add an alter decorator to the discovery process, but this should happen only when performing it in a non-bootstrap context. Still thinking about which could be the best approach. Suggestions welcome :)

One last thing: I tried to remove the initializing flag but I found that parsing plugin definitions triggers t() calls while bootstrapping a request with cold plugin discovery cache. For now I left it there but the current solution sounds a bit hackish. I'm wondering whether we can do better than that.

Shouldn't we use derivatives for this?
So just add "LanguageNegotiationUI" as the plugin ID. LanguageInterface as the class name. Reference a derivative for this plgin and we are done. (ater creating the derivative)

Sorry, I don't understand your point. What would derivatives be useful for here? We don't need multiple variations of a language negotiation method.

It's used to reference the negotiation method in external code without hardcoding the string id. Basically in some places we need to check whether the negotiation method is enabled in the language negotiations settings.

Not sure but isn't cleaner to hardcode the string id. And make a helper to get the negotation method. This feels so not like everything else we have in core. :s
Or make it a property with a helper function or whatever.

I really cannot understand your resistance to using a constant: that's what they exist for. I can understand the consistency argument, but the fact that [IMHO]somewhere else we are not doing things the right way[/IMHO] is not a good reason for doing them wrong once more.

I might be mistaken, but I think before the patch the code always set 'default' to TRUE and with the patch it's not always applied. If 'default' is set to FALSE in variable_get() then that will be used.

The patch in #66 is cycling endlessly in the queue, re-testing without reporting ... due to the testbot tripping a "Failed to send result: Parse error. Request not well formed." error. I presume this may be due to the size of the results which are being generated, since it appears to be generating the following error in the apache error log at a fairly frequent pace:

[Mon Mar 25 21:26:17 2013] [error] [client 10.20.0.107] Uncaught PHP Exception Doctrine\\Common\\Annotations\\AnnotationException: "[Semantical Error] The annotation "@Drupal\\Core\\Annotation\\Plugin" in class Drupal\\Core\\Plugin\\Core\\LanguageNegotiation\\LanguageNegotiationUI does not exist, or could not be auto-loaded." at /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/doctrine/common/lib/Doctrine/Common/Annotations/AnnotationException.php line 52

As a result, I'm killing testing on the patch, and would ask that you do not re-test it.

Needed a reroll after services moving to yaml - also the compiler pass was still being run, I wonder if that was the reason it made testbot choke? I've removed the compiler pass and just made minor changes to a couple of tests.

Thanks for the review @dawehner. Except below items, others in #78 are fixed in this patch:

1. Should the db connection be injected?
2. I try to understand whether we need the constant on the class + the id. I can't find a place where the METHOD_ID is used elsewhere - Not sure this can be removed.
3. It would be possible to just store the language.negotiation config object, as nothing else is needed on here, but that's out of scope of this issue.

This happens when there are so many errors that the XML-RPC request times out between the bot and qa.drupal.org so I cancelled this patch because it seemed never to finish. If tests pass locally, let me know and we will see what we can do.

head is broken so i cant do much, i fixed obvious stuff, and what i could test locally, some language tests fail about half..

I fixed installation and moved plugin manager to use ContainerFactory.

I moved Plugins to Core/Language namespace, they shouldnt be in Core/Plugin
Also, why not have them in the language module? well we need for the installer one, but couldnt the rest be moved to language module?

Also, why not have them in the language module? well we need for the installer one, but couldnt the rest be moved to language module?

Well, theoretically the language negotiation methods are part of the Language system, not the Language module, which only exposes a UI to configure them. Since we need the installer one in the Core namespace anyway, I thought it would be more consistent to place them together.

- We didn't reset the language list when adding a language, so when updating the prefix defaults, the new language wasn't there and it didn't get updated.
- We need to rebuild the container when adding a language and changing the default language. Alternatively, we need to do it when calling language_save(), not sure yet what makes more sense.

Additionally to that, I also have very crazy issues with the container being rebuilt or better, not being rebuilt in tests. I can't trigger the rebuild on a page request properly (it doesn't seem to rebuild) and when I do it from within the test method, it does not see the new configuration?!

@catch: well, that *someone* marks an issue critical does not make it critical for maintainers. I've been asked to validate any such issues that need API changes with maintainers to ensure there is consent involved. I also think its fine to keep the API change tag(?)

But on the next request, when booting again, it loads *this* file, which is something completely else:
.../sites/default/files/php/service_container/service_container_prod_simpletest330378.php/8a5a978dc3eb937fc9c0d632e4a6bbb1a4606a4ccf2b78beebb7083d247ecbf0.php

So it is within the normal service_container folder but a different filename. WTF?

Turns out, we've fixed that already! The problem is that the file public path isn't overridden in time during a test request, so we sometimes use the parent directory, but we can't delete that later on or clean it up like all other test data.

Ok, that didn't fix as many tests as I hoped, *but* it's now actually possible fix most of them. This should fix a lot, still a few that I don't understand. And still a ton of crazy procedural code, and the old hook is still around.. me confused a bit ;)

@andypost: The patch isn't ready for code reviews, I'm just trying to get it somehow working for most tests and then improve it.

@plach: Can we discuss your ideas/plans with this? Would like to know how far you wanted to go here, don't really understand why we still have hook_language_negotation_info(), I suppose you just didn't get to that yet, it's not that it's still supposed to exist?

Some clean-up, caching the negotiation plugins, adding a separate install-time language manger to avoid the conditional state stuff, I think we might also be able skip the negotiation stuff there and only support the default language. Updated the plugins a bit and replaced the unused injected database with the configuration storage. Maybe we don't want to use the config factory at all there and directly load from storage, not sure.

The config factory can not be injected into the language manager as there is a circular dependency due to the locale stuff.

I had a look to the current patch and I think most of the legacy procedural code can be moved onto the language manager, more details below.

I noticed the latest iterations introduced a Language Manager provided by the Language module. This fits perfectly one improvement I was planning, not sure whether I mentioned it, that is providing a basic core Language Manager and a complete one provided by the Language module, which would swap it in when more than one language is enabled. This approach would have the advantage of not requiring to load most of the code currently in language.inc on monolingual sites. AAMOF the basic LM would assume a monolingual scenario and would provide only very simple versions of the exposed methods (an interface would be nice here :). OTOH the full LM would provide the full method implementations including the language negotiation stuff. This would more or less match the current situation where we load language.inc and language.negotation.inc only in multilingual environments.

One thing that is not clear to me is that the Language module LM currently receives a $config_storage instance and a config array. Unless I am missing something, it would make more sense to either always use the config storage or to put the info additionally required in the config array. Having both is confusing.

language_negotiation_info() -> This must DIE along with the info hook and the related docs in language.api.php.

language_types_info() -> Language types should become plugins as well, I guess. If that happens this must DIE along with the info hook and the related docs in language.api.php. Otherwise this should somehow move to the LM.

In any case we should retain the ability to alter plugin definitions, I'd simply keep the current alter hooks. This might be tricky as we will end-up invoking hooks very early in the bootstrap phase, however I've done a quick test and things seem to work properly.

language.negotiation.inc

What's left in here is mostly obsolete constants and the language switcher callbacks, which I guess should become plugins too, or maybe plugins could implement both the LanguageNegotiationMethodInterface and a new LanguageSwitcherInterface. This might make sense as the implemented logics are usually tied.

The session URL rewriter needs to become a path processor as PathProcessorLanguage (which should be renamed to something like PathProcessorLanguageURL to match PathProcessorLanguageSession).

The other functions/constants could move to the LM, I guess.

language.module

language_negotiation_include() should DIE along with the related calls when we are done with the two files above. It would be nice to alter the negotiation plugin definitions to add the config paths (which should use the routing system patterns btw) from here, instead of hardcoding them in the core namespace where they wouldn't make sense. AAMOF the UI is provided the Language module.

Sorry, if this sounds as a lot of stuff, but I wanted to be sure not to forget about all of this :)

@plach: Thanks, haven't read in detail yet but that makes a lot of sense to me :) Agree that we could move all the multilingual and and negotiation stuff completely to language module, that will make the install time language manager unnecessary.

file_public_path is in, will do a re-roll and start converting this stuff asap.

I was thinking about the conversion of language types to plugin: atm we have the system.language.types config object which will no longer make sense, as the info contained there would be provided by the plugin definitions. The primary reason behind the original variable was avoiding to perform a hook invocation during bootstrap, a sort of early cache. I think this role maps way more accurately to state, that is we could cache the language type definitions into state, which is available to the language manager.

If we do the same with language negotiation methods, we should no longer need to inject the config storage/array to break the circular dependency. AAMOF, as it happens now, language negotiation and type info will be (re)built, altered and eventually stored in the state while configuring language settings in the UI. Then they will be always available during bootstrap.

Sorry i have no idea whats wrong. But it looks like the Field UI to enable/disable Field translation is broken for fields with existing data. (Path: admin/structure/types/manage/article/fields/node.article.body/field)

Re-roll, fixed that constant for now and fixed the typo pointed above.

I'm not sure why we even need that METHOD_ID stuff. I don't think we do that anywhere else, why not just put the ID as a string in there. This means that to parse the annotation, we need to load the file in PHP, something that we're currently trying to avoid.

@Berdir: as for the method_id, we put it in the negotiation system on the negotiated language, so code later on can tell how Drupal arrived to the language in question. At least tell the method that picked the language at the end. Also, it is used in the tests to ensure proper behavior, so not only the right language is picked but with the right method :) Not sure if this helps at all :)

Yes. So it's the plugin id. Why does it need to be a constant on the class that is then self-referenced in the plugin definition, that is my question :) Of the 37 (rough guess ;)) other plugin systems, nobody else does it like this.

Well, its used on initializeType(), depending on how that is implemented, we may need it or not. The current code seems to store the methods keyed by ID. If we just get it as a plugin ID, then that is fine to use as-is as well IMHO.

FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1862202.objectify-language.186.patch. Unable to apply patch. See the log in the details link for more information. View

Another re-roll, and a hell of a debug run, that language negotiation stuff is so messed up :(

The problem turned out to be very simple, the URL didn't have the types set, which apparently have to be. Which is weird, because he lists all that core has be default, and only has to do that because during the test, content is not seen as configurable and therefore not part of the default. Then we somewhere in the middle dropped the method for the content negotation and ended up with the default en.

Also found a lot of other things, various dead drupal_static_reset() calls, the interface negotiation plugin was missing (shouldn't that be the default and not URL?

Anywy, this should hopefully fix the translation UI test fails.

And I also had very weird class already defined fatal errors for the test negotiation implementations when using the constant for the ID, so hardcoded it there for now.

How close is this patch to being committed? Is it worth doing to variable > configuration step first? Is it worth splitting into multiple patches so we can get one committed and then do a variable > configuration step?

FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1862202.objectify-language.195.patch. Unable to apply patch. See the log in the details link for more information. View

#194: The reason of this issue is to be able to do the variable to configuration conversion. Yes, it's a blocker, because this needs to happen first. This is too low level to convert to CMI directly.

#193: Those methods are needed, that's why I moved them there. They're part of the negotiation plugins and yes I'll need to find a better way to deal with them and make them nicer, I just moved them. But I just want to get the tests to pass first.

Fixing more tests. Also got confused, the interface negotation is called UI now. Cleaned that up again.

I'm not sure why we even need that METHOD_ID stuff. I don't think we do that anywhere else, why not just put the ID as a string in there. This means that to parse the annotation, we need to load the file in PHP, something that we're currently trying to avoid.

Well, we have some business-logic that requires to be able to explictly tell which detection method was used to determine language, hence having a constant is way better for DX instead of using a plain string. OTOH is there's a good reason not to use it, and performance might be one, then we can also skip it.

If you are referring to this one, it should not happen when installing CT. Content language is defined by core and it should inherit interface language even when CT is disabled. This is how D7 works, btw.

@plach: Absolutely agree, that's what I expected to happen. But looks like at least in the test, something is making content language negotiation configurable and then it has none, because by the time language_install() runs, it is not. Will figure out why.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -177,16 +347,104 @@ protected function getLanguageTypes() {
+ * (optional) Specifies the state of the languages that have to be returned.
+ * It can be: Language::STATE_CONFIGURABLE, Language::STATE_LOCKED, Language::STATE_ALL.
...
+ * An associative array of languages, keyed by the language code, ordered by
@@ -308,4 +566,14 @@ public static function getStandardLanguageList() {
+ * An array of all language types where the keys of each are the language type

80 chars.

+++ b/core/lib/Drupal/Core/Language/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php
@@ -0,0 +1,153 @@
+ // so we multiply the qvalue by 1000 to avoid floating point comparisons.

1. Fixed
2. Don't know which issue id you're talking about
3. Fixed
4. Don't know which issue id you're talking about
5. Because then the config you inject via the contructor would have to know about all of the config within the system.
6. Because the object has no moduleHandler as a property
7. Fixed
8. Fixed

Applying: 226
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Removing core/modules/language/tests/language_test/lib/Drupal/language_test/LanguageTestServiceProvider.php
CONFLICT (modify/delete): core/modules/language/tests/language_test/lib/Drupal/language_test/LanguageTestManager.php deleted in 226 and modified in HEAD.
Version HEAD of core/modules/language/tests/language_test/lib/Drupal/language_test/LanguageTestManager.php left in tree.

that file was modified by #1999388: Use Symfony Request for language module
checked if that code was moved anywhere in that issue and it did not look like it.
so thought I would just remove it, same as in our previous patch.
@berdir confirmed it was ok to kill it.
deleting.

head is setting the accept language differently and this patch removed the old way with this one...
since this is in a test, we can just create the request the way we want it.
and we dont want to be calling out to \Drupal->request() because we want to be able to test it.
Note: @berdir says a follow up can be to move this to a unit test since it is close to one anyway.
So taking out the lines from head and keeping the lines from the patch.

<<<<<<< HEAD
use Drupal\Component\Utility\String;
use \Symfony\Component\HttpFoundation\Request;
=======
use Drupal\Core\Language\Plugin\LanguageNegotiation\LanguageNegotiationSession;
>>>>>>> 226

use's should have been under the @file. that can be a followup.... wait.

next chunk of conflict is a huge chunk being removed in this patch. diffed what it was before that was removed with what this is removing and the only diff was this patch also removed one of the use's that was after the @file.

so just removed the hunk. when I do, phpStorm tells me all the use's are unused. removing them all. even LanguageNegotiationSession which this patch was adding.

In #2107427: Regression: Language names should display in their native names in the language switcher block it is evident we need a version of language_list() which returns the language names in their native form (vs. right now all localized to the request's language). The former is needed for things like the language switcher block. That issue so far extends language_list() with arguments and different caches. Any better suggestions? Sounds like there are things we could do there that would fit better with this refactor :) Either way, the OO version will need to take care of that too.

I was confused by this removal, but it's needed now that we use two different language managers in installation and on an installed system. That's why we have to register the service by code later on the patch.

Sorry, I am in the middle of a complete revamp to move most of the language negotiation functionality to the Language module and leave only the basic stuff in core/lib. I should have a new patch ready tonight.

And here it is, lots of stuff going on here so no interdiff. Basically most of the language manager now lives in the language module and only the basic stuff is still in the core namespace. One relevant thing is that now only the complete LM depends on config data. This patch also factors the language negotiation code out of the LM and moves it into a dedicated class, which is injected in the LM when needed. In that phase the full config factory is available to language negotiation plugins, whitout re-introducing the ciruclar dependency on the LM. Finally every reference to language.inc and language.negotiation.inc has been removed and the few leftovers of those files now live in bootstrap.inc (BC layer) and language.module (configurable language negotiation API).

I am afraid this will re-introduce quite some failure, but I think it's a big step ahead: wrt the plan outlined in #155 we should be missing only the "pluginification" of the language types and then we should be done.

FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch language-oo-1862202-278.patch. Unable to apply patch. See the log in the details link for more information. View

Rerolled after #1786490: Add caching to the state system. Since checking language count no longer is an expensive operation, I just removed the language_count state entry. Lost the interdiff during the merge, sorry.

I did some quick profiling and couldn't spot relevant stuff, as the differences between runs on the same branch by far outnumbered the differences between head/patch, both in a monolingual and in a multilingual environment. Here is the XHProf diff between head and patch on a (non-default language) front page with 25 nodes (session files attached):

I picked the most performant runs for both head and patch, but it would be good if someone else could do some more profiling to confirm my results. The query log showed no apparent differences in the query number/duration.

I think the fact that we can move all the dynamic stuff to language.module and strip down the core language manager to being so simple and hardcoded is really cool.

Also, the patch is too big for poor dreditor ;) And to review it in detail in a single go, but I think this is really getting ready and we need to move forward to be able to unblock the CMI stuff and just as important, unblock @plach to work on other issues again :)

The complexity of this save/purge/update stuff is extremely complicated and caused all sorts of craziness when I was working on fixing tests with things getting lost in the middle. Not here, but is there a chance to simplify this afterwards?

I was wondering about this too, would be nice if we could make it lazy-initate the language types that we need, would avoid to load and execute all the negotation stuff for every language on every request.

6. I may be wrong, but the rationale is that we want to test a fixed behavior, that is language prefixes being there, thus we need to hardcode that in the test. This is more or less the same thing we do, for instance, when testing storage: we use the API to load/save stuff and directly access the DB to perform assertions. In this particular case, if I am not mistaken, using the API would mean relying on the host environment language configuration, which usually does not match the test site's one.

11. Totally. I get a headache every time I try to wrap my mind around that code (and I designed it).

#293.6 Hardcoding it makes sense. I think the problem is that just by making it hardcoded, it will still run through the url altering, so if the current language would have a prefix configured, you'd end up with a double prefix, I think I had that once.

13: Not exactly what I meant and there are a few more like this:) Here's a patch that updates all language managers that I could find to use the interface, had to update a few classes to type hint on the interface for that.

Needs tests was added in reference to upgrade tests I think, which we no longer need.

@larowlan:
1. Not sure if we can inject the string translation service into the language manger.
3. Agree that adding language_rebuild_services() as a function feels wrong, not changed yet.
7. Fixed.
8. Nice, simplified.
9. Not sure what else we could do, I assume this is the same right now...

14/15 and others: Yes, I moved a few of the domain tests to a unit test because they were impossible to re-create in the old way. In general, I think the test coverage of those negotiation methods is quite good, we're just moving code around, so I'd suggest to do any further unit test additions/conversions in follow-ups.

This should address #294.1-2 by using setter injection on the LM to provide the string translation service. This allowed also to move the last language-related function from bootstrap.inc to the LM, yay! This required some additional DI, at least where it wouldn't require to touch dozen of files.

#294.3 was also addressed. As a bonus the function body has been simplified as there were leftovers of a previous approach.

Additionally this moves also language_update_locked_weights() onto the LM.

4. I think would be ok to move those to the respective classes (the related negotiation plugins) in a follow-up, we are already touching too much stuff here.

5. We should be able to get rid of that during the CMI conversion.

9. I'd avoid touching the business logic here, we are just moving stuff around.

12. Done :)

@berdir:

What do we need to finish this? I can't RTBC myself,

I think we should be done. I'd like to move also language CRUD functions on the configurable language manager, but to achieve that we'd need to fix another circular dependency between the language manager and the entity manager. I think it's ok to leave that for a follow-up since there are many tests that need to be updated.

I will ping @tha_sun to check whether he is available to review this until RTBC, unless @larowlan feels bold enough to, obviously :)

Reviewed the patch. It is very hard to do line by line obviously, and dreditor will not work with this patch anymore... The architectural changes explained in the issue summary and evident in the services setup are superb IMHO.

I only noticed "minor" problems like getDefaultLanguage(), getLanguageList() but loadLanguage(). (Also getLanguageList() could be getLanguages(), no?). Also the naming of core/lib/Drupal/Component/Utility/Browser.php may be too generic? It only provides language extraction from the list so far. (But the abstraction of that from the data providers is also nicely done).

All-in-all this patch seems to be full of win, and the naming stuff can be cleaned up afterwards as well.

While there are several @todos removed, there are also several added, and not all of them are qualified with URLs to issues. It would be important to open the followups desired and reflect them in the @todos. Other than that I have no complaints that would hold back this patch :)

Thanks a lot for reviewing this! I'd wish to avoid further API changes after this is committed if we can, so any suggestion for new method names is welcome, but I'd prefer to implement it here.

About getLanguages(): in itself sounds perfect but I'm wondering whether it might clash with getLanguage(), which does a completely different thing. Maybe we should rename the latter to getCurrentLanguage()? This way loadLanguage() would become getLanguage(), which in this scenario would make perfectly sense.

Also the naming of core/lib/Drupal/Component/Utility/Browser.php may be too generic? It only provides language extraction from the list so far.

Yup, I was trying to be future-proof :)
Should we have any other browser-related utility function it would be better not to be too specific in class naming.

Well, for educated suggestions on API changes (especially if you want this to be the final-final API) it would be good to collect a table of API changes. Either way, it would be good to have before/after code examples for the issue summary for some typical tasks. Those will be good to copy-paste into D8 docs later on, so not wasted time at all :)

Thanks for the updated issue summary. The main bit I'm not sure about is this:

Replace it with a fully-functional implementation provided by the Language module. This depends on the configuration system to retrieve the list of configured languages. The circular dependency is fixed by using the config.storage service instead of the config.factory one, that is by reading configuration values directly from the storage, thus bypassing the context system.

Why can't we do the following?

1. Read the config with no language override at all, but using config.factory

2. Add the language context dynamically after this has happened (similar approach ought to be possible once the built-in language support/single override patch is in too).

However this feels like something we could open a follow-up for - even if that follow-up is another critical this patch unblocks lots of others.

I think this addresses #320 by introducing a new language manager provided by the Locale module. It extends the one provided by the Language module with a method performing registration of the locale config subscriber after the language manager is instantiated.

It also renames LanguageNegotiationMethodInterface::getLanguage() to LanguageNegotiationMethodInterface::getLangcode(), since it returns a language code and not a language object.

@Gabor:

After compiling the API changes list, I'd be definitely in favor of doing the following renames:

Thanks a ton for working on this! Awesome job. Here comes a deeper technical review.

Before you read on, let me clarify a few thoughts upfront:

This change is an epic and utterly important milestone for D8. We need it to move forward as quickly and as pragmatic as possible.

I am interpreting the major change-set here as the initial commit for a total revamp of Drupal's language subsystem. There's no point in downplaying the extent of this issue and the proposed changes.

Most of the basic/fundamental architectural aspects make sense to me. Only some of them are concerning, and some other aspects do not appear to use or follow common paradigms and techniques that are being used elsewhere in the system already (mainly concerning plugins).

However, we need this revamp ASAP. We are able to revise major parts of the technical implementation in follow-up issues. — That should not be unexpected to anyone in the first place, since, again, we're swapping out one of the most fundamental aspects of Drupal's base system with a new implementation. Major clean-ups, simplifications, and improvements are only natural after committing such a big change.

The vast majority of review issues outlined below can and should be addressed in separate, partially major follow-up issues. Unless @plach or others beat me to it, I'll try to help with creating them.

What matters most for D8 and the Drupal product as a whole is that we move forward. This change blocks a full range of other, partially critical issues. As such, even if I have some major reservations concerning architectural and technical aspects, strategically and tactically it would make more sense to move forward with this (potentially as-is), so as to get the initial baseline into HEAD, to unblock other issues, and to fix up, clean up, and polish this new code separately in follow-up issues instead.

That's not meant to downplay the enormous effort that went into this patch. Thanks a lot to @plach and @Berdir for making this happen! :)

However, quickly checking the surrounding code of install_begin_request() within that spot, there appears to be more stuff going on that lives in the wrong spot, so let's not do that here, but ensure to create a follow-up issue to clean up the language related stuff in install_begin_request() instead.

"default locked languages" requires one to understand what Drupal understands under that term. Wasn't there a formal expression for these linguistic/language codes à la "universal languages" or "non-linguistic languages" or similar?

Would be great to have a follow-up issue to find a better API/method name for this.

Renaming this method from loadLanguage() to getLanguage() does not really make sense to me. Neither the old nor the new one really makes sense. The method does not load or get a language. It loads translations for a language.

loadTranslations($langcode) or getTranslations($langcode) would be more accurate.

Happy to defer this rename to a follow-up issue though, unless you want to quickly do it here.

The current ConfigurableLanguageManagerInterface approach cannot really work in practice:

1. The interface is defined in/by Language module, but if Language module is not installed, then its namespace is not registered/available, hence the interface cannot be autoloaded.

2. If our intention is to globally declare a swappable API for configurable language managers - in other words - allowing for alternative implementations of the exact same concept and features in contrib, then we need to declare and supply that interface in Core.

3. However, I wonder whether there really is a use-case for swapping out Language module and/or ConfigurableLanguageManager in the first place? Did anyone ever attempt to do that? Why would you want to do that? If there is no use-case, then we do not really need a separate interface.

4. Perhaps more fundamentally, I wonder why any kind of other module has to be aware of a non-configurable vs. configurable implementation of LanguageManager? — Shouldn't the additional configurable concepts be transparent for the rest of the system? Why does it matter to e.g. Block module or any other module whether languages are configurable or not? → All they should care about is whether the site is multilingual or not. (?)

5. The Configurable…Interface should not define any methods that are outside of the scope of "Configurable"… — It appears that at least language type info has been mixed into that, which has nothing to do with languages being configurable or not. → Unless there is a very good reason for why the base/core LanguageManager cannot expose (hard-code?) a predefined list of language types & Co, then all of those methods should (1) either be moved into LanguageManagerInterface or (2) we need additional interfaces (in core).

The important parts are (1) separation of concerns through well-defined strategy interfaces and (2) ensuring that the implementation is properly swappable (and can be feature-detected, if necessary).

Slightly OT, but then again also not — in general, I'm very skeptical whether we really need to provide the level of abstractions of e.g. language types in Drupal core.

Almost no one needs these facilities, but yet, we're shipping with tons of complexity. Based on what I've heard, people much rather found use-cases for e.g. additional language types, which could have been core patches to extend a built-in and hard-coded list at any time (minor feature addition).

Removing all unnecessary pluggability would vastly simplify the language system in Drupal core, for the benefit of everyone.

My considerations are not only based on technical aspects, but also product/organizational community aspects: It is extraordinarily concerning that (literally) only a handful of core contributors are able to digest the (utterly complex) language subsystem in core in order to work on issues like this here.

So as a completely separate follow-up to this issue, I'd really appreciate to have a discussion about getting rid of as many pluggable language stuff as possible, so as to simplify the implementation provided by Drupal core, and if necessary (at all), to leave a more complex and more pluggable re-implementation for contrib.

…and make the plugin use the (existing?) config factory on the language manager instead? — That said, why do these plugins need access to other configuration in the first place? Normally a plugin gets its configuration injected and that is self-contained; i.e., a plugin should not need any additional configuration besides its own?

Shouldn't the check whether the returned language of a(ny) negotiator plugin actually exists be rather part of the negotiation manager, instead of possibly duplicated one-off implementations like this?

I'd expect this method to just return its negotiation result. Whether that result is valid and can be used or not is a completely different question?

These lines of code are breaking the contract of the getLangcode() API method — instead of distilling information from given context, they are manipulating global state (and context).

The operation is conceptually wrong and misplaced, because this negotiator may not necessarily yield the final negotiation result to begin with.

Given that there is a use-case for such an operation, we need to enhance the negotiator plugin API to allow a negotiator to perform operations in the event that its negotiation result is actually used, through a new method; e.g., persist().

All of the constants in the language classes/interfaces (including STATE_…) are poorly named, because they (1) require one to understand deep internals and (2) do not map at all to their intended purpose/meaning.

E.g., these here appear to be defining the source for URL negotiation, but yet, they are named "CONFIG_…" instead of "SOURCE_…".

Let's create a follow-up issue to re-evaluate and rename all language system constants.

Thanks for the detailed review @sun. I think some of these changes are easier to do before committing this instead of creating follow-up issues (that might be critical too if they result in API changes), maybe @plach has time to do an update tonight?

Thanks for starting to prepare the change notice. I think it would make sense to have at least two change notices here, one that targets users of the Language Manager and all the different API functions/methods. And the second to describe how language negotiation implementions have changed, by moving from a hook + callbacks to a plugin.

Rerolled and addressed two of @sun's points - 5 and 7. I don't think it is necessary to use the kernel config storage - just plain all config.storage will do and that has a listAll() cache so it should make the isMultilingual quicker - but actually this should not really matter as this called during compiling the container so not critical path. But exposing kernel.config.storage as a service seems like it could cause trouble so lets not.

I am interpreting the major change-set here as the initial commit for a total revamp of Drupal's language subsystem. There's no point in downplaying the extent of this issue and the proposed changes.

To be honest my goal for this issue was "just" modernizing the existing code to match the D8 architecture. There was agreement in the D8MI team that the language (negotiation) system was good enough for our current goals, and that we should focus our efforts in other areas needing more love. This is not to say that the current system is perfect, but we didn't see the need of spending too much energies in refactoring it.

However, we need this revamp ASAP. We are able to revise major parts of the technical implementation in follow-up issues. — That should not be unexpected to anyone in the first place, since, again, we're swapping out one of the most fundamental aspects of Drupal's base system with a new implementation. Major clean-ups, simplifications, and improvements are only natural after committing such a big change.

I certainly was expecting some follow-up work, but honestly I did not foresee major changes after the initial commit: we are moving towards beta and, if I didn't miss something big, the current recommendation is to commit stuff that bring us closer to it, and not the opposite. I guess that adding more major/critical API changes to the queue after this is committed would not help with that. Additionally I need to get back to the entity stuff ASAP, so I don't think I'd be able to work and other big changes in this area. This is why I'd like to address your main concerns now, and leave "optional" ones to anyone having the time to work on them.

I'd be ok with this if it turns out we cannot find an agreement quickly.

About #10:

The interface is defined in/by Language module, but if Language module is not installed, then its namespace is not registered/available, hence the interface cannot be autoloaded.

Are you sure about this? From my tests it just retuns FALSE if the interface does not exist.

Perhaps more fundamentally, I wonder why any kind of other module has to be aware of a non-configurable vs. configurable implementation of LanguageManager? — Shouldn't the additional configurable concepts be transparent for the rest of the system?

The answer is obviously yes :)

AAMOF we have only three references to ConfigurableLanguageManagerInterface outside the Language module (excluding unit tests):

One in BlockFormController where it makes evident an architectural problem: the Block module exposes a (kind of) visilibity API, that other modules should implement. It should be responsibility of the Language module to alter the configuration form, provide its visibility conditions, and implement access control based on those.

Two in the User module, which is implementing a UI to configure the two user-related negotiation plugins. And those should probably be provided by the User module itself.

5. The Configurable…Interface should not define any methods that are outside of the scope of "Configurable"… — It appears that at least language type info has been mixed into that, which has nothing to do with languages being configurable or not.

You are right: definable language types should be part of a different interface, something like \Drupal\Core\Language\ExtensibleLanguageManager.

So as a completely separate follow-up to this issue, I'd really appreciate to have a discussion about getting rid of as many pluggable language stuff as possible, so as to simplify the implementation provided by Drupal core, and if necessary (at all), to leave a more complex and more pluggable re-implementation for contrib.

I understand the goal, although from my (very biased) POV I don't think the Language system is more complex than many other core subsystems. My personal feeling about this is that very few people actually care about knowing it :)

Anyway, already in D7 one of my design goals was allowing for the whole system to be swapped/replaced, hence, as long as we retain the capability to implement the same feature set in contrib, I have nothing against such a discussion. I probably won't have the time to work on that, though.

About the rest of your review:

5: Fixed by calling the bootstrap config factory directly. See the interdiff in #348.

9: I don't get this point: getLanguage() just returns a (valid) language object given its language code, there are no translations involved here.

12: This is the kind of unnecessary API change that I'd like to avoid if we can come up to a quick consensus: I have no strong preference about this, I'd be happy to shorten method names by removing the "language" term wherever it makes sense. Gabor et al, any preference on this?

13: Actually I think we can get rid of that method, its implementation is really trivial and I am not sure it actually makes sense. Language types are defined in code, so disabling them in config doesn't really make sense.

14: If we want to support lazy language negotiation, I cannot see an alternative to storing those into the negotiator. Anyway, I will fix #2.

15: Why? It's statically cached and the loop is executed on an average of just one plugin. I did some profiling earlier and I couldn't spot evident performance changes with the patch applied. IIRC adding that in a compiler pass was tricky because retrieving plugin definitions while building the container resulted in lots of problems. Happy to open a follow-up to explore this solution more thoroughly, if you think it's worth. Manually probing outside the method saves one method call for each url generation and might be a worthy optimization: we'd need to profile that to see whether keeping it makes any sense.

16: Yes, this is not ideal, AAMOF in the early patches here I was trying to provide plugins just their configuration and avoid DI. There are some problems though:

Currently the CMI keys are named a bit randomly, so it's not easy to automatically map a plugin to its configuration.

Some plugins may need to access the other plugins' configuration, for instance the URL fallback needs that.

Some plugins are used also in other contexts (language switching, path processing) so we'd need to pass services in all those cases (and inject them in the caller code).

We could open a follow-up to explore this, but I'm not very optimistic about it.

17: Yup, I don't find that name particularly self-documenting either, but I felt changes like this were a bit out of scope for this issue. What about LanguageNegotiationConfigured?

19: Good point!

24: Hey, I didn't write every line of that code! I was just moving it around ;) I will fix it, though

8: Not sure about that: initLanguageManager() updates the language manager status, while retrieving the default language from it seems unrelated to me. AAMOF we don't need the translation manager to retrieve the default language.

11: Done

13: Done

14.2: Done

16.1: Done

18: Done

19: Done

20: Done

21: I think there is a use case because in a lot of places we need to refer to the plugin identifiers, often to implement specific business logic. This avoids repeating the use of a plain string which would be more error-prone.

22: Done

23: Done

24: Done

25: A follow-up works for me, although I don't think renaming all language system constants now is a good idea: they are used in a gazillion of places, aside from the API changes it would mean breaking lots of patches.

Do we want to commit this and leave the rest to follow-up or quickly discuss the remaining non-controversial bullets?

Note that #2161397: Update to Symfony 2.4.1 (which seemed innocuous enough) conflicted with this, so I rolled it back. However, it may be worth looking at the fixes in that patch to see if they make sense to pull in here.

I agree with @sun that this is important to get in sooner than later and that we can discuss details in followups, although as said above a few times, admittedly the D8 multilingual team has other loose ends we are working on (schemas, content entity stuff, etc). This blocks way too much other things, so it would be good to get it as soon as possible.

(1) They are hard-coding the BootstrapConfigFactory even more than it was before? — Given this use-case, perhaps it wasn't actually wrong to expose it as a service, and instead, (i) we need to sure that it is no longer registered/available in the compiled/regular Container and (ii) for clarity, it should be renamed to ContainerBuilderConfigFactory.

(2) They are further hard-coding a baked-in concept of a ConfigurableLanguageManager override vs. a non-configurable LanguageManager, whereas the config factory needs to get the current, request-specific language in all cases. Neither the fact that the site is multilingual nor the fact that languages are configurable plays any role.

However, let's re-evaluate and address that in a follow-up, if necessary.

Just for kicks:

To be honest my goal for this issue was "just" modernizing the existing code to match the D8 architecture.
…
I certainly was expecting some follow-up work, but honestly I did not foresee major changes after the initial commit.

Given the major changes to one of Drupal's most fundamental base systems here, I believe that's as if I'd (1) refactor the entire installer and (2) wouldn't expect any kind of major or possibly even critical follow-up issues. ;-)

Even the best patch authored by the best developers would have that consequence here. :) My main intention was to clarify (for core maintainers) that this change will require a good amount of follow-up work, both concerning already known issues as well as unexpected bugs.

In other words, I tried to underline the importance of getting this patch committed rather sooner than later, so we can move forward. :)

#341.12: This is the kind of unnecessary API change that I'd like to avoid if we can come up to a quick consensus: I have no strong preference about this, I'd be happy to shorten method names by removing the "language" term wherever it makes sense. Gabor et al, any preference on this?

In my mind, renaming methods is a simple search&replace across core, so even if it does present an API change (on top of this here), ensuring well-named class methods could happily happen in a follow-up issue, IMO.

Let's have that follow-up issue to not only discuss those method names, but also, all of the other (weird) naming like the "selected language" (statically configured fallback language).

Likewise, #353 introduced some major inconsistencies between LanguageNegotiation vs. LanguageNegotiator namespaces and class names, both within plugins but also outside of plugins... The change to "negotiator" for plugins makes sense to me, but let's make sure that the whole shebang makes sense (in that follow-up) :-)

/me LOLs at UserAgent::getBestMatchingLangcode()! :-D — That was really meant as a stupid suggestion only ;) → One more name to re-evaluate :)

#341.15: [PathProcessorLanguage] Why? It's statically cached and the loop is executed on an average of just one plugin. I did some profiling earlier and I couldn't spot evident performance changes with the patch applied.

IIRC adding that in a compiler pass was tricky because retrieving plugin definitions while building the container resulted in lots of problems. Happy to open a follow-up to explore this solution more thoroughly, if you think it's worth.

Manually probing outside the method saves one method call for each url generation and might be a worthy optimization: we'd need to profile that to see whether keeping it makes any sense.

Let's create a follow-up issue for this. Paths/URLs definitely depend on the current (sub-)request [think ESI], so any kind of static cache can very easily not be a given and not reliable/performant anymore.

Yes, this is not ideal, AAMOF in the early patches here I was trying to provide plugins just their configuration and avoid DI. There are some problems though:

Currently the CMI keys are named a bit randomly, so it's not easy to automatically map a plugin to its configuration.

Some plugins may need to access the other plugins' configuration, for instance the URL fallback needs that.

Some plugins are used also in other contexts (language switching, path processing) so we'd need to pass services in all those cases (and inject them in the caller code).

IMO, this aspect definitely needs a major follow-up issue, because the current implementation of language negotiator plugins massively diverges from any other plugin implementation in D8 that happens to rely on plugin-specific configuration.

We've invented very smart concepts in the meantime. By now, I'm absolutely sure that someone like @tim.plunkett will be able to refactor this plugin code in a few minutes to leverage them. :-)

Likewise, even with the additional changes, I still don't see why we need to maintain so much state on language negotiation plugin instances instead of injecting the current request + current user into the getLangcode() [negotiate()] methods. — Architecturally, I don't see why these language negotiation plugins shouldn't be able to operate in a "stand-alone" way; i.e., (almost) comparable to static classes/methods.

EDIT: Architecturally, I'd compare language negotiator plugins to text/input filter plugins → they may get additional context injected, but in essence, each of them ought to be able to operate without.

But again, while that's going to be a major Language Negotiation API change, let's move forward here and clean up in a separate follow-up issue.

And once more to ensure I'm coming across like a parrot:

The gist of this change is fine. Let's do this now. Separate follow-up issues, pretty please. :)

Given the major changes to one of Drupal's most fundamental base systems here, I believe that's as if I'd (1) refactor the entire installer and (2) wouldn't expect any kind of major or possibly even critical follow-up issues. ;-)

Sure, what I was not expecting were major architectural changes like stripping out the whole plugin-based language negotiation system :)
We need a follow-up to discuss that, I guess.