Objectify the language system

Closely related is #1859110: Circular dependencies and hidden bugs with configuration overrides.

d8mi tag?

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 :)

BTW there is also #1512424: Add a LanguageInterface, and property setters/getters to Language class about the proper OOP-ifying of the Language objects themselves, where there is not really a great agreement as to how it should look like. So there is definitely a need for good ideas and feedback :)

This was on my todo list (actually not in very high position :), as well as converting language detection methods to plugins.

I'd be totally happy to work on this but I have some more ugent tasks to complete right now. If someone wishes to step up I'll gladly review the patches, otherwise this will have to wait...

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!

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

@chx:

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.

@chx:

Not sure whether this is blocked on my feedback, or @sun's, or you're just busy with something else, but +1 from me for the outlined plan :)

I got entangled with metadata :(

adding challenging tag for the focus board (+d8mi +challenging) to help new contributors jump in.
I'll update the issue summary: approach suggested.

Next steps, this is available for someone to pick up and make an initial patch.

Updated issue summary to make easier for people to jump in and join this issue, encorporate the clarification of the proposal, and use the issue summary template

fixed html close list tag

Here's a start. It's not working or anything but it's a start.

Some random observations:

+++ b/core/modules/language/lib/Drupal/language/Plugin/NegotiationInterface.php
@@ -0,0 +1,9 @@
+namespace Drupal\language\Plugin;

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.

+++ b/core/modules/language/lib/Drupal/language/Plugin/NegotiationInterface.php
@@ -0,0 +1,9 @@
+interface NegotiationInterface {
+  function negotiation($languages);
+  function languageSwitch();
+  function urlRewrite();
+}

I am not sure about the methode names here: while those are good as keys to identify the callbacks, I guess we should have something more self-documenting here, like:

<?php
interface NegotiationMethodInterface {
  // We should probably always deal with full objects and skip language codes everywhere...
  function getLanguage();
  function getLanguageSwitchLinks($type, $path);
  function alterUrl(&$path, &$options);
}
?>

+++ b/core/includes/language.inc
@@ -461,6 +461,9 @@ function language_negotiation_method_invoke($method_id, $method = NULL, $request
+    $languageManager = $container->get('language_manager');
+    $plugin = $languageManager->createInstance($method['id'], $container->get('request'), $container->getParameter('language.negotiation'));

Did you mean NegotiationManager here?

+++ b/core/modules/language/lib/Drupal/language/Plugin/NegotiationBase.php
@@ -0,0 +1,65 @@
+abstract class NegotiationBase implements NegotiationInterface {

We spent quite some time trying to standardize on a single terminology for this stuff, I think we should rename this to NegotiationMethodBase.

+++ b/core/modules/language/lib/Drupal/language/Plugin/NegotiationBase.php
@@ -0,0 +1,65 @@
+    $this->config = $config;

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.

+++ b/core/modules/language/lib/Drupal/language/Plugin/NegotiationBase.php
@@ -0,0 +1,65 @@
+  abstract function negotiation($languages);

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.

+++ b/core/modules/language/lib/Drupal/language/Plugin/NegotiationBase.php
@@ -0,0 +1,65 @@
+    throw new \Exception('Not implemented');
...
+    throw new \Exception('Not implemented');

Are these supposed to throw NegotiationNotImplementedException?

+++ b/core/modules/language/lib/Drupal/language/Plugin/NegotiationBase.php
@@ -0,0 +1,65 @@
+  protected function getLanguage($type) {
+    return $this->languageManager->getLanguage($type)->langcode;
+  }

I'd remove this method and just access the language manager directly if needed.

+++ b/core/modules/language/lib/Drupal/language/Plugin/NegotiationManager.php
@@ -0,0 +1,23 @@
+    $this->discovery = new AnnotatedClassDiscovery('language', 'negotiation');

Why lower case?

+++ b/core/modules/language/lib/Drupal/language/Plugin/language/negotiation/Browser.php
@@ -0,0 +1,117 @@
+class Browser extends NegotiationBase {

Not sure whether this would end up being too much verbose, but perhaps BrowserNegotationMethod (et al.) would be more self-documenting.

+++ b/core/modules/language/lib/Drupal/language/Plugin/language/negotiation/Session.php
@@ -0,0 +1,35 @@
+class User extends NegotiationBase {

+++ b/core/modules/language/lib/Drupal/language/Plugin/language/negotiation/UrlFallback.php
@@ -0,0 +1,36 @@
+class UrlFallback extends NegotiationBase {

+++ b/core/modules/language/lib/Drupal/language/Plugin/language/negotiation/User.php
@@ -0,0 +1,31 @@
+ */
+class User extends NegotiationBase {

Class/filename/PHPDoc mismatch here :)

+++ b/index.php
@@ -32,11 +32,12 @@
+// Create a request object from the HTTPFoundation.
+$request = Request::createFromGlobals();
 $kernel->boot();
+drupal_container()->enterScope('request');
+drupal_container()->set('request', $request, 'request');

I think I understaand why this is needed becuase I saw t() having troubles during an upgrade from a multilingual D7 installation to a D8 one, but would you please clarify what this is actually fixing?

Suppose this issue duplicates #1754246: Languages should be configuration entities
The only question here is how to make entity system accessible at early boostrap and installer

@andypost: please take a look at the actual patch, not just the issue title :) Retitled to avoid future misunderstandings.

From the top of my head proposed code looks cleaner, but we need to check with @Rob Loach who was behind the initial conversion. I'm not really familiar with the new language() function.

Fails likely not the problem of this patch/issue
It's a misbehaving testbot #1891598: 983 is misbehaving (failed to create checkout database)
#983

Getting this in the testbot watchdog log:

Failed to send result: Parse error. Request not well formed.

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.

Also seeing this in the apache error log:

Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest640225cache_config' doesn't exist: SELECT cid, data, created, expire, serialized, tags, checksum_invalidations, checksum_deletions FROM {cache_config} WHERE cid IN (:cids_0); Array\n(\n [:cids_0] => system.site\n)

SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest640225cache_path' doesn't exist: SELECT cid, data, created, expire, serialized, tags, checksum_invalidations, checksum_deletions FROM {cache_path} WHERE cid IN (:cids_0); Array\n(\n [:cids_0] => system-test/sleep/10\n)\n)

There are efforts underway (#500866: [META] remove t() from assert message) to remove t() from tests, for this exact reason. An actual unit test should definitely not have t() in it in the first place.

I know, doesn't help much now but will eventually. :-)

While I can think of hypothetical cases, I'm happy to move forward as-is and deal with them if/when they arise in practice. (I think they're situations we don't handle right now anyway.)

@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.

@katbailey: yeah I probably did not look at the patches close enough. I was afraid @chx's work would be lost. Maybe my reading of #21 was not clear then :) Thanks!

#1899994: Disentangle language initialization from path resolution is now committed, so should we go back to @chx's suggested changes?

Tagging for myself, as Kat asked for general cleanliness feedback. And bumping for the language geeks. :-)

@Crell:

I'll try to review this ASAP. I'd be glad to see your feedback on #43 :)

Also we have #1919002: Upgrade to D8 broken when D7 has more then one language enabled that points that current tests are not enough

Certainly this is more approachable than the old code. I actually sort of understand it. :-) I still defer to the language gurus to review the actual business logic.

Comments on general code flow, architecture, etc. below:

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -111,6 +112,65 @@ public function getLanguage($type) {
 
   /**
+   * Adds a language negotiator object to the array of negotiators.
+   */
+  public function addNegotiator($method_id, $negotiator) {
+    $this->negotiators[$method_id] = $negotiator;
+  }

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.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -111,6 +112,65 @@ public function getLanguage($type) {
+      // Skip negotiation methods that have not been registered or that are not
+      // appropriate for this type.
+      if (!isset($this->negotiators[$method_id]) || (isset($method['types']) && !in_array($type, $method['types']))) {
+        continue;

Wha? Isn't that what getNegotiationForType() should be doing? We shouldn't get back negotiators that don't apply. Refiltering that list shouldn't be our job here.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -111,6 +112,65 @@ public function getLanguage($type) {
+    global $user;

*cries*

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -165,4 +225,15 @@ protected function getLanguageDefault() {
+  protected function getLanguageList() {
+    return language_list();

This should be the other way around. Make this the real method, and the function should be a dumb wrapper for this object and method call.

+++ b/core/lib/Drupal/Core/Language/LanguageNegotiation.php
@@ -0,0 +1,20 @@
+final class LanguageNegotiation {
+
+  /**
+   * No language negotiation. The default language is used.
+   */
+  const LANGUAGE_NEGOTIATION_SELECTED = 'language-selected';
+

Why is this constant not just on the LanguageManager?

And even if it has to stay here, no need to repeat the class name in the constant name.

+++ b/core/lib/Drupal/Core/Language/LanguageNegotiationInterface.php
@@ -0,0 +1,22 @@
+interface LanguageNegotiationInterface {
+
+  public function getTypes();
+
+  /**
+   * Performs language negotiation.
+   */
+  public function negotiateLanguage($languages = array(), $request = NULL);
+

Needs docblocks.

+++ b/core/modules/language/lib/Drupal/language/LanguageNegotiation.php
@@ -0,0 +1,59 @@
+final class LanguageNegotiation {
+  /**
+   * The language is determined using path prefix or domain.
+   */
+  const LANGUAGE_NEGOTIATION_URL = 'language-url';
+
+  /**
+   * The language is set based on the browser language settings.
+   */
+  const LANGUAGE_NEGOTIATION_BROWSER = 'language-browser';
+
+  /**
+   * The language is determined using the current interface language.
+   */
+  const LANGUAGE_NEGOTIATION_INTERFACE = 'language-interface';
+
+  /**
+   * If no URL language, language is determined using an already detected one.
+   */
+  const LANGUAGE_NEGOTIATION_URL_FALLBACK = 'language-url-fallback';
+
+  /**
+   * The language is set based on the user language settings.
+   */
+  const LANGUAGE_NEGOTIATION_USER = 'language-user';
+
+  /**
+   * The language is set based on the user admin language settings.
+   */
+  const LANGUAGE_NEGOTIATION_USER_ADMIN = 'language-user-admin';
+
+  /**
+   * The language is set based on the request/session parameters.
+   */
+  const LANGUAGE_NEGOTIATION_SESSION = 'language-session';
+
+  /**
+   * URL language negotiation: use the path prefix as URL language indicator.
+   */
+  const LANGUAGE_NEGOTIATION_URL_PREFIX = 'path_prefix';
+
+  /**
+   * URL language negotiation: use the domain as URL language indicator.
+   */
+  const LANGUAGE_NEGOTIATION_URL_DOMAIN = 'domain';
+

*cries* again.

See above regarding constants.

+++ b/core/modules/language/lib/Drupal/language/LanguageNegotiationBrowser.php
@@ -0,0 +1,149 @@
+  public function negotiateLanguage($languages = array(), $request = NULL) {

$request should be type-hinted.

Also, reading through the code of this method... *facepalm*. Dear god the web is messed up. :-)

+++ b/core/modules/language/lib/Drupal/language/LanguageNegotiationUI.php
@@ -0,0 +1,38 @@
+    return $this->languageManager->getLanguage(LANGUAGE_TYPE_INTERFACE)->langcode;

Why is LANGUAGE_TYPE_INTERFACE still a global constant rather than a class constant?

+++ b/core/modules/language/lib/Drupal/language/LanguageNegotiationUrl.php
@@ -0,0 +1,91 @@
+            // Rebuild $path with the language removed.
+            _language_resolved_path(implode('/', $path_args));

Can we not inline this function, or turn it into a method?

+++ b/core/modules/language/lib/Drupal/language/LanguageNegotiationUrl.php
@@ -0,0 +1,91 @@
+        // Get only the host, not the port.
+        $http_host= $_SERVER['HTTP_HOST'];

Don't use $_SERVER. Use the $request object.

+++ b/core/modules/language/lib/Drupal/language/LanguageNegotiationUrlFallback.php
@@ -0,0 +1,74 @@
+  /**
+   * Overrides Drupal\Core\Language\LanguageNegotiationInterface::negotiateLanguage().
+   */
+  public function negotiateLanguage($languages = array(), $request = NULL) {

Strictly speaking this isn't an Override. It's an Implements. An interface has nothing to override, by definition.

I couldn't review #43, just wanted to mention that we have an issue to move all the constants in the language manager, although it seems some of them would be better fit into the LanguageNegotiation class: #1620010: Move LANGUAGE constants to the Language class.

Not sure we want to merge it here since it has the potential to make this patch huge.

@katbailey:

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.

@katbailey:

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.

I have a working branch implementing #43 + #7. Still cleaning-up stuff, tomorrow I should be able to post a patch.

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.

*** Didn't read every comment ***

+++ b/core/lib/Drupal/Core/Plugin/Core/LanguageNegotiation/LanguageNegotiationUI.phpundefined
@@ -0,0 +1,40 @@
+
+/**
+ * Class for identifying the language from the current interface language.
+ *
+ * @Plugin(
+ *   id = LanguageNegotiationUI::METHOD_ID,
+ *   types = {LANGUAGE_TYPE_CONTENT},
+ *   weight = 9,
+ *   name = @Translation("Interface"),
+ *   description = @Translation("Use the detected interface language.")
+ * )
+ */
+class LanguageNegotiationUI extends LanguageNegotiationMethodBase {
+
+  /**
+   * The language negotiation method id.
+   */
+  const METHOD_ID = 'language-interface';

LanguageNegotiationUI::METHOD_ID

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)

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.

Than I don't know why there is "METHOD_ID" in the plugin id.
Just looks a bit strange...

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.

hmm

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.

:s

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.

Adding to the D8MI focus issues.

Not really resistance, just giving my opinion.

Re-rolling...

+++ b/core/includes/bootstrap.inc
@@ -2918,33 +2849,8 @@ function language_is_locked($langcode) {
-  $info = variable_get('language_default', array(
-    'langcode' => 'en',
-    'name' => 'English',
-    'direction' => 0,
-    'weight' => 0,
-    'locked' => 0,
-  ));
-  $info['default'] = TRUE;
-  return new Language($info);
...
+  $default_info = variable_get('language_default', Language::$DEFAULT_VALUES);
+  return new Language($default_info + array('default' => TRUE));

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.

+++ b/core/includes/install.core.inc
@@ -334,6 +335,12 @@ function install_begin_request(&$install_state) {
+    $namespaces = array(
+      'Drupal\Core' => DRUPAL_ROOT . '/core/lib',
+      'Drupal\Component' => DRUPAL_ROOT . '/core/lib',
+    );
+    $container->setParameter('container.namespaces', $namespaces);

Doesn't this make it impossible for contrib to provide negotiation plugins?

Thanks for reviewing!

You're right on the first one, although I don't think it would actually happen in real life.

Doesn't this make it impossible for contrib to provide negotiation plugins?

I think that applies only to installer code. In the normal execution flow the regular namspeaces should be used.

D'uh, missed that little detail. :-) Thanks for the explanation.

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.

Sorry about #70. Updating plugin location and re-rolling with current code base.

Thanks, hope to get back to this as soon as I'm done with the ML node issue.

#72 was also cycling on the testbot, with 'Parse error. Request not well formed' responses. The Doctrine errors are gone; and the only thing that doesn't look familiar is a Plugin exception:

The plugin (aggregator_feed_block:1) did not specify an instance class." at /core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php line 62

That doesn't mean that the above is coming from the patch ... it could also simply be something that is appearing on all D8 test runs; I just haven't seen it before today.

Amazing work, here are some mostly nitpicks.

+++ b/core/includes/bootstrap.incundefined
@@ -2762,10 +2762,7 @@ function language_types_get_default() {
+  return drupal_container()->get('language_manager')->isMultilingual();

@@ -2843,24 +2791,7 @@ function language_list($flags = LANGUAGE_CONFIGURABLE) {
+  return drupal_container()->get('language_manager')->getDefaultLockedLanguages($weight);

Should be probably Drupal::service('language_manager')

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -29,6 +28,34 @@ class LanguageManager {
+   * The configuration factory service.
+   *
+   * @var array
+   */
+  protected $config;

I guess it's now just the configuration as an array, not the service anymore...

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -38,13 +65,15 @@ class LanguageManager {
+  public function __construct(array $config, PluginManagerInterface $negotiatorManager) {

Should have some documentation.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -86,31 +117,105 @@ public function getLanguage($type) {
+    $language->method_id = LanguageManager::METHOD_ID;
...
+      $cache = !isset($method['cache']) || $user->uid || $method['cache'] == variable_get('cache', 0);

I know this comes directly from the previous code, though shouldn't variable_get('cache') be now config('system.performance')->get('cache.page.use_internal')?

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -86,31 +117,105 @@ public function getLanguage($type) {
+  protected function negotiateLanguage($type, $method_id) {

Missing @return

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -154,15 +262,106 @@ protected function getLanguageTypes() {
+      if ($this->isMultilingual() || module_exists('language')) {

Should the ModuleHandler() be injected?

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -154,15 +262,106 @@ protected function getLanguageTypes() {
+        $this->languageList = db_query('SELECT * FROM {language} ORDER BY weight ASC, name ASC')->fetchAllAssoc('langcode', \PDO::FETCH_ASSOC);

Should the db connection be injected?

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -154,15 +262,106 @@ protected function getLanguageTypes() {
+      'weight' => ++$weight,
...
+      'weight' => ++$weight,

It's a bit odd to start with weight = 1, instead of weight = 0.

+++ b/core/lib/Drupal/Core/Language/LanguageNegotiationInterface.phpundefined
@@ -0,0 +1,37 @@
+ * Contains Drupal\Core\Language\LanguageNegotiationInterface.

Missing "\"

+++ b/core/lib/Drupal/Core/Language/LanguageNegotiationInterface.phpundefined
@@ -0,0 +1,37 @@
+   * @param $languages

Missing array()

+++ b/core/lib/Drupal/Core/Language/LanguageNegotiationMethodBase.phpundefined
@@ -0,0 +1,43 @@
+  public function __construct(array $config) {

+++ b/core/lib/Drupal/Core/Language/LanguageNegotiationMethodManager.phpundefined
@@ -0,0 +1,26 @@
+  public function __construct(array $namespaces) {

Missing documentation.

+++ b/core/lib/Drupal/Core/Plugin/Core/LanguageNegotiation/LanguageNegotiationBrowser.phpundefined
@@ -0,0 +1,150 @@
+ *   id = LanguageNegotiationBrowser::METHOD_ID,
...
+  /**
+   * The language negotiation method id.
+   */
+  const METHOD_ID = 'language-browser';

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

+++ b/core/lib/Drupal/Core/Plugin/Core/LanguageNegotiation/LanguageNegotiationUrlFallback.phpundefined
@@ -0,0 +1,72 @@
+ * Class that determines the language to be assigned to URLs when none is detected.

> 80 chars :(

+++ b/core/lib/Drupal/Core/Plugin/Core/LanguageNegotiation/LanguageNegotiationUserAdmin.phpundefined
@@ -0,0 +1,50 @@
+ * Definition of \Drupal\Core\Plugin\Core\LanguageNegotiation\LanguageNegotiationUserAdmin.

Should be Contains.

+++ b/core/modules/language/lib/Drupal/language/HttpKernel/PathProcessorLanguage.phpundefined
@@ -25,11 +26,12 @@ class PathProcessorLanguage implements InboundPathProcessorInterface {
    * Constructs a PathProcessorLanguage object.
@@ -41,31 +43,33 @@ class PathProcessorLanguage implements InboundPathProcessorInterface {

@@ -41,31 +43,33 @@ class PathProcessorLanguage implements InboundPathProcessorInterface {
    *   An array of languages, keyed by language code, representing the languages
    *   currently enabled on the site.
    */
...
+  public function __construct(LanguageManager $language_manager, ConfigFactory $config) {

The docs needs some adaption.

+++ b/core/modules/language/lib/Drupal/language/HttpKernel/PathProcessorLanguage.phpundefined
@@ -41,31 +43,33 @@ class PathProcessorLanguage implements InboundPathProcessorInterface {
     $this->config = $config;

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.

+++ b/core/modules/language/tests/language_test/lib/Drupal/language_test/LanguageTestLanguageNegotiation.phpundefined
@@ -0,0 +1,31 @@
+ * Definition of Drupal\language\LanguageTestLanguageNegotiation.

Contains ...

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.

Additionally added missing method in LanguageTestLanguageNegotiation::setLanguageManager.

Yes, the DB connection should always be injected if possible. Although if this issue is now using plugins the "if possible" is kinda iffy still. :-(

+++ b/core/modules/language/tests/language_test/lib/Drupal/language_test/LanguageTestLanguageNegotiation.php
@@ -22,10 +22,17 @@ public function getTypes() {
-  public function negotiateLanguage($languages = array(), $request = NULL) {
+  public function setLanguageManager(\Drupal\Core\Language\LanguageManager $languageManager) {
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function negotiateLanguage(array $languages = array(), \Symfony\Component\HttpFoundation\Request $request = NULL) {
     return 'it';
   }

The class names here should be "use"d and then just listed here by their short name.

Thanks for the review @Crell. Seems the patch doing some crazy stuff to testbot. Check the attachment.

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.

something similar with the tests for #1813762-50: Introduce unified interfaces, use dependency injection for interface translation

Re-roll of #79.

i will give this a shot during the weekend

Thanks! We probably want to #2020249: Create Drupal::languageManager for improved DX before this one lands.

here is a straight reroll, so that i can interdiff

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?

i think i should wait for #1903346: Establish a new DefaultPluginManager to encapsulate best practices

@ParisLiakos:

Thanks, I'll review the changes tonight.

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.

Btw, I was playing with the idea of defining a container-backed cache backend to cache the plugin definitions. The reason would be avoiding cache queries and in general any dependency on the DB.

Changes look good, only one thing:

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -300,7 +300,7 @@ function getLanguageList($flags = LANGUAGE_CONFIGURABLE) {
+      if ($this->isMultilingual() || \Drupal::moduleHandler()->moduleExists('language')) {

I know this was already there, but we should inject the module handler, I guess. Or better we should find a way to avoid needing to know anything about the Language module, if possible :)

yeah i would rather we do the second as well
back on track now that #1903346: Establish a new DefaultPluginManager to encapsulate best practices is in

Paris is working on this, so back on sprint.

@Paris:

Rerolled #94 and pushed the changes.

I was working on this in https://drupal.org/sandbox/plach/1719670, branch 8.x-language-oo-1862202-plach. I granted you access just in case you wished to keep working there.

oh, thats great, sandbox rocks! thanks! i am flying in a bit, will get back to this first thing in Dublin;) (and/or during the flight)
thanks for the reroll!

Awesome! See you there soon :)

Is there a reason this wasn't sent to testbot? It will get picked up the next time it's set to needs review anyway :)

We should postpone #1754246: Languages should be configuration entities and #1856976: Convert language_count to the state system. on this!
It much easy to attach config storage for language on top of this conversion

Overall this is a awesome clean-up! But some tasks still badly implemented...

Most visible is the injection of services to language manager
- strong dependency moduleHandler - to check that language module enabled (no module handler - only default language?)
- storage (optional) to load languages or mock/fallback to get default list (maybe better implement separate managers for install and runtime?)
- config/state services (optional) to replace all variable_get|set()

Also global $user in some plugins

+++ b/core/includes/install.core.incundefined
@@ -383,7 +392,11 @@ function install_begin_request(&$install_state) {
+    $container->register('language_manager', 'Drupal\Core\Language\LanguageManager')
+      ->addArgument(array())

Language manager initialized with empty config

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -125,4 +127,22 @@ public static function registerTwig(ContainerBuilder $container) {
+    $config = $storage->read('language.negotiation') ?: array();
+    $config += array('browser' => array('mappings' => $storage->read('language.mappings')));
...
+    $container->register('language_manager', 'Drupal\Core\Language\LanguageManager')
+      ->addArgument($config)

I'd suggest to pass here $active_config_storage and memory mock for install time

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -439,6 +439,8 @@ protected function buildContainer() {
+    // Register the kernel-level config storage.
+    $container->set('kernel.config.storage', $this->configStorage);

+1

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -87,31 +127,108 @@ public function getLanguage($type = Language::TYPE_INTERFACE) {
+    return array_keys(variable_get("language_negotiation_$type", array()));
...
+    $languages = $this->getLanguageList();
+    $method = $this->negotiatorManager->getDefinition($method_id);
...
+      $cache = !isset($method['cache']) || $user->uid || $method['cache'] == config('system.performance')->get('cache.page.use_internal');
+      if ($cache) {
+        $negotiator = $this->negotiatorManager->createInstance($method_id, $this->config);
+        $negotiator->setLanguageManager($this);
+        $langcode = $negotiator->negotiateLanguage($languages, $this->request);

@@ -136,13 +253,16 @@ public function reset($type = NULL) {
     return variable_get('language_count', 1) > 1;

@@ -155,18 +275,109 @@ protected function getLanguageTypes() {
+    $default_info = variable_get('language_default', Language::$DEFAULT_VALUES);

this needs @todo url to replace

variables should be moved to config too.
you should use injected config storage to replace config()

I have trying to implement this in #1856976: Convert language_count to the state system.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -155,18 +275,109 @@ protected function getLanguageTypes() {
+      if ($this->isMultilingual() || \Drupal::moduleHandler()->moduleExists('language')) {
...
+        $this->languageList = db_query('SELECT * FROM {language} ORDER BY weight ASC, name ASC')->fetchAllAssoc('langcode', \PDO::FETCH_ASSOC);

both module handler and storage needs to be injected (optionally?)

This makes me use config_get_storage_names_with_prefix() in #1754246: Languages should be configuration entities - patch is green

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -155,18 +275,109 @@ protected function getLanguageTypes() {
+      $default->name = t("Site's default language (@lang_name)", array('@lang_name' => $default->name));

t() needs injection too

+++ b/core/lib/Drupal/Core/Language/Plugin/LanguageNegotiation/LanguageNegotiationSession.phpundefined
@@ -0,0 +1,58 @@
+      global $user;
+      $languages = $this->languageManager->getLanguageList();
+      if ($user->uid && isset($languages[$langcode])) {
+        $_SESSION[$param] = $langcode;

is it possible to replace Global $user?

+++ b/core/lib/Drupal/Core/Language/Plugin/LanguageNegotiation/LanguageNegotiationUrl.phpundefined
@@ -0,0 +1,100 @@
+        if (strpos($http_host, ':') !== FALSE) {
+          $http_host_tmp = explode(':', $http_host);
+          $http_host = current($http_host_tmp);
+        }
+        $domains = $this->config['url']['domains'];
+        foreach ($languages as $language) {
+          // Skip the check if the language doesn't have a domain.
+          if (!empty($domains[$language->langcode])) {
+            // Ensure that there is exactly one protocol in the URL when
+            // checking the hostname.
+            $host = 'http://' . str_replace(array('http://', 'https://'), '', $domains[$language->langcode]);
+            $host = parse_url($host, PHP_URL_HOST);

maybe there's some useful code for that in core\libs?

+++ b/core/lib/Drupal/Core/Language/Plugin/LanguageNegotiation/LanguageNegotiationUserAdmin.phpundefined
@@ -0,0 +1,51 @@
+    // User preference (only for authenticated users).
+    global $user;
...
+    if ($user->uid && !empty($user->preferred_admin_langcode) && isset($languages[$user->preferred_admin_langcode]) && path_is_admin($request_path)) {
+      return $user->preferred_admin_langcode;

suppose better use accountInterface here, or session (still in progress?)

+++ b/core/modules/language/lib/Drupal/language/HttpKernel/PathProcessorLanguage.phpundefined
@@ -42,51 +42,44 @@ class PathProcessorLanguage implements InboundPathProcessorInterface, OutboundPa
+    $languages = $this->languageManager->getLanguageList();
+    $prefixes = $this->config->get('language.negotiation')->get('url.prefixes');

Maybe introduce languageManager()->setStorage() method?

@andypost:

I granted you access to the sandbox if you wish to help with this too. Paris is actively working on it right now.

Lets leave the $user global out of this. If things go well in #1890878: Add modular authentication system, including Http Basic; deprecate global $user plugins will be able to pull it out from the request.

t() needs injection too

This should also wait unless really necessary for #2018411: Figure out a nice DX when working with injected translation

I added @todo for variable_conversions
Removed a few test classes not being used
Splitted the module_exists check to another language manager provided by language module.

I cant move at all with tests..i just cant reproduce simpletest's failures, debugging html prints output twice ending with a twig error from UI...from cli debug html pages appear normally, but failures rise a lot..

Let's see that tests, related to variables #1833022: Only display interface language detection options to customize more granularity

i am probably just wasting time..maybe its easier for someone else

Ok....

Spent some time on this and found and fixed two problems.

- 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?!

Possibly more tomorrow, we'll see.

All over core drupal_static_reset('language_list'); now does nothing, so trying to clean-up its usage

+++ b/core/includes/bootstrap.incundefined
@@ -2465,56 +2462,7 @@ function language_multilingual() {
 function language_list($flags = Language::STATE_CONFIGURABLE) {
-
-  $languages = &drupal_static(__FUNCTION__);
...
+  return Drupal::languageManager()->getLanguageList($flags);

So all static reset should be replaced

+++ b/core/modules/language/language.admin.incundefined
@@ -255,6 +255,7 @@ function language_admin_add_form_submit($form, &$form_state) {
+  drupal_rebuild_language_negotiation_settings();

@@ -559,6 +560,9 @@ function language_negotiation_configure_form_submit($form, &$form_state) {
+  // Rebuild the container to update the submitted settings.
+  drupal_rebuild_language_negotiation_settings();

@@ -743,6 +747,8 @@ function language_negotiation_configure_browser_form_submit($form, &$form_state)
+  // Rebuild the container to update the submitted settings.
+  drupal_rebuild_language_negotiation_settings();

+++ b/core/modules/language/language.moduleundefined
@@ -503,8 +503,8 @@ function language_save($language) {
-  // Kill the static cache in language_list().
-  drupal_static_reset('language_list');
+  // Reset the language information.
+  Drupal::languageManager()->reset();

@@ -847,4 +847,5 @@ function language_system_regional_settings_form_submit($form, &$form_state) {
   language_save($language);
+  drupal_rebuild_language_negotiation_settings();

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigurationElementTest.phpundefined
@@ -85,6 +85,7 @@ public function testDefaultLangcode() {
+    $this->resetAll();

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/CacheDecoratorLanguageTest.phpundefined
@@ -69,6 +69,7 @@ public function setUp() {
       variable_set('locale_custom_strings_' . $langcode, array('' => $custom_strings));
+      $this->rebuildContainer();

hard solution!
Suppose better to implement special method a-la resetLanguages() in language manager

As per #1754246-145: Languages should be configuration entities, this is a cleanup required to make languages injectable (eg. for the installer) and also make the new Drupal 8 language API independent of module functions. Some of the workarounds required for #1754246: Languages should be configuration entities can/should be cleaned up here.

Tagging.

#2039051: Convert hook_language_negotiation_info() to plugin system was marked as a duplicate of this, but it doesn't seem to actually do anything with that hook...

Probably this is not getting rid of hook_language_negotiation_info() yet but it already converts all the language negotiation methods to plugins. The patch is incomplete AFAIR.

Tagging for massive API change. I'm not 100% sure this is maintainer approved. Ideally we'd have written record.

This is issue a blocker for #1827038: Remove stale references to language_content_type variable - without this we can't resolve the circular dependency between language and config - and this will change the multilingual API.

If it's critical then it's guaranteed to get in. The only exception would be something wrongly classified as critical, hopefully those don't stay that way too long.

@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(?)

Second #119... tough re-rolls...

Grml.

Missed one merge conflict.

This should fix the installer, state needs to be optional.

Forgot the language.module language manager.

I need to stop posting untested patches, sorry for the spam. This should be better.

Fixing some unit tests and the user negotation plugin.

Most of those fails are I think related to a weird behavior with the php storage and simpletest.

Somehow, that handling isn't consistent:
Calling PhpStorageFactory::get('service_container')->deleteAll() inside a test deletes this directory:
DELETE: .../sites/default/files/simpletest/330378/php/service_container

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.

#1856766-50: Convert file_public_path to the new settings system (make it not configurable via UI or YAML) fixes it, because it converts the file public path to settings and that overrides it in a test environment much more consistently. Tried one of the failing tests and it passes with that applied. Might even make most of DrupalKernel::getClassName() unnecessary.

This patch combines the two, but this is now blocked on that getting in.

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 ;)

1. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -105,31 +137,109 @@ public function getLanguage($type = Language::TYPE_INTERFACE) {
   +      $cache = !isset($method['cache']) || $user->isAuthenticated() || $method['cache'] == config('system.performance')->get('cache.page.use_internal');
   

   deprecated, $this->config !?

2. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -177,16 +289,90 @@ protected function getLanguageTypes() {
   +     );
   ...
   +     'weight' => ++$weight,
   

   bad indent

3. +++ b/core/lib/Drupal/Core/Language/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php
   @@ -0,0 +1,150 @@
   + *   config = "admin/config/regional/language/detection/browser"
   
   +++ b/core/lib/Drupal/Core/Language/Plugin/LanguageNegotiation/LanguageNegotiationSelected.php
   @@ -0,0 +1,42 @@
   + *   config = "admin/config/regional/language/detection/selected"
   
   +++ b/core/lib/Drupal/Core/Language/Plugin/LanguageNegotiation/LanguageNegotiationSession.php
   @@ -0,0 +1,58 @@
   + *   config = "admin/config/regional/language/detection/session"
   
   +++ b/core/lib/Drupal/Core/Language/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
   @@ -0,0 +1,100 @@
   + *   config = "admin/config/regional/language/detection/url"
   

   using direct path in annotation is totally wrong

@Berdir:

And still a ton of crazy procedural code, and the old hook is still around.. me confused a bit ;)

Yep, the patch is still incomplete, I couldn't fix everything I would like to :(

@andypost:

using direct path in annotation is totally wrong

Can you please explain why?

@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?

@Berdir:

I'll be in IRC this evening after 21:00. Btw I've been working on this in http://drupalcode.org/sandbox/plach/1719670.git/tree/refs/heads/8.x-lang..., you should already have full access to the sandbox if you wish to go on there :)

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.

Let's see what I broke...

Ok, that's why language_default() doesn't use the language manager...

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.

Anyway, this how I would proceed with the OO-fication:

bootstrap.inc

• language_types_get_all() -> Move to the LM.
• language_types_get_default() -> Move to the LM.
• language_load() -> Move to the LM.

language.inc

• language_negotiation_get_switch_links() -> Move to the LM.
• language_fallback_get_candidates() -> Skip this, it is handled in #2019055: Switch from field-level language fallback to entity-level language fallback.
• language_from_selected() -> Already replaced by the LanguageNegotiationSelected plugin, DIE!
• 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.

Started moving more stuff, removed the negotiation info hook.

we could cache the language type definitions into state, which is available to the language manager.

So language manager depends only on state service!

Suppose we need to rename the issue to somehow #2084567: Implement a EntityLockedInterface and service to allow locking entities
I think better to store default language by the same and list of locked languages (probably EntityStatableInterface)

Always fun to re-roll this one...

Fixed a few easy test fails.

Most test fails are content translation UI test fails, something is off there, not exactly sure what...

Didn't check why (too late :) but tests are failing because the language_negotiation_language_content variable has an empty values instead of holding the language-interface method.

Here is a reroll

I try to re-roll this one.

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)

content translation is totally broken #1946462-52: Convert content_translation_translatable_form() to the new form interface and according @plach should be removed

so lets fix the annotations after #2090353: Don't require to put the use statement into plugin classes went in..we need to put the full namespace there, which kinda sucks

lets get rid of all those exceptions

Re-roll.

+++ b/core/includes/bootstrap.inc
@@ -2515,10 +2413,11 @@ function language_default_locked_languages($weight = 0) {
+ * @see \Druoal\Core\Language\LanguageManager::loadLanguage().

Typo. Mentioning it so it's not forgotten.

I understand we're blocked on #2097585: Allow plugin managers to register namespaces to the annotation reader?

Updated issue summary.

Updated issue summary.

Nope, just putting the full namespace in the annotation still works so we can do that for now

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.

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.