Updated: Comment #364

Suggested commit message:

Issue #1862202 by plach, Berdir, katbailey, ParisLiakos, alexpott, chx,  sun, larowlan, Gábor Hojtsy, cosmicdreams, vijaycs85, YesCT, penyaskito, andypost, Albert Volkman, joelpitett: Objectify the language system.

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:

<?php
 
public function getLanguage($type) {
    if (isset(
$this->languages[$type])) {
      return
$this->languages[$type];
    }

   
// @todo Objectify the language system so that we don't have to do this.
   
if (language_multilingual()) {
      include_once
DRUPAL_ROOT . '/core/includes/language.inc';
     
$this->languages[$type] = language_types_initialize($type, $this->request);
    }
    else {
     
$this->languages[$type] = language_default();
    }
    return
$this->languages[$type];
  }
?>

This has several drawbacks, as the current code:

  • 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.
  • 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 initializing per-language config overrides only after instantiating the language manager (this will no longer be necessary after #2098119: Replace config context system with baked-in locale support and single-event based overrides).
  • 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

  • Agree on an approach
  • Write a patch
  • Fix test failures
  • Reviews (enough? As of Jan 7, reviews were done by @Berdir @larowlan @joelpitett @sun)
  • Create follow-up issues Done Jan 3 2014
  • 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:
    bootstrap.inc
    language() LanguageManagerInterface::getCurrentLanguage()
    language_default() LanguageManagerInterface::getDefaultLanguage()
    language_list() LanguageManagerInterface::getLanguages()
    language_load() LanguageManagerInterface::getLanguage()
    language_default_locked_languages() LanguageManagerInterface::getDefaultLockedLanguages()
    language_types_get_default() LanguageManagerInterface::getLanguageTypes()
    language_name() LanguageManagerInterface::getLanguageName()
    language_is_locked() LanguageManagerInterface::isLanguageLocked()
    language.inc
    language_negotiation_get_switch_links() LanguageManagerInterface::getLanguageSwitchLinks()
    language_types_info() ConfigurableLanguageManagerInterface::getDefinedLanguageTypesInfo()
    language_types_get_configurable() ConfigurableLanguageManagerInterface::getLanguageTypes()
    language_types_disable() Removed, no actual use case for this.
    language_update_locked_weights() ConfigurableLanguageManagerInterface::updateLockedLanguageWeights()
    language_types_get_all() ConfigurableLanguageManagerInterface::getDefinedLanguageTypes()
    language_types_set() LanguageNegotiatorInterface::updateConfiguration()
    language_types_initialize() LanguageNegotiatorInterface::initializeType()
    language_negotiation_method_get_first() LanguageNegotiatorInterface::getPrimaryNegotiationMethod()
    language_negotiation_method_enabled() LanguageNegotiatorInterface::isNegotiationMethodEnabled()
    language_negotiation_purge() LanguageNegotiatorInterface::purgeConfiguration()
    language_negotiation_set() LanguageNegotiatorInterface::saveConfiguration()
    language_negotiation_info() LanguageNegotiatorInterface::getNegotiationMethods()
    language_negotiation_method_invoke() LanguageNegotiator::negotiateLanguage() (protected)
    language_url_split_prefix() LanguageNegotiationUrl::processInbound()
    language.negotiation.inc
    language_from_selected() LanguageNegotiationSelected::getLangcode()
    language_from_browser() LanguageNegotiationBrowser::getLangcode()
    language_from_user() LanguageNegotiationUser::getLangcode()
    language_from_user_admin() LanguageNegotiationUserAdmin::getLangcode()
    language_from_session() LanguageNegotiationSession::getLangcode()
    language_from_url() LanguageNegotiationUrl::getLangcode()
    language_url_fallback() LanguageNegotiationUrlFallback::getLangcode()
    language_switcher_session() LanguageNegotiationSession::getLanguageSwitchLinks()
    language_switcher_url() LanguageNegotiationUrl::getLanguageSwitchLinks()
    language_url_rewrite_session() LanguageNegotiationSession::processOutbound()

Related

Follow ups

bugs Caused

architecture follow-ups

DX follow-up

some coding-style issues

Files: 
CommentFileSizeAuthor
#372 language-oo-1862202-372.patch366.39 KBplach
PASSED: [[SimpleTest]]: [MySQL] 59,884 pass(es).
[ View ]

Comments

beejeebus’s picture

yes please. this is on my list, depending on how the config stuff goes.

catch’s picture

YesCT’s picture

Issue tags:+D8MI

d8mi tag?

Gábor Hojtsy’s picture

Issue tags:+language-base

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

plach’s picture

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

chx’s picture

Assigned:Unassigned» chx
chx’s picture

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!

sun’s picture

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

plach’s picture

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

chx’s picture

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.

chx’s picture

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

plach’s picture

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

chx’s picture

I got entangled with metadata :(

YesCT’s picture

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.

YesCT’s picture

Issue summary:View changes

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

YesCT’s picture

Issue summary:View changes

fixed html close list tag

chx’s picture

Status:Active» Needs review
Issue tags:-needs initial patch
StatusFileSize
new16.14 KB

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

plach’s picture

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?

andypost’s picture

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

Gábor Hojtsy’s picture

Title:Objectify the language system» Objectify the language negotiation system

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

katbailey’s picture

StatusFileSize
new9.08 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

plach’s picture

Assigned:Unassigned» chx

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.

chx’s picture

Assigned:chx» Unassigned
katbailey’s picture

Assigned:chx» Unassigned
Status:Needs work» Needs review
StatusFileSize
new10.61 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Fixed the installer so we should at least get a fail count this time...

@plach actually I wrote the LanguageManager class originally, along with the horrible language() function - that was all part of the bundles patch: #1599108: Allow modules to register services and subscriber services (events), and actually I just found a comment there from sun where he says

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

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new10.61 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

I have no idea what's going wrong with that patch - try this one?

YesCT’s picture

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

jthorson’s picture

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)

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new848 bytes
new10.95 KB
FAILED: [[SimpleTest]]: [MySQL] 50,544 pass(es), 5 fail(s), and 31 exception(s).
[ View ]

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.

Crell’s picture

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

katbailey’s picture

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.

Crell’s picture

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

Gábor Hojtsy’s picture

@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’s picture

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

+++ b/index.phpundefined
@@ -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');
drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);

-// Create a request object from the HTTPFoundation.
-$request = Request::createFromGlobals();

This was the red flag for me.

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

Gábor Hojtsy’s picture

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

katbailey’s picture

As it turns out, the work I started here to make the language manager independent of request scope is also needed in order to move forward with #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence, so I've decided to break it off into a separate issue: #1899994: Disentangle language initialization from path resolution

Gábor Hojtsy’s picture

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

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new54.22 KB

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

Anyway, of course this breaks the installer, because... https://twitter.com/Crell/status/298871064795168768. So there's no point letting testbot at it until I figure out how to get around that.

But my main concern at the moment is that this might be over-engineered and "clunky" compared with the existing procedural code - language negotiation experts, please weigh in!

Crell’s picture

Issue tags:+Stalking Crell

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

plach’s picture

@Crell:

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

andypost’s picture

Issue tags:+Needs tests

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

Crell’s picture

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.

plach’s picture

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.

plach’s picture

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

Crell’s picture

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.

plach’s picture

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’s picture

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.

The issue dealing with injecting services into plugins is #1863816: Allow plugins to have services injected - looks like yched has put a lot of work into it so hopefully that will be resolved soon.

So I guess my fear of the approach being too clunky was well-founded - thanks for the feedback @plach, I look forward to seeing what you come up with!

plach’s picture

Assigned:Unassigned» plach

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

plach’s picture

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

plach’s picture

StatusFileSize
new78.83 KB
FAILED: [[SimpleTest]]: [MySQL] 51,275 pass(es), 231 fail(s), and 371 exception(s).
[ View ]

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.

aspilicious’s picture

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

plach’s picture

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.

aspilicious’s picture

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

plach’s picture

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.

aspilicious’s picture

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

plach’s picture

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.

plach’s picture

Issue tags:+sprint

Adding to the D8MI focus issues.

aspilicious’s picture

Not really resistance, just giving my opinion.

vijaycs85’s picture

StatusFileSize
new79 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Re-rolling...

tstoeckler’s picture

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

plach’s picture

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.

tstoeckler’s picture

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

jthorson’s picture

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.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new77.84 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

plach’s picture

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

jthorson’s picture

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

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new77.44 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

dawehner’s picture

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

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new9.82 KB
new78.68 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

Crell’s picture

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.

vijaycs85’s picture

StatusFileSize
new131.59 KB

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

chx’s picture

Status:Needs work» Needs review

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.

YesCT’s picture

Albert Volkman’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new77.31 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Re-roll of #79.

ParisLiakos’s picture

Assigned:Unassigned» ParisLiakos

i will give this a shot during the weekend

plach’s picture

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

ParisLiakos’s picture

StatusFileSize
new76.05 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

here is a straight reroll, so that i can interdiff

ParisLiakos’s picture

StatusFileSize
new22.79 KB
new76.69 KB
FAILED: [[SimpleTest]]: [MySQL] 56,561 pass(es), 351 fail(s), and 79,183 exception(s).
[ View ]

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?

ParisLiakos’s picture

Status:Needs work» Needs review
plach’s picture

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

plach’s picture

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.

plach’s picture

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

ParisLiakos’s picture

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

plach’s picture

Issue tags:+sprint

Paris is working on this, so back on sprint.

plach’s picture

StatusFileSize
new76.65 KB
FAILED: [[SimpleTest]]: [MySQL] 57,047 pass(es), 344 fail(s), and 78,929 exception(s).
[ View ]

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

ParisLiakos’s picture

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!

plach’s picture

Awesome! See you there soon :)

Berdir’s picture

Status:Needs work» Needs review

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

andypost’s picture

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

andypost’s picture

Issue tags:+API clean-up

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?

plach’s picture

@andypost:

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

ParisLiakos’s picture

StatusFileSize
new79.31 KB
FAILED: [[SimpleTest]]: [MySQL] 57,120 pass(es), 346 fail(s), and 78,882 exception(s).
[ View ]

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

andypost’s picture

Status:Needs work» Needs review
ParisLiakos’s picture

Assigned:ParisLiakos» Unassigned
Status:Needs review» Needs work
Issue tags:+PHPUnit Blocker

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

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new1.99 KB
new80.3 KB
FAILED: [[SimpleTest]]: [MySQL] 57,865 pass(es), 82 fail(s), and 28 exception(s).
[ View ]

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.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new93.39 KB
FAILED: [[SimpleTest]]: [MySQL] 57,795 pass(es), 72 fail(s), and 5 exception(s).
[ View ]
new15.03 KB

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

Gábor Hojtsy’s picture

Priority:Normal» Critical

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.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new92.96 KB
FAILED: [[SimpleTest]]: [MySQL] 56,543 pass(es), 120 fail(s), and 91 exception(s).
[ View ]

Here's a reroll, possibly with more breakage - it was a gnarly reroll.

tim.plunkett’s picture

Issue tags:+Plugin-conversion

Tagging.

tim.plunkett’s picture

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

plach’s picture

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.

Gábor Hojtsy’s picture

Issue tags:+API change

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

alexpott’s picture

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.

catch’s picture

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.

Gábor Hojtsy’s picture

Issue tags:+API change

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

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new93.27 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/install.core.inc.
[ View ]

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

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new697 bytes
new93.27 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/language/lib/Drupal/language/Tests/LanguageListTest.php.
[ View ]

Grml.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new1.04 KB
new93.24 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Missed one merge conflict.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new2.85 KB
new93.21 KB
FAILED: [[SimpleTest]]: [MySQL] 51,590 pass(es), 4,007 fail(s), and 5,974 exception(s).
[ View ]

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

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new2.51 KB
new93.29 KB
FAILED: [[SimpleTest]]: [MySQL] 46,986 pass(es), 134 fail(s), and 334 exception(s).
[ View ]

Forgot the language.module language manager.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new1.88 KB
new93.29 KB
FAILED: [[SimpleTest]]: [MySQL] 57,789 pass(es), 142 fail(s), and 90 exception(s).
[ View ]

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

Berdir’s picture

Status:Needs work» Needs review
Issue tags:-challenging
StatusFileSize
new4.79 KB
new95.3 KB
FAILED: [[SimpleTest]]: [MySQL] 57,794 pass(es), 121 fail(s), and 85 exception(s).
[ View ]

Fixing some unit tests and the user negotation plugin.

Berdir’s picture

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?

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new120.69 KB
FAILED: [[SimpleTest]]: [MySQL] 57,972 pass(es), 116 fail(s), and 113 exception(s).
[ View ]

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.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new7.71 KB
new126.75 KB
FAILED: [[SimpleTest]]: [MySQL] 58,294 pass(es), 26 fail(s), and 27 exception(s).
[ View ]

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’s picture

  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

plach’s picture

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

Berdir’s picture

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

plach’s picture

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

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new18.39 KB
new128.88 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new507 bytes
new128.97 KB
FAILED: [[SimpleTest]]: [MySQL] 58,013 pass(es), 37 fail(s), and 27 exception(s).
[ View ]

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

plach’s picture

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

Berdir’s picture

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

plach’s picture

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.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new11.64 KB
new122.34 KB
FAILED: [[SimpleTest]]: [MySQL] 57,977 pass(es), 220 fail(s), and 5 exception(s).
[ View ]
Berdir’s picture

Started moving more stuff, removed the negotiation info hook.

andypost’s picture

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)

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new126.24 KB
FAILED: [[SimpleTest]]: [MySQL] 58,777 pass(es), 229 fail(s), and 5 exception(s).
[ View ]

Always fun to re-roll this one...

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new2.2 KB
new128.48 KB
FAILED: [[SimpleTest]]: [MySQL] 58,866 pass(es), 223 fail(s), and 6 exception(s).
[ View ]

Fixed a few easy test fails.

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

plach’s picture

Status:Needs work» Needs review

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.

plach’s picture

StatusFileSize
new128.48 KB
FAILED: [[SimpleTest]]: [MySQL] 58,384 pass(es), 224 fail(s), and 6 exception(s).
[ View ]

Here is a reroll

webflo’s picture

Assigned:Unassigned» webflo

I try to re-roll this one.

webflo’s picture

Assigned:webflo» Unassigned

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)

andypost’s picture

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

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new8.61 KB
new128.14 KB
FAILED: [[SimpleTest]]: [MySQL] 58,517 pass(es), 205 fail(s), and 362 exception(s).
[ View ]

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

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new815 bytes
new128.14 KB
FAILED: [[SimpleTest]]: [MySQL] 58,004 pass(es), 206 fail(s), and 1 exception(s).
[ View ]

lets get rid of all those exceptions

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new127.08 KB
FAILED: [[SimpleTest]]: [MySQL] 57,800 pass(es), 163 fail(s), and 0 exception(s).
[ View ]

Re-roll.

Tor Arne Thune’s picture

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

Berdir’s picture

Berdir’s picture

Issue summary:View changes

Updated issue summary.

plach’s picture

Issue summary:View changes

Updated issue summary.

ParisLiakos’s picture

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

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new127.13 KB
FAILED: [[SimpleTest]]: [MySQL] 58,258 pass(es), 160 fail(s), and 0 exception(s).
[ View ]
new1.49 KB

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.

Gábor Hojtsy’s picture

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

Berdir’s picture

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.

Gábor Hojtsy’s picture

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.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new14.34 KB
new135.05 KB
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.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new135.01 KB
FAILED: [[SimpleTest]]: [MySQL] 59,336 pass(es), 129 fail(s), and 1,661 exception(s).
[ View ]

Re-roll....

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new1.65 KB
new135.02 KB
FAILED: [[SimpleTest]]: [MySQL] 58,842 pass(es), 30 fail(s), and 0 exception(s).
[ View ]

Fixed two stupid mistakes.

Go testbot, go!

Berdir’s picture

StatusFileSize
new3.26 KB
new135.73 KB
FAILED: [[SimpleTest]]: [MySQL] 58,501 pass(es), 24 fail(s), and 0 exception(s).
[ View ]

Fixing some more tests. No clue about a few of those :(

andypost’s picture

  1. +++ b/core/lib/Drupal/Core/Language/Plugin/LanguageNegotiation/LanguageNegotiationSession.php
    @@ -53,4 +53,35 @@ public function negotiateLanguage(array $languages, Request $request = NULL) {
    +  function languageSwitchLinks($type, $path) {
    ...
    +    $language_query = isset($_SESSION[$param]) ? $_SESSION[$param] : language($type)->id;
    ...
    +    $query = $_GET;
    ...
    +      $links[$langcode] = array(
    +        'href'       => $path,
    +        'title'      => $language->name,
    +        'attributes' => array('class' => array('language-link')),
    +        'query'      => $query,

    Any reason to build links here and use the globals?

  2. +++ b/core/lib/Drupal/Core/Language/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
    @@ -95,4 +96,25 @@ public function negotiateLanguage(array $languages, Request $request = NULL) {
    +  function languageSwitchLinks($type, $path) {
    +    $languages = language_list();
    +    $links = array();
    +
    +    foreach ($languages as $language) {
    +      $links[$language->id] = array(
    +        'href'       => $path,
    +        'title'      => $language->name,
    +        'language'   => $language,
    +        'attributes' => array('class' => array('language-link')),

    Same here. The method will be orphaned from interface so maybe better to find another place for that (some manager at least)

ianthomas_uk’s picture

I'm concerned #1775842: [meta] Convert all variables to state and/or config systems is blocked on this via #2108599: Convert language_default to CMI.

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?

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new12.25 KB
new137.82 KB
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.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new137.91 KB
FAILED: [[SimpleTest]]: [MySQL] 58,844 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Re-roll.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new9.25 KB
new143.96 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Added the alter hook and fixed cache clearing, replaced the crazy domain tests with unit tests.

Challenging the testbot gods: This will be green.*

* This is however by far not done.

plach’s picture

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.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new1.25 KB
new144 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found.
[ View ]

This had to happen.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new858 bytes
new144.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,828 pass(es).
[ View ]

Time to go to bed...

jibran’s picture

Yay!!! it is green. I know it is WIP so discard anything which is temporary.

  1. +++ b/core/includes/bootstrap.inc
    @@ -2418,65 +2391,7 @@ function language_multilingual() {
    function language_list($flags = Language::STATE_CONFIGURABLE) {

    @@ -2490,24 +2405,7 @@ function language_list($flags = Language::STATE_CONFIGURABLE) {
    function language_default_locked_languages($weight = 0) {

    @@ -2518,10 +2416,11 @@ function language_default_locked_languages($weight = 0) {
    function language_load($langcode) {

    +++ b/core/includes/language.inc
    @@ -315,28 +275,7 @@ function language_negotiation_method_enabled($method_id, $type = NULL) {
    function language_negotiation_get_switch_links($type, $path) {

    We should add @deprecated here.

  2. +++ b/core/lib/Drupal/Core/Language/Language.php
    @@ -17,6 +17,23 @@
    +   * @todo Remove once converted to config.

    Issue id should be nice.

  3. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
    @@ -154,21 +275,67 @@ public function reset($type = NULL) {
    +   * @param $type
    ...
    +   * @param $path

    @@ -177,16 +344,104 @@ protected function getLanguageTypes() {
    +    * @param $flags

    typehint missing.

  4. +++ b/core/lib/Drupal/Core/Language/LanguageNegotiationMethodBase.php
    @@ -0,0 +1,48 @@
    +   * Implements \Drupal\Core\Language\LanguageNegotiationInterface::setLanguageManager().

    @inheritdoc

  5. +++ 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.

    80 char.

  6. +++ b/core/modules/language/lib/Drupal/language/LanguageServiceProvider.php
    @@ -0,0 +1,34 @@
    +  public function register(ContainerBuilder $container) { }

    should be multiline.

  7. +++ b/core/modules/language/tests/language_elements_test/language_elements_test.info.yml
    @@ -4,4 +4,4 @@ description: 'Support module for the language form elements tests.'
    +#hidden: true

    +++ b/core/modules/language/tests/language_test/language_test.module
    @@ -96,16 +69,9 @@ function language_test_language_negotiation_info_alter(array &$negotiation_info)
    +  print_r(Drupal::languageManager()->getLanguageTypes());

    debuging :D

Berdir’s picture

StatusFileSize
new48.1 KB
new183.27 KB
FAILED: [[SimpleTest]]: [MySQL] 58,505 pass(es), 137 fail(s), and 8 exception(s).
[ View ]

Ok, starting to gradually clean this stuff up, I fear the patch will still get quite a bit bigger, we'll see.

What this does:
- Drop the old constants, replace with the new
- simplify the language_negotiation_$type variables
- Remove some probably unnecessary code. :)
- Some other cleanup
-

plach’s picture

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php
@@ -257,74 +264,74 @@ function testUILanguageNegotiation() {
+  protected function assertNegotation($test) {

I was skimming through the interdiff and I noticed this typo.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new16.44 KB
new187.96 KB
FAILED: [[SimpleTest]]: [MySQL] 58,864 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Fixing upgrade path and apparently that call in content_translation() is needed, seems wrong but so be it.

plach’s picture

+++ b/core/modules/content_translation/content_translation.install
@@ -86,6 +86,8 @@ function content_translation_install() {
+  language_negotiation_include();
+  language_negotiation_set(Language::TYPE_CONTENT, array(LanguageNegotiationUrl::METHOD_ID => 0));

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.

Berdir’s picture

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

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new1.03 KB
new187.59 KB
FAILED: [[SimpleTest]]: [MySQL] 59,821 pass(es), 3 fail(s), and 210 exception(s).
[ View ]

Re-roll and using ServiceProviderBase. Stay tuned for more changes ;)

cosmicdreams’s picture

Berdir I see a lot of documentation on a few other code changes that are asking to be done. I can take a shot at fixing them when you give the all clear.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new3.51 KB
new188.38 KB
PASSED: [[SimpleTest]]: [MySQL] 59,669 pass(es).
[ View ]

Fixing those tests.

Done with this for the weekend, feel free to work on it.

cosmicdreams’s picture

StatusFileSize
new188.81 KB
PASSED: [[SimpleTest]]: [MySQL] 59,578 pass(es).
[ View ]
jibran’s picture

Yay!! it is green. Here is little help :).

  1. +++ b/core/includes/bootstrap.inc
    @@ -2418,65 +2394,7 @@ function language_multilingual() {
    function language_list($flags = Language::STATE_CONFIGURABLE) {

    @@ -2490,24 +2408,7 @@ function language_list($flags = Language::STATE_CONFIGURABLE) {
    function language_default_locked_languages($weight = 0) {

    @@ -2518,10 +2419,11 @@ function language_default_locked_languages($weight = 0) {
    function language_load($langcode) {

    @deprecated missing.

  2. +++ b/core/includes/bootstrap.inc
    @@ -2568,15 +2470,8 @@ function language_is_locked($langcode) {
    +  $default_info = variable_get('language_default', Language::$defaultValues);

    +++ b/core/includes/language.inc
    @@ -372,36 +297,27 @@ function language_negotiation_purge() {
    +  variable_set("language_negotiation_$type", $method_weights);

    @todo with issue id perhaps.

  3. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
    @@ -37,6 +36,34 @@ class LanguageManager {
    +   * @var array

    @@ -46,20 +73,23 @@ class LanguageManager {
    +   * @param array $config

    \Drupal\Core\Config\Config[] now.

  4. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
    @@ -71,9 +101,11 @@ public function init() {
    +      include_once DRUPAL_ROOT . '/core/includes/language.inc';

    @todo here when/how we are going to remove it with issue id.

  5. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
    @@ -105,31 +137,122 @@ public function getLanguage($type = Language::TYPE_INTERFACE) {
    +        $config = \Drupal::config('system.performance');

    Why not $this->config?

  6. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
    @@ -154,21 +278,67 @@ public function reset($type = NULL) {
    +          \Drupal::moduleHandler()->alter('language_switch_links', $result, $type, $path);

    Why not $this->moduleHandler.

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

  8. +++ 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.

    80 chars

cosmicdreams’s picture

StatusFileSize
new1.2 KB
new189.31 KB
PASSED: [[SimpleTest]]: [MySQL] 59,216 pass(es).
[ View ]

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

P.S. Having weird problems with interdiff. hopefully this works

ianthomas_uk’s picture

Berdir’s picture

3. No, $config is a raw php array.
5. The config system needs the language manager, we can't inject config because that would result in a circular dependency.

cosmicdreams’s picture

StatusFileSize
new0 bytes
new189.35 KB
PASSED: [[SimpleTest]]: [MySQL] 59,646 pass(es).
[ View ]

Because of #225,
Reverted type for $config so that it's an array again.

Because of #224,
added more specific issues to the todo's of methods in LanguageManager that include a variable_get/set

P.S.: interdiffs are still giving me issues, I don't have a good fix so my interdiffs might be wrong from now on.

cosmicdreams’s picture

Issue summary:View changes

Updated issue summary.

YesCT’s picture

Assigned:Unassigned» YesCT
Issue summary:View changes
Issue tags:+Needs reroll
StatusFileSize
new1.61 KB

Here is the interdiff from 223 to 226.
needs a reroll though. I'll try that now. (following https://drupal.org/patch/reroll)

YesCT’s picture

Assigned:YesCT» Unassigned
Issue tags:-Needs reroll
StatusFileSize
new19.45 KB
new189.92 KB
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.

Auto-merging core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php
CONFLICT (content): Merge conflict in core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php

#1999388: Use Symfony Request for language module modified the hunk this patch is removing. checked that change it did. I think it is still ok to remove the hunk. removing.

Auto-merging core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.php
CONFLICT (content): Merge conflict in core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.php

this one is:

<<<<<<< HEAD
      \Drupal::request()->server->set('HTTP_ACCEPT_LANGUAGE', $accept_language);
      $result = language_from_browser($languages);
=======
      $request = Request::create('', 'GET', array(), array(), array(), array('HTTP_ACCEPT_LANGUAGE' => $accept_language));
      $result = $language_neg->negotiateLanguage($languages, $request);
>>>>>>> 226

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.

Auto-merging core/modules/language/lib/Drupal/language/Form/LanguageAddForm.php
Auto-merging core/modules/language/language.negotiation.inc
CONFLICT (content): Merge conflict in core/modules/language/language.negotiation.inc

this one is

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

no need for a followup to move the uses.

Auto-merging core/includes/common.inc
Auto-merging core/core.services.yml

attached a txt to show exactly how the conflicts were resolved.
removing needs reroll tag
unasigning from me
still needs review?

YesCT’s picture

Assigned:YesCT» Unassigned
StatusFileSize
new1.53 KB
new189.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1862202_231.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

in language.negotiation.inc

<<<<<<< HEAD
      $query = \Drupal::request()->query;
      if ($query->has($query_param)) {
        $query_value = String::checkPlain(\Drupal::request()->query->get($query_param));
      }
      else {
        return FALSE;
      }
      $query_rewrite = isset($languages[$query_value]) && language_negotiation_method_enabled(LANGUAGE_NEGOTIATION_SESSION);
=======
      $query_param = check_plain(\Drupal::config('language.negotiation')->get('session.parameter'));
      $query_value = isset($_GET[$query_param]) ? check_plain($_GET[$query_param]) : NULL;
      $query_rewrite = isset($languages[$query_value]) && language_negotiation_method_enabled(LanguageNegotiationSession::METHOD_ID);
>>>>>>> 226

looking at the code before, and what this patch was trying to do, I ended up with keeping what was in head and just making the change to the one query_rewrite = line that it was trying to do.

adding back the uses to make that ok.

---
at the end of language_url_rewrite_session() there may not be a return if it goes in those last if's there. I'm not sure this issue needs to worry about that.

podarok’s picture

Issue summary:View changes
xjm’s picture

Gábor Hojtsy’s picture

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.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new187.2 KB
FAILED: [[SimpleTest]]: [MySQL] 49,594 pass(es), 835 fail(s), and 1,834 exception(s).
[ View ]

Another re-roll.

plach’s picture

Assigned:Unassigned» plach

Working on this.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new2.77 KB
new187.35 KB

Hopefully this should be green again. More work tomorrow.

plach’s picture

StatusFileSize
new2.77 KB
new187.35 KB
FAILED: [[SimpleTest]]: [MySQL] 58,956 pass(es), 19 fail(s), and 1 exception(s).
[ View ]

...

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new22.24 KB
new200.5 KB
PASSED: [[SimpleTest]]: [MySQL] 59,050 pass(es).
[ View ]

More test fixes.

penyaskito’s picture

Awesome!

Some thoughts while reviewing/bikeshedding/actual reviews above:

  1. +++ b/core/core.services.yml
    @@ -229,9 +229,6 @@ services:
    -  language_manager:
    -    class: Drupal\Core\Language\LanguageManager
    -    arguments: ['@state', '@module_handler']

    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.

  2. +++ b/core/includes/bootstrap.inc
    @@ -2386,37 +2386,13 @@ function language($type) {
    + * @deprecated as of Drupal 8.0, use
    + *   \Drupal::languageManager()->isMultilingual() instead.

    We need follow-ups for removing uses of all the deprecated methods.

  3. +++ b/core/lib/Drupal/Core/Language/InstallLanguageManager.php
    @@ -0,0 +1,40 @@
    +class InstallLanguageManager extends LanguageManager {

    Bikeshedding: If we had to reroll this, I would suggest to rename this class to InstallationLanguageManager.

  4. +++ b/core/lib/Drupal/Core/Language/Language.php
    @@ -17,6 +17,23 @@
    +   * @todo Remove once converted to config.
    +   *
    +   * @var array
    +   */
    +  public static $defaultValues = array(
    +    'id' => 'en',
    +    'name' => 'English',
    +    'direction' => 0,
    +    'weight' => 0,
    +    'locked' => 0,
    +    'default' => TRUE,
    +  );
    +

    First occurence of Language::defaultValues really annoyed me, but later in review found that there is a ToDo for moving that to config and we can do that in a follow-up.

  5. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
    @@ -46,6 +40,34 @@ class LanguageManager {
       protected $languages;
    ...
    +  protected $languageList;

    I'm not sure why we have two lists of languages ($languages, $languageList), I have to investigate that later.

  6. +++ b/core/lib/Drupal/Core/Language/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
    @@ -0,0 +1,120 @@
    + *   id = \Drupal\Core\Language\Plugin\LanguageNegotiation\LanguageNegotiationUrl::METHOD_ID,

    Cool that we don't need to duplicate the strings, didn't know that annotations support that.

This issue is blocking some other issues in the D8MI, so I would say that we need the follow-ups and we can RTBC this one.

plach’s picture

StatusFileSize
new4.22 KB
new200.85 KB
PASSED: [[SimpleTest]]: [MySQL] 59,150 pass(es).
[ View ]

Here are some clean-ups. In Vienna I'll work on the plan outlined in #155.

plach’s picture

StatusFileSize
new217.08 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Temporary patch. Let's see how many failures we have.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new34.07 KB
new220.22 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Another interim patch, still needs work. Let's see how it behaves.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new5.31 KB
new221.32 KB
FAILED: [[SimpleTest]]: [MySQL] 48,571 pass(es), 1,176 fail(s), and 148 exception(s).
[ View ]

Let's try again.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new870 bytes
new221.33 KB
FAILED: [[SimpleTest]]: [MySQL] 54,353 pass(es), 3,193 fail(s), and 349 exception(s).
[ View ]
+++ b/core/modules/language/lib/Drupal/language/LanguageManager.php
@@ -67,7 +67,7 @@ public function getLanguageList($flags = Language::STATE_CONFIGURABLE) {
-      foreach (\Drupal::service('config.factory')->loadMultiple($language_ids) as $language_config) {
+      foreach ($this->configStorage->loadMultiple($language_ids) as $language_config) {

Reverted an uninteded change.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new12.29 KB
new222.93 KB
FAILED: [[SimpleTest]]: [MySQL] 57,938 pass(es), 308 fail(s), and 119 exception(s).
[ View ]

This should fix the DI errors.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new18.17 KB
new225.22 KB
FAILED: [[SimpleTest]]: [MySQL] 58,289 pass(es), 216 fail(s), and 79 exception(s).
[ View ]

Hopefully this should fix some more failures.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new20.04 KB
new228.38 KB
FAILED: [[SimpleTest]]: [MySQL] 59,033 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

And more fixes.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new7.93 KB
new232.58 KB
FAILED: [[SimpleTest]]: [MySQL] 59,042 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Hopefully this should be green again.

Aside from fixing a couple of bugs this removes from tests the assumption that the host and the test environments share the same language configuration.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new973 bytes
new233.17 KB
PASSED: [[SimpleTest]]: [MySQL] 59,114 pass(es).
[ View ]

Last fix?

plach’s picture

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

And now even with patch attached!

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new233.11 KB
FAILED: [[SimpleTest]]: [MySQL] 59,178 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Another reroll

penyaskito’s picture

Status:Needs work» Needs review
StatusFileSize
new6.92 KB
new233.11 KB
FAILED: [[SimpleTest]]: [MySQL] 59,316 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Rerolled. Attached diff -u language-oo-1862202-270.patch language-oo-1862202-272.patch > interdiff-between-patches.txt

plach’s picture

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.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new261.32 KB
FAILED: [[SimpleTest]]: [MySQL] 55,208 pass(es), 1,515 fail(s), and 362 exception(s).
[ View ]

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.

Let's see what breaks.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new8.69 KB
new261.8 KB
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 ]

Some test fixes, more to come.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new268.93 KB
FAILED: [[SimpleTest]]: [MySQL] 53,318 pass(es), 4,021 fail(s), and 3,781 exception(s).
[ 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.

penyaskito’s picture

There are some calls to language_update_count() in the patch. If we remove the state entry, we should remove those calls and probably the function itself. Not sure if it is in scope.

plach’s picture

StatusFileSize
new7.66 KB
new263.55 KB
FAILED: [[SimpleTest]]: [MySQL] 53,319 pass(es), 4,039 fail(s), and 3,764 exception(s).
[ View ]

Reverted some uninteded changes and removed some language_count removal leftovers.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new685 bytes
new263.51 KB
FAILED: [[SimpleTest]]: [MySQL] 57,823 pass(es), 806 fail(s), and 489 exception(s).
[ View ]

Oops

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new3.18 KB
new264.08 KB
FAILED: [[SimpleTest]]: [MySQL] 57,888 pass(es), 798 fail(s), and 173 exception(s).
[ View ]

And more fixes

plach’s picture

FYI we have a green (again) patch in #2156425-16: [ignore] Test issue for "Objectify the language negotiation system". I will try to get the last things done tomorrow (mostly converting language types) and then we should be ready for review.

Meanwhile could a power-user delete the bot messages? We are approaching the 300 threshold...

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new228.88 KB
new1.93 KB
new297.89 KB
PASSED: [[SimpleTest]]: [MySQL] 59,890 pass(es).
[ View ]

Ok, we should be ready for review. Posting here the patch from #2156425-37: [ignore] Test issue for "Objectify the language negotiation system" which should come back green as the only failure over there was a bot fluke.

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

Run #52b78c738c073 Run #52b78e3941d7b Diff Diff%
Number of Function Calls 405,290 405,495 205 0.1%
Incl. Wall Time (microsec) 926,721 923,404 -3,317 -0.4%
Incl. CPU (microsecs) 912,000 912,000 0 0.0%
Incl. MemUse (bytes) 23,145,648 23,268,424 122,776 0.5%
Incl. PeakMemUse (bytes) 24,488,480 24,590,936 102,456 0.4%

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.

Edit: please, ignore the interdiff, it's totally meaningless here, sorry.

plach’s picture

Title:Objectify the language negotiation system» Objectify the language system

Better title :)

Berdir’s picture

Not a full review, but picking out some things that I noticed.

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

  1. +++ b/core/includes/bootstrap.inc
    @@ -2541,10 +2448,14 @@ function language_default_locked_languages($weight = 0) {
    + *
    + * @see \Drupal\Core\Language\LanguageManager::loadLanguage().
    + *

    No . after a @see

  2. +++ b/core/includes/bootstrap.inc
    @@ -2578,10 +2489,14 @@ function language_name($langcode) {
    + *
    + * @see \Drupal\Core\Language\LanguageManager::isLanguageLocked().
    + *

    another one.

  3. +++ b/core/includes/bootstrap.inc
    @@ -2591,33 +2506,56 @@ function language_is_locked($langcode) {
      *   A language object.
      */
    function language_default() {

    Missing @deprecated.

  4. +++ b/core/includes/bootstrap.inc
    @@ -2591,33 +2506,56 @@ function language_is_locked($langcode) {
    + * Returns the language switch links for the given language type.
    + *
    + * @param $type
    + *   The language type.
    + * @param $path
    + *   The internal path the switch links will be relative to.
    + *
    ...
    +}
    ...
    + * @return
    + *   A keyed array of links ready to be themed.
    + *
    + * @deprecated as of 8.0, use
    + *   \Drupal::languageManager()->getLanguageSwitchLinks() instead.
    + */
    +function language_negotiation_get_switch_links($type, $path) {
    +  return \Drupal::languageManager()->getLanguageSwitchLinks($type, $path);

    Wondering how many of those function we really need to keep? Because if we keep it, we'll have to fix coding style like the missing types on @param and @return.

  5. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
    @@ -129,7 +129,7 @@ public function __construct($subdir, \Traversable $namespaces, $plugin_definitio
        *   definitions should be cleared along with other, related cache entries.
        */
    -  public function setCacheBackend(CacheBackendInterface $cache_backend, LanguageManager $language_manager, $cache_key_prefix, array $cache_tags = array()) {
    +  public function setCacheBackend(CacheBackendInterface $cache_backend, LanguageManagerInterface $language_manager, $cache_key_prefix, array $cache_tags = array()) {

    There's a @todo in the default plugin manager about using the language manager for the language list, should be easy to get rid of that now.

  6. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockLanguageTest.php
    @@ -74,14 +74,13 @@ public function testLanguageBlockVisibility() {
         // Check that a page has a block.
    -    $this->drupalget('', array('language' => language_load('en')));
    +    $this->drupalGet('en');
         $this->assertText('Powered by Drupal', 'The body of the custom block appears on the page.');

         // Check that a page doesn't has a block for the current language anymore.
    -    $this->drupalGet('', array('language' => language_load('fr')));
    +    $this->drupalGet('fr');

    Is it really ok to replace them like this? I know I had a test once where this didn't work.

  7. +++ b/core/modules/language/language.module
    @@ -580,114 +571,189 @@ function language_language_types_info() {
    + *
    + * @see hook_language_types_info().
    + *

    another .

  8. +++ b/core/modules/language/language.module
    @@ -580,114 +571,189 @@ function language_language_types_info() {
    + *
    + * @deprecated as of Drupal 8.0, use
    + *   \Drupal::service('language_negotiator')->updateConfiguration($types)
    + *   instead.

    I don't think method references like this should have arguments.

  9. +++ b/core/modules/language/lib/Drupal/language/LanguageNegotiationMethodManager.php
    @@ -0,0 +1,48 @@
    +  }
    ...
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function clearCachedDefinitions() {
    +    $this->definitions = NULL;
    +    $this->cacheBackend->delete($this->cacheKey);

    If we use the injected language manager in the base class, then we can check for it's existence and don't need this override.

  10. +++ b/core/modules/language/lib/Drupal/language/LanguageNegotiationMethodManager.php
    @@ -0,0 +1,48 @@
    +    }
    ...
    +    if ($module_handler) {
    +      $this->alterInfo($module_handler, 'language_negotiation_info');

    Is this still necessary (it being conditional I mean)

  11. +++ b/core/modules/language/lib/Drupal/language/LanguageNegotiator.php
    @@ -0,0 +1,360 @@
    +  function updateConfiguration(array $types) {
    ...
    +  function purgeConfiguration() {
    ...
    +  function saveConfiguration($type, $method_weights) {

    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?

  12. +++ b/core/modules/language/lib/Drupal/language/ConfigurableLanguageManager.php
    @@ -0,0 +1,352 @@
    +    // TODO is this necessary?
    +    $this->init();

    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.

  13. +++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
    @@ -79,7 +79,10 @@ protected function setUp() {
    +
    ...

         $this->urlGenerator = $this->getMock('\Drupal\Core\Routing\UrlGenerator', array(), array(), '', FALSE);
         $this->moduleHandler = $this->getMock('Drupal\Core\Extension\ModuleHandlerInterface');
    -    $this->languageManager = $this->getMock('Drupal\Core\Language\LanguageManager');
    +    $this->languageManager = $this->getMockBuilder('Drupal\Core\Language\LanguageManager')
    +      ->disableOriginalConstructor()
    +      ->getMock();

    We should be able to simplify those lines by using the interface now for the mock.

plach’s picture

StatusFileSize
new42.26 KB
new312.09 KB
PASSED: [[SimpleTest]]: [MySQL] 60,053 pass(es).
[ View ]

Thanks! Addressed #292 except for:

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

Note: 12 is in the patch although I couldn't make it appear in the interdiff due to a previous merge. It should be ok, see #2156425-40: [ignore] Test issue for "Objectify the language negotiation system".

larowlan’s picture

  1. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
    @@ -7,245 +7,162 @@
    +      $default->name = t("Site's default language (@lang_name)", array('@lang_name' => $default->name));

    can we inject the string translation service here?

  2. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
    @@ -7,245 +7,162 @@
    +    // FIXME t() calls here!

    ah, looks like that's on the radar already :)

  3. +++ b/core/modules/language/language.module
    @@ -580,114 +571,189 @@ function language_language_types_info() {
    +function language_rebuild_services() {

    Any reason this can't be a static method on the object? Seems unfortunate that an issue to convert stuff to OO has to add a new global function :)

  4. +++ b/core/modules/language/language.module
    @@ -580,114 +571,189 @@ function language_language_types_info() {
    +function language_negotiation_url_prefixes() {
    ...
    +function language_negotiation_url_prefixes_update() {
    ...
    +function language_negotiation_url_prefixes_save(array $prefixes) {
    ...
    +function language_negotiation_url_domains() {
    ...
    +function language_negotiation_url_domains_save(array $domains) {

    Same - should there be follow ups for moving these to a service?

  5. +++ b/core/modules/language/lib/Drupal/language/ConfigurableLanguageManager.php
    @@ -0,0 +1,352 @@
    +      $default_info = variable_get('language_default', Language::$defaultValues);

    ouch

  6. +++ b/core/modules/language/lib/Drupal/language/LanguageServiceProvider.php
    @@ -0,0 +1,72 @@
    +      $container->register('language_request_subscriber', 'Drupal\language\EventSubscriber\LanguageRequestSubscriber')
    ...
    +      $container->register('path_processor_language', 'Drupal\language\HttpKernel\PathProcessorLanguage')

    this is nice cleanup!

  7. +++ b/core/modules/language/lib/Drupal/language/LanguageServiceProvider.php
    @@ -0,0 +1,72 @@
    +   * @param \Drupal\Core\DependencyInjection\ContainerBuilder

    nitpick: missing $container

  8. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
    @@ -0,0 +1,209 @@
    +          $http_host = $request->server->get('HTTP_HOST');
    +          if (strpos($http_host, ':') !== FALSE) {
    +            $http_host_tmp = explode(':', $http_host);
    +            $http_host = current($http_host_tmp);

    This could be simplified to use Request->getHost(), which (unlike ::getHttpHost()) excludes the port.

  9. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
    @@ -0,0 +1,209 @@
    +    if ($request) {

    can we really hardcode the port to 80 when the request is null? In what circumstances is this called without a request?

  10. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
    @@ -0,0 +1,209 @@
    +        $options['base_url'] = $url_scheme . '://' . $config['domains'][$options['language']->id];
    +
    +        // In case either the original base URL or the HTTP host contains a
    +        // port, retain it.
    +        if (isset($normalized_base_url) && strpos($normalized_base_url, ':') !== FALSE) {
    +          list(, $port) = explode(':', $normalized_base_url);
    +          $options['base_url'] .= ':' . $port;
    +        }
    +        elseif ($port != 80) {
    +          $options['base_url'] .= ':' . $port;
    +        }
    +
    +        if (isset($options['https']) && !empty($options['mixed_mode_sessions'])) {
    +          if ($options['https'] === TRUE) {
    +            $options['base_url'] = str_replace('http://', 'https://', $options['base_url']);
    +          }
    +          elseif ($options['https'] === FALSE) {
    +            $options['base_url'] = str_replace('https://', 'http://', $options['base_url']);
    +          }
    +        }
    +
    +        // Add Drupal's subfolder from the base_path if there is one.
    +        $options['base_url'] .= rtrim(base_path(), '/');

    there's a lot going on here, if we don't have unit-tests for this code-path, we need a followup to add it.

  11. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationUserAdmin.php
    @@ -0,0 +1,53 @@
    +    $request_path = $request ? urldecode(trim($request->getPathInfo(), '/')) : _current_path();

    fyi, working to remove _current_path in #2016629: Refactor bootstrap to better utilize the kernel

  12. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationUserAdmin.php
    @@ -0,0 +1,53 @@
    +    if ($this->languageManager && $this->currentUser->isAuthenticated() && path_is_admin($request_path)) {

    wrapping path_is_admin() in a method that could be mocked would enabled unit testing

  13. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageDependencyInjectionTest.php
    @@ -74,12 +61,12 @@ function testDependencyInjectedNewDefaultLanguage() {
    -    drupal_language_initialize();
    +    $this->languageManager->init();
    ...
    -    $result = language(Language::TYPE_INTERFACE);
    +    $result = $this->languageManager->getLanguage();

    oh yeah!

  14. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php
    @@ -316,50 +319,14 @@ function testUILanguageNegotiation() {
    -
    -    // Setup for domain negotiation, first configure the language to have domain
    -    // URL.
    -    $edit = array("domain[$langcode]" => $language_domain);
    -    $this->drupalPostForm("admin/config/regional/language/detection/url", $edit, t('Save configuration'));
    -    // Set the site to use domain language negotiation.
    -
    -    $tests = array(
    -      // Default domain, browser preference should have no influence.
    -      array(
    -        'language_negotiation' => array(LANGUAGE_NEGOTIATION_URL, LANGUAGE_NEGOTIATION_SELECTED),
    -        'language_negotiation_url_part' => LANGUAGE_NEGOTIATION_URL_DOMAIN,
    -        'path' => 'admin/config',
    -        'expect' => $default_string,
    -        'expected_method_id' => LANGUAGE_NEGOTIATION_SELECTED,
    -        'http_header' => $http_header_browser_fallback,
    -        'message' => 'URL (DOMAIN) > DEFAULT: default domain should get default language',
    -      ),
    -      // Language domain specific URL, we set the 'HTTP_HOST' property of
    -      // \Drupal::request()->server in \Drupal\language_test\LanguageTestManager
    -      // to simulate this.
    -      array(
    -        'language_negotiation' => array(LANGUAGE_NEGOTIATION_URL, LANGUAGE_NEGOTIATION_SELECTED),
    -        'language_negotiation_url_part' => LANGUAGE_NEGOTIATION_URL_DOMAIN,
    -        'language_test_domain' => $language_domain . ':88',
    -        'path' => 'admin/config',
    -        'expect' => $language_string,
    -        'expected_method_id' => LANGUAGE_NEGOTIATION_URL,
    -        'http_header' => $http_header_browser_fallback,
    -        'message' => 'URL (DOMAIN) > DEFAULT: domain example.cn should switch to Chinese',
    -      ),
    -    );
    -
    -    foreach ($tests as $test) {
    -      $this->runTest($test);
    -    }

    question: where is this moved to?

  15. +++ b/core/modules/language/tests/Drupal/language/Tests/LanguageNegotiationUrlTest.php
    @@ -0,0 +1,165 @@
    +  public function providerTestDomain() {

    this looks like where it was moved to?
    neat!

Berdir’s picture

Issue tags:-Needs tests
StatusFileSize
new321.37 KB
PASSED: [[SimpleTest]]: [MySQL] 59,743 pass(es).
[ View ]
new14.78 KB

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

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

plach’s picture

StatusFileSize
new35.79 KB
new337.97 KB
PASSED: [[SimpleTest]]: [MySQL] 59,770 pass(es).
[ View ]

@larowlan:

Thanks!

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

plach’s picture

@Berdir:

I just noticed that the test session takes way longer here (~1h 45') than for the D8 head (~1h 10'): is there anything obvious we should look for?

Berdir’s picture

Test runs differ a lot, those numbers don't mean anything, some bots are physical machines and twice as fast as the virtual ones and even the same servers often differ quite a bit.

Would be more meaningful to e.g. run the a large group of tests, specifically those related to this (Language/Locale tests?) and compare how long they take on a local, stable system.

penyaskito’s picture

Last run: Test run duration: 1 hour 11 min.

plach’s picture

Cool :)

On my machine the Language group actually took less time (7' 41'' vs 8' 09'') with the patch applied. Sorry for the false alarm.

plach’s picture

StatusFileSize
new337.97 KB
PASSED: [[SimpleTest]]: [MySQL] 59,939 pass(es).
[ View ]
plach’s picture

joelpittet’s picture

This patch is looking really good! Though dreditor is having a hissy fit with the size I noticed some minor things that could be picked off.

Sorry super nitpicky things to follow:

There are a few 80 char run overs:
(optional) Specifies the state of the languages that have to be returned.
An associative array of languages, keyed by the language code, ordered by

There are two spots that say:
@deprecated as of Drupal 8.0, use
which should be, this to be consistent.
@deprecated as of Drupal 8.0. Use

plach’s picture

StatusFileSize
new4.5 KB
new338.13 KB
PASSED: [[SimpleTest]]: [MySQL] 60,123 pass(es).
[ View ]

Rerolled and fixed #310, thanks.

Gábor Hojtsy’s picture

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

plach’s picture

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.

Gábor Hojtsy’s picture

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

plach’s picture

StatusFileSize
new340.56 KB
PASSED: [[SimpleTest]]: [MySQL] 59,532 pass(es).
[ View ]
new12.21 KB

This should fix the @todos as per @Gabor's request. I left out the one in TestBase since we are only fixing a whitespace there and I have no idea whether there is an issue covering it.

Meanwhile the language negotiation settings UI was converted to OO code so we have a big interdiff.

plach’s picture

Issue summary:View changes

Updated the API changes section.

PS: I don't want this to be necessarily the final-final version, but I'd like to avoid avoidable API changes :)

plach’s picture

StatusFileSize
new340.56 KB
PASSED: [[SimpleTest]]: [MySQL] 59,609 pass(es).
[ View ]

Re-uploading #316 as the bot didn't pick it up.

plach’s picture

Issue summary:View changes

Follow-ups created.

catch’s picture

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.

plach’s picture

StatusFileSize
new20.41 KB
new345.48 KB
FAILED: [[SimpleTest]]: [MySQL] 59,293 pass(es), 21 fail(s), and 29 exception(s).
[ View ]

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:

LanguageManagerInterface::getLanguage() => LanguageManagerInterface::getCurrentLanguage()
LanguageManagerInterface::loadLanguage() => LanguageManagerInterface::getLanguage()

afterwards we should be done IMHO :)

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new2.64 KB
new348.13 KB
FAILED: [[SimpleTest]]: [MySQL] 59,330 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

This should fix most failing tests. I still need to figure out what's going wrong with the last one.

Gábor Hojtsy’s picture

Re #320/#321: The direction in #2098119: Replace config context system with baked-in locale support and single-event based overrides is to move the config override entirely to language module, so the language manager in language module would handle it, no need for extending it and overwriting it from locale module AFAIS. (The reason for that is locale module is not needed to do these overrides given #2098119: Replace config context system with baked-in locale support and single-event based overrides would load and manage them on the config factory directly in itself). So if this separation is introduced here, it would need to be removed there again :)

Re the renames suggested in #320, sounds good :)

Also *huge* thanks for the issue summary update, looks clear now :)

plach’s picture

Gabor:

So, as a temporary measure, would it be ok to move the initConfigSubscriber() method onto the Language module language mananger?

Gábor Hojtsy’s picture

@plach: I think so yes.

catch’s picture

Fine with #326 as well (although I'd also be fine with #321 since both solves reading config from disk and we'll need to preserve not doing that).

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new49.85 KB
new365.32 KB
FAILED: [[SimpleTest]]: [MySQL] 59,313 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Ok, this should fix config overrides and performs the renames suggested in #320. Hopefully I didn't break anything :)

plach’s picture

plach’s picture

plach’s picture

Issue summary:View changes
StatusFileSize
new24.3 KB
new365.06 KB
FAILED: [[SimpleTest]]: [MySQL] 59,255 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

I forgot @Gabor suggested to rename also LM::getLanguageList() to LM::getLanguages().

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new485 bytes
new365.54 KB
PASSED: [[SimpleTest]]: [MySQL] 59,318 pass(es).
[ View ]

And this should fix the last failures.

plach’s picture

Issue summary:View changes
Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

Based on several deep reviews above from @Berdir (suggesting to RTBC already in December), @larowlan, @joelpitett and our discussion with the remaining questions with @catch and @plach being resolved to everyone's satisfaction, I think this is clearly ready to get in. This is also in agreement in config approach with #2098119: Replace config context system with baked-in locale support and single-event based overrides even though the first that is committed will make the other one need a reroll.

jessebeach’s picture

I'm stubbing out the Change Record here: http://piratepad.net/c5nQEfPwZi

Please contribute details. This patch will need some substantial explanation.

jessebeach’s picture

This is my breakdown of the changes from the patch in #336. Does it make sense to include removed Classes/Services/Plugins/etc as well?

New Objects

\Drupal\Component\Uticodety\Browser
\Drupal\language\ConfigurableLanguageManager
\Drupal\Core\EventSubscriber\LanguageRequestSubscriber
\Drupal\language\EventSubscriber\LanguageRequestSubscriber
\Drupal\language\LanguageNegotiationMethodBase
\Drupal\language\LanguageNegotiationMethodManager
\Drupal\language\LanguageNegotiator

New Interfaces

\Drupal\Core\Language\LanguageManagerInterface
\Drupal\language\ConfigurableLanguageManagerInterface
\Drupal\language\LanguageNegotiationMethodInterface
\Drupal\language\LanguageNegotiatorInterface
\Drupal\language\LanguageSwitcherInterface

New Plugins

\Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationBrowser
\Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationSelected
\Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationSession
\Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUI
\Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUrl
\Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUrlFallback
\Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUrl
\Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUserAdmin

New Services

\Drupal\language\LanguageServiceProvider
plugin.manager.language_negotiation_method
language_negotiator

Deprecated functions in bootstrap.inc

language()
\Drupal::languageManager()->getCurrentLanguage()
language_list()
\Drupal::languageManager()->getLanguages()
language_load()
\Drupal::languageManager()->getLanguage()
language_default()
\Drupal::languageManager()->getDefaultLanguage()
sun’s picture

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:

  1. This change is an epic and utterly important milestone for D8. We need it to move forward as quickly and as pragmatic as possible.
  2. 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.
  3. 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).
  4. 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.
  5. 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.
  6. Given that we're beyond 300 comments here already (whereas I'm constantly deleting testbot comments), I'd even be fine with discussing + addressing the more major architectural issues in a (perhaps single) follow-up issue, too.
  7. 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! :)

  1. +++ b/core/includes/install.core.inc
    @@ -302,6 +303,9 @@ function install_begin_request(&$install_state) {
    +  // Register the 'language_manager' service.
    +  $container->register('language_manager', 'Drupal\Core\Language\LanguageManager');

    This should be moved into the else control structure path...

    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.

  2. +++ b/core/includes/install.core.inc
    @@ -1616,22 +1615,19 @@ function install_select_language_form($form, &$form_state, $files = array()) {
    +  $browser_langcode = Browser::getLangcode($request->server->get('HTTP_ACCEPT_LANGUAGE'), $browser_options);

    "Browser" is a term used in common speech, which is ambiguous without context.

    The proper technical term is User Agent. Let's ensure to create a follow-up issue to rename Utility\Browser into Utility\UserAgent.

  3. +++ b/core/includes/update.inc
    @@ -555,7 +550,7 @@ function update_prepare_d8_language() {
    +      $languages = \Drupal::languageManager()->getDefaultLockedLanguages($max_language_weight);

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

  4. +++ b/core/lib/Drupal/Component/Utility/Browser.php
    @@ -0,0 +1,141 @@
    +  public static function getLangcode($http_accept_language, $langcodes, $mappings = array()) {

    getLangcode() does not really encompass what this method is doing. It's rather a getBestMatchingLangcode() or similar.

    Let's create a follow-up issue to find a better name for this method.

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

    This addition is very concerning -- the bootstrap config storage used within DrupalKernel should not be exposed nor used anywhere else in the system, because it is entirely uncached.

    If it is required for any reason for this patch here, then let's create a major/critical follow-up issue to resolve that dependency and remove this addition.

  6. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -426,7 +426,7 @@ public function getBundleInfo($entity_type) {
    +      $langcode = $this->languageManager->getCurrentLanguage()->id;

    I wonder why we're accessing a public $id property directly everywhere, instead of using an id() method?

    Any chance to clean that up in a follow-up issue?

  7. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
    @@ -7,245 +7,202 @@
    +  public function __construct() {
    +    $this->defaultLanguage = new Language(Language::$defaultValues);
    +  }
    ...
    +  public function getDefaultLanguage() {
    +    return $this->defaultLanguage;
    +  }

    Would it make sense to move the instantiation of $defaultLanguage into getDefaultLanguage(), so as to instantiate upon access only?

  8. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -46,11 +54,24 @@ class TranslationManager implements TranslationInterface, TranslatorInterface {
    +    $this->defaultLangcode = $language_manager->getDefaultLanguage()->id;
    ...
    -    $this->defaultLangcode = language_default()->id;
    +  public function initLanguageManager() {
    +    $this->languageManager->setTranslation($this);

    Would it make sense to move the $defaultLangcode property initialization into the initLanguageManager() method?

  9. +++ b/core/lib/Drupal/Core/StringTranslation/Translator/StaticTranslation.php
    @@ -37,7 +37,7 @@ public function __construct($translations = array()) {
       public function getStringTranslation($langcode, $string, $context) {
         if (!isset($this->translations[$langcode])) {
    -      $this->translations[$langcode] = $this->loadLanguage($langcode);
    +      $this->translations[$langcode] = $this->getLanguage($langcode);
         }

    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.

  10. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
    @@ -13,7 +13,8 @@
    +use Drupal\language\ConfigurableLanguageManagerInterface;

    @@ -168,11 +169,11 @@ public function form(array $form, array &$form_state) {
    +    if ($this->languageManager->isMultilingual() && $this->languageManager instanceof ConfigurableLanguageManagerInterface) {

    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.

  11. +++ b/core/modules/language/lib/Drupal/language/ConfigurableLanguageManager.php
    @@ -0,0 +1,400 @@
    +  public function setNegotiator(LanguageNegotiatorInterface $negotiator) {
    +    $this->negotiator = $negotiator;
    +    $this->reset();
    +  }

    Why is the entire data reset when setting a negotiator?

    I think this should be removed, or if it is strictly required, then we should have a comment to explain why that is necessary.

  12. +++ b/core/modules/language/lib/Drupal/language/ConfigurableLanguageManagerInterface.php
    @@ -0,0 +1,89 @@
    +  public function getNegotiator();
    ...
    +  public function getDefinedLanguageTypes();

    I wonder whether we really need to repeat the term "Language" in 90% of all methods on classes that are named LanguageManager already?

    That's like having Entity::getEntityId() and similar, which we don't have either.

    I realize this remark is quite debatable, so I'd be more than happy to discuss a rename/shortening of these methods in a separate follow-up issue.

  13. +++ b/core/modules/language/lib/Drupal/language/ConfigurableLanguageManagerInterface.php
    @@ -0,0 +1,89 @@
    +   * Disables the given language types.
    ...
    +  function disableLanguageTypes(array $types);

    1. What is this method good for, or why would anyone want to disable language types?

    2. Why is there no corresponding method to enable language types?

  14. +++ b/core/modules/language/lib/Drupal/language/HttpKernel/PathProcessorLanguage.php
    @@ -38,129 +39,121 @@ class PathProcessorLanguage implements InboundPathProcessorInterface, OutboundPa
    +      $this->negotiator->setContext($this->currentUser, $request);

    1. Hm, that's a lot of contextual state being set on a service, no?

    2. A setContext() method with a bunch of arguments is a legacy pattern in my mind... Can't we split that into setUser() and setRequest()?

  15. +++ b/core/modules/language/lib/Drupal/language/HttpKernel/PathProcessorLanguage.php
    @@ -38,129 +39,121 @@ class PathProcessorLanguage implements InboundPathProcessorInterface, OutboundPa
    +  protected function initProcessors($scope) {
    +    $interface = '\Drupal\Core\PathProcessor\\' . Unicode::ucfirst($scope) . 'PathProcessorInterface';
    +    $this->processors[$scope] = array();
    +    foreach ($this->languageManager->getLanguageTypes() as $type) {
    +      foreach ($this->negotiator->getNegotiationMethods($type) as $method_id => $method) {
    +        if (!isset($this->processors[$scope][$method_id])) {
    +          $reflector = new \ReflectionClass($method['class']);
    +          if ($reflector->implementsInterface($interface)) {
    +            $this->processors[$scope][$method_id] = $this->negotiator->getNegotiationMethodInstance($method_id);
               }
             }

    This looks like a lot of runtime overhead in the critical path?

    Can we find a way to prepare this information ahead of time; e.g., in a new Container CompilerPass?

    Happy to defer to a separate follow-up issue though, as this patch is large enough already.

    That said, the dependent code in this class is manually probing whether $processors[$type] is set already, which could be inlined (once) into the initProcessors() method.

  16. +++ b/core/modules/language/lib/Drupal/language/LanguageNegotiationMethodInterface.php
    @@ -0,0 +1,56 @@
    +   * @return string
    +   *   A valid language code or FALSE if the negotitation was unsuccessful.
    +   */
    +  public function getLangcode(Request $request = NULL);

    1. Why return FALSE instead of just NULL/nothing?

    2. Why do we need so much state on these plugin class instances? Wouldn't it be much simpler to inject the contextual data into the method?

    function negotiate($language_manager, $request = NULL, $current_user = NULL)

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

  17. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationSelected.php
    @@ -0,0 +1,48 @@
    + * Class for identifying language from a selected language.
    ...
    +class LanguageNegotiationSelected extends LanguageNegotiationMethodBase {

    It's not clear to me what a "selected language" is.

    Given the implementation, this rather seems to be a "negotiation fallback to a statically configured language"?

  18. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationSelected.php
    @@ -0,0 +1,48 @@
    +      $langcode = $this->config->get('language.negotiation')->get('selected_langcode');
    +      if (!isset($languages[$langcode])) {
    +        $langcode = $this->languageManager->getDefaultLanguage()->id;
    +      }

    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?

  19. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationSession.php
    @@ -0,0 +1,146 @@
    +  public function getLangcode(Request $request = NULL) {
    ...
    +    // We need to update the session parameter with the request value only if we
    +    // have an authenticated user.
    +    if ($langcode && $this->languageManager) {
    +      $languages = $this->languageManager->getLanguages();
    +      if ($this->currentUser->isAuthenticated() && isset($languages[$langcode])) {
    +        $_SESSION[$param] = $langcode;
    +      }
    +    }

    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().

  20. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationSession.php
    @@ -0,0 +1,146 @@
    +        $links[$langcode]['attributes']['class'][] = ' session-active';

    Here + elsewhere (please grep):

    Bogus D6-style leading space in the added class name.

  21. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
    @@ -0,0 +1,203 @@
    + * @Plugin(
    + *   id = \Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUrl::METHOD_ID,
    ...
    +  const METHOD_ID = 'language-url';

    Given that these are plugins (for now), is there really a use-case for having the METHOD_ID constant, instead of using and relying on the plugin ID everywhere?

  22. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
    @@ -0,0 +1,203 @@
    + *   config = "admin/config/regional/language/detection/url"

    "config" is a weird annotation property name for declaring a URL?

    Can we at least append "_path" there?

    And why is that a system URL/path to begin with? :) Aren't we supposed to use "route" everywhere now?

  23. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
    @@ -0,0 +1,203 @@
    +           if ($negotiated_language !== FALSE && $negotiated_language instanceof \Drupal\Core\Language\Language) {

    How could it not be an instance of Language?

  24. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
    @@ -0,0 +1,203 @@
    +    // We allow only enabled languages here.
    +    elseif (is_object($options['language']) && !isset($languages[$options['language']->id])) {
    +      return $path;
    +    }

    Did you mean !is_object() || !isset()?

    Otherwise, passing a langcode string as $options['language'] won't be caught?

  25. +++ b/core/modules/language/lib/Drupal/language/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
    @@ -0,0 +1,203 @@
    +    if ($config['source'] == LanguageNegotiationUrl::CONFIG_PATH_PREFIX) {
    ...
    +    elseif ($config['source'] ==  LanguageNegotiationUrl::CONFIG_DOMAIN) {

    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.

  26. +++ b/core/modules/locale/locale.services.yml
    @@ -1,8 +1,6 @@
       locale_config_subscriber:
         class: Drupal\locale\LocaleConfigSubscriber
    -    tags:
    -      - { name: event_subscriber }

    Merge conflict?

jessebeach’s picture

Ok, I pulled a succinct distillation from the issues summary for the Change Record and added some info from my analysis of the patch:

http://piratepad.net/c5nQEfPwZi

For the authors of the patch, please edit/reword as you see appropriate.

Berdir’s picture

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.

plach’s picture

Status:Reviewed & tested by the community» Needs work

Thanks Daniel, that sounds like a needs work to me :)

No time for updating this tonight (got to get up early tomorrow): I just wanted to point out that most of your points make total sense to me, but I'll provide a detailed answer + new patch tomorrow.

Maybe we should pause the work on change notices until it's clear what the final patch here will look like ;)

catch’s picture

I just committed #2098119: Replace config context system with baked-in locale support and single-event based overrides which is going to mean a (non-trivial unfortunately) re-roll here. Anything else that's not also a difficult critical issue let's postpone on this though if it's going to conflict.

alexpott’s picture

Status:Needs work» Needs review
Issue tags:+Avoid commit conflicts
StatusFileSize
new2.88 KB
new362.7 KB
FAILED: [[SimpleTest]]: [MySQL] 57,689 pass(es), 98 fail(s), and 68 exception(s).
[ View ]

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.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new7.28 KB
new364.27 KB
PASSED: [[SimpleTest]]: [MySQL] 59,551 pass(es).
[ View ]

Okay we do need to use the bootstrap config storage! Also resolved a todo with respect to how language is set on the config factory.

plach’s picture

@alexpott:

Thanks for the reroll, changes look good to me :)

@sun:

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.

Given that we're beyond 300 comments here already (whereas I'm constantly deleting testbot comments), I'd even be fine with discussing + addressing the more major architectural issues in a (perhaps single) follow-up issue, too.

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.
  • 6: I think this might be covered by #1512424: Add a LanguageInterface, and property setters/getters to Language class.
  • 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:
    1. Currently the CMI keys are named a bit randomly, so it's not easy to automatically map a plugin to its configuration.
    2. Some plugins may need to access the other plugins' configuration, for instance the URL fallback needs that.
    3. 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
  • 26: Nope, however it's gone now :)

I am still working on addressing the rest of your review.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new364.53 KB
PASSED: [[SimpleTest]]: [MySQL] 59,765 pass(es).
[ View ]

Rerolled - since #2171015: Drupal 8 HEAD broken: installing Language module fails, after that cannot install any other module is partially responsible for needing it.

Plus this change means that language_test module had an undeclared dependency on Language module. But the test added by #2171015: Drupal 8 HEAD broken: installing Language module fails, after that cannot install any other module tests Language install using the language_test module therefore had to do the following:

diff --git a/core/modules/language/tests/language_test/language_test.module b/core/modules/language/tests/language_test/language_test.module
index c556c6f..73f8d80 100644
--- a/core/modules/language/tests/language_test/language_test.module
+++ b/core/modules/language/tests/language_test/language_test.module
@@ -66,11 +66,13 @@ function language_test_language_negotiation_info_alter(array &$negotiation_info)
  * Store the last negotiated languages.
  */
function language_test_store_language_negotiation() {
-  $last = array();
-  foreach (\Drupal::languageManager()->getDefinedLanguageTypes() as $type) {
-    $last[$type] = language($type)->id;
+  if (\Drupal::moduleHandler()->moduleExists('language')) {
+    $last = array();
+    foreach (\Drupal::languageManager()->getDefinedLanguageTypes() as $type) {
+      $last[$type] = language($type)->id;
+    }
+    \Drupal::state()->set('language_test.language_negotiation_last', $last);
   }
-  \Drupal::state()->set('language_test.language_negotiation_last', $last);
}

/**

plach’s picture

StatusFileSize
new39.94 KB
new365.63 KB
FAILED: [[SimpleTest]]: [MySQL] 59,811 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

This should address the "easy" stuff of #341:

  • 1: Follow-up works for me.
  • 2: Done
  • 3: The most relevant resource I was able to find on this matter is http://www.w3.org/International/questions/qa-no-language but it does not mention a generic name for this type of laguage codes. In the IANA subtag registry they call them "special" (see http://www.iana.org/assignments/language-subtag-registry/language-subtag...).
  • 4: Done
  • 7: Done, see #348
  • 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?

Berdir’s picture

+++ b/core/modules/language/lib/Drupal/language/LanguageNegotiationMethodInterface.php
@@ -53,4 +54,11 @@ public function setCurrentUser(AccountInterface $current_user);
+  /**
+   * Notifies the plugin that the language code it returned has been accepted.
+   *
+   * @param string $langcode
+   */
+  public function persist(Language $language);
+

+++ b/core/modules/language/lib/Drupal/language/LanguageNegotiationMethodBase.php
@@ -57,4 +58,12 @@ public function setCurrentUser(AccountInterface $current_user) {
+  /**
+   * {@inheritdoc}
+   */
+  public function persist(Language $language) {
+    // Remember the method ID used to detect the language.
+    $language->method_id = static::METHOD_ID;
+  }

@param type doesn't match and missing description. I guess $anguage is used in some of the subclasses?

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new1.39 KB
new366.37 KB
PASSED: [[SimpleTest]]: [MySQL] 59,841 pass(es).
[ View ]

This should fix failures and #356.

I guess $anguage is used in some of the subclasses?

Yup, in the session plugin.

webchick’s picture

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.

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

The immediate resolutions of @sun's concerns look good.

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.

catch’s picture

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

I'd personally like to see this in as soon as possible, and we can open clear follow-ups for anything remaining.

penyaskito’s picture

RTBC++

However, if it was rerolled, please fix the typo:

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php
@@ -113,19 +121,16 @@ function testUninstallProcess() {
+      $message = 'Language negotiation is not avilable.';

Looking forward to see this one land ASAP.

plach’s picture

StatusFileSize
new774 bytes
new366.37 KB
PASSED: [[SimpleTest]]: [MySQL] 59,792 pass(es).
[ View ]

Rerolled and fixed #362.

FWIW I am ok with committing this and opening follow-ups for the rest.

plach’s picture

Issue summary:View changes
sun’s picture

Thanks for taking my concerns into account and also for the elaborate replies.

  1. I'm not sure whether I agree with @alexpott's changes in #348:

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

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

  3. #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 :)

  4. #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.

  5. #341.16: [on not injecting config + dependencies to NegotiatorPlugin::negotiate() methods]

    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:

    1. Currently the CMI keys are named a bit randomly, so it's not easy to automatically map a plugin to its configuration.
    2. Some plugins may need to access the other plugins' configuration, for instance the URL fallback needs that.
    3. 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. :)

webchick’s picture

Assigned:plach» catch

Though I'm loathe to put more things on catch's plate, it really does seem like he's been the most involved here.

catch’s picture

Status:Reviewed & tested by the community» Needs work

Was about to commit this, but it no longer applies.

plach’s picture

I will reroll it in a few minutes

plach’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new366.39 KB
PASSED: [[SimpleTest]]: [MySQL] 59,884 pass(es).
[ View ]

 

plach’s picture

Issue summary:View changes

@catch:

Suggested commit message:

Issue #1862202 by plach, Berdir, katbailey, ParisLiakos, alexpott, chx, sun, larowlan, Gábor Hojtsy, cosmicdreams, vijaycs85, YesCT, penyaskito, andypost, Albert Volkman, joelpitett: Objectify the language system.
catch’s picture

Title:Objectify the language system» Change notice: Objectify the language system
Priority:Critical» Major
Status:Reviewed & tested by the community» Active

Committed/pushed to 8.x, thanks!

plach’s picture

Whoo-hoo!

Thanks to everybody helping here :)

plach’s picture

Status:Active» Needs review

Here is the change notice based on the draft provided by @jessebeach (thanks!):

https://drupal.org/node/2174591

plach’s picture

@sun:

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.

I created these follow-ups:

Gábor Hojtsy’s picture

Title:Change notice: Objectify the language system» Objectify the language system
Assigned:catch» Unassigned
Priority:Major» Critical
Status:Needs review» Fixed
Issue tags:-sprint

The change notice looks good and comprehensive.

effulgentsia’s picture

Great work, everyone!

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

znerol’s picture

YesCT’s picture

Issue summary:View changes

caused #2240007-4: Regression: early installer is not in RTL after selecting RTL language

tried to categorize the relates/follow-ups in the issue summary.
note there is still a remaining task to check if all the follow-ups have been opened.