Problem

  • PathSubscriber::onKernelRequestLanguageResolve() cannot be converted to a path processor, because it relies on a side-effect of language initialization.
  • The language manager is tied to the request scope.

Goal

  • Split language initialization and path resolution into two separate processes.

Details

  • PathSubscriber::onKernelRequestLanguageResolve() currently shows this:

    <?php
     
    /**
       * Decode language information embedded in the request path.
       *
       * @todo Refactor this entire method to inline the relevant portions of
       *   drupal_language_initialize(). See the inline comment for more details.
       *
       * @param Symfony\Component\HttpKernel\Event\GetResponseEvent $event
       *   The Event to process.
       */
     
    public function onKernelRequestLanguageResolve(GetResponseEvent $event) {
       
    // drupal_language_initialize() combines:
        // - Determination of language from $request information (e.g., path).
        // - Determination of language from other information (e.g., site default).
        // - Population of determined language into drupal_container().
        // - Removal of language code from _current_path().
        // @todo Decouple the above, but for now, invoke it and update the path
        //   prior to front page and alias resolution. When above is decoupled, also
        //   add 'langcode' (determined from $request only) to $request->attributes.
       
    drupal_language_initialize();
      }
    ?>
  • This only works, because, at the time the listener is called, the container is within request scope.

  • Thus, when drupal_language_initialize() calls language(), it uses the LanguageManager rather than language_default().

  • The LanguageManager, which depends on the request, passes the request on to language negotiation functions. One of them is URL-based. That callback performs this:

    <?php
          $current_path
    = $request->attributes->get('system_path');
          if (!isset(
    $current_path)) {
           
    $current_path = trim($request->getPathInfo(), '/');
          }
          list(
    $language, $path) = language_url_split_prefix($current_path, $languages);
         
    // Store the correct system path, i.e. minus the path prefix, in the
          // request.
         
    $request->attributes->set('system_path', $path);
    ?>
  • There is no good reason for binding the language manager to the request. It was originally introduced with #1599108: Allow modules to register services and subscriber services (events).

Proposed solution

  1. Make the LanguageManager independent of request scope. It is always responsible for language initialization, even if there is no request.

  2. Add an event listener that sets the request on the LanguageManager, in order to perform request-aware language initialization when there is a request.

  3. Convert all path listeners (the methods in the PathSubscriber class) to path processors, which are called in sequence by a single listener (#1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence).

Notes

  • The language-aware path resolution still needs to happen in the onKernelRequestLanguageResolve() listener (i.e. the process whereby e.g. 'fr/my/path' gets converted to 'my/path', which in turn is set as the 'system_path' request attribute).

  • When moving language initialization into its own request listener, we need a (new) way to retrieve the 'system_path' request attribute.

  • The proper solution for this is to make Language module provide a "PathFutzer" class that we can call a method on.

    That's done in #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence. We need a temporary solution, which just stores the language-derived path from language negotiation in a static variable, and we then retrieve it from there in the path listener.

    This is dirty, but only a temporary measure.

Related issues

Files: 
CommentFileSizeAuthor
#48 1899994.disentangle-language-init.48.patch3.29 KBplach
PASSED: [[SimpleTest]]: [MySQL] 49,383 pass(es).
[ View ]
#41 1899994.disentangle-language-init.42.patch3.24 KBplach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1899994.disentangle-language-init.42.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#41 1899994.disentangle-language-init.42.interdiff.do_not_test.patch1.49 KBplach
#41 1899994.disentangle-language-init.42.test.patch2.25 KBplach
FAILED: [[SimpleTest]]: [MySQL] 49,297 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#35 1899994.disentangle-language-init.35.patch3.17 KBplach
PASSED: [[SimpleTest]]: [MySQL] 49,033 pass(es).
[ View ]
#35 1899994.disentangle-language-init.35.test.patch2.33 KBplach
FAILED: [[SimpleTest]]: [MySQL] 49,043 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#32 1899994.disentangle-language-init.32.interdiff.do_not_test.patch1.68 KBplach
#32 1899994.disentangle-language-init.32.patch36.05 KBplach
PASSED: [[SimpleTest]]: [MySQL] 48,716 pass(es).
[ View ]
#31 1899994.disentangle-language-init.31.patch36.04 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 48,672 pass(es).
[ View ]
#31 interdiff.txt2.57 KBkatbailey
#25 1899994.disentangle-language-init.25.patch35.22 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 48,623 pass(es).
[ View ]
#25 interdiff-22-25.txt6.96 KBYesCT
#22 1899994.disentangle-language-init.22.interdiff.do_not_test.patch711 bytesplach
#22 1899994.disentangle-language-init.22.patch35.04 KBplach
PASSED: [[SimpleTest]]: [MySQL] 48,629 pass(es).
[ View ]
#15 1899994.disentangle-language-init.15.patch35.04 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 48,629 pass(es).
[ View ]
#15 interdiff.txt6.73 KBkatbailey
#12 1899994.disentangle-language-init.12.patch33.8 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 49,569 pass(es).
[ View ]
#12 interdiff.txt6.01 KBkatbailey
#9 1899994.disentangle-language-init.9.patch28.82 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 49,276 pass(es), 3 fail(s), and 30 exception(s).
[ View ]
#9 interdiff.txt5.62 KBkatbailey
#1 1899994.disentangle-language-init.patch27.6 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 49,194 pass(es), 33 fail(s), and 76 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new27.6 KB
FAILED: [[SimpleTest]]: [MySQL] 49,194 pass(es), 33 fail(s), and 76 exception(s).
[ View ]

This patch includes work originally started in #1862202: Objectify the language system and then continued in #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence - it represents the changes that are required by both of those issues.

Status:Needs review» Needs work

The last submitted patch, 1899994.disentangle-language-init.patch, failed testing.

I've taken the freedom to completely rewrite the issue summary, in order to understand the proposal. Please correct where I misunderstood it, thanks.

Issue summary:View changes

Rewrite.

I guess my original summary was a little rambling :-P Thanks for restructuring it - I just made a few corrections here and there.

Just to clarify: are you suggesting that if the LanguageManager has to initialize a language type without having a request object set, the request-dependent language detection methods (URL, Browser, Session) would just bail out and return FALSE, leaving the language negotiation process proceed happily?

@plach with this patch the language negotiation methods will still only get called when there is a request, it's just that the logic about whether to call them gets moved out of the language() function in bootstrap.inc and into the LanguageManager itself. If there are language negotiation methods that are not dependent on the request then we can rework the logic of that in the other issue once this first step has been completed.

I am no expert on the language system, but in my brief read through this seems overall sane, at least enough to unblock the two related issues.

There is probably only one negotiation method in core that has internal configuration and is independent of the request (the "Selected language" method, which returns a pre-selected language set up by the site admin). So the negotiation process as a whole cannot really be trusted to return the right value if there is no information about the request.

Status:Needs work» Needs review
StatusFileSize
new5.62 KB
new28.82 KB
FAILED: [[SimpleTest]]: [MySQL] 49,276 pass(es), 3 fail(s), and 30 exception(s).
[ View ]

Making very slow progress on these test failures :-/ Still have no idea why the simpletest test is exploding - @sun, if you get a chance to look at that, it might be something really obvious to you.

It looks like we don't need to pass config to the language manager at all (I think the config reset() call was only needed in drupal_language_initialize() because of the hook_language_init invocation, and the only implementation of that was locale.module using it to add a config event subscriber... I converted that to a bundle), so that's good at least, unless testbot proves otherwise.

Status:Needs review» Needs work

The last submitted patch, 1899994.disentangle-language-init.9.patch, failed testing.

Oh yay - the simpletest test got fixed by one of the other fixes - progress not as slow as I had thought :-)

I should explain the fix I did for the locale config override test though as it looks like a total cheat (just remove the failing assertion - done! :-D)
The patch removes this hook_language_init implementation from locale.module (and in fact gets rid of hook_language_init entirely as this was the only implementation in core):

+++ b/core/modules/locale/locale.moduleundefined
@@ -1303,11 +1303,3 @@ function _locale_rebuild_js($langcode = NULL) {
-/**
- * Implements hook_language_init().
- */
-function locale_language_init() {
-  // Add locale helper to configuration subscribers.
-  drupal_container()->get('event_dispatcher')->addSubscriber(new LocaleConfigSubscriber());
-}

... and adds instead a LocaleBundle where the event subscriber gets registered. This means we don't need to explicitly call drupal_language_initialize() for the override to take effect. The assertion I removed was just checking the pre-overridden config setting, then calling drupal_language_initialize() and checking the overridden value. I think all this test needs to care about is that final assertion, so I removed the rest.

+++ b/core/modules/config/lib/Drupal/config/Tests/LocaleConfigOverride.phpundefined
@@ -33,10 +33,6 @@ function testLocaleConfigOverride() {
-    $this->assertIdentical($config->get('foo'), 'bar');
-    // Spoof multilingual.
-    $GLOBALS['conf']['language_count'] = 2;
-    drupal_language_initialize();
     $this->assertIdentical($config->get('foo'), 'en bar');

Does anyone see any problem with that?

Status:Needs work» Needs review
StatusFileSize
new6.01 KB
new33.8 KB
PASSED: [[SimpleTest]]: [MySQL] 49,569 pass(es).
[ View ]

This hopefully gets us green. The fix for the DateUpgradePath test probably stands in need in an explanation: basically, because the locale config override now actually works, i.e. without having to explicitly call drupal_language_initalize(), the 'en' config is used when the formats are looked up. So I'm changing the test to expect the 'en' formats, and there's no need to include 'en' later on so we just check the 'de' formats.

Sounds great :)

Will have a look to this ASAP.

Status:Needs review» Needs work

Overall this looks great to me. Probably we should ask @Jose Reyero to have a look to the config overrides stuff.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -2,52 +2,145 @@
+    if ($this->getLanguageCount() > 1) {

Can we have an isMultilingual() method instead? We usually have no interest in knowing how many languages are installed.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -2,52 +2,145 @@
+  /**
+   * Sets the $request property and resets all language types.
+   */
+  public function setRequest(Request $request) {
...
+  /**
+   * Returns a language object for the given type.
+   */
   public function getLanguage($type) {
     if (isset($this->languages[$type])) {
...
+  /**
+   * Resets the given language type or all types if none specified.
+   */
...
+  /**
+   * Returns the number of currently enabled langauges.
+   */
+  protected function getLanguageCount() {
+    return variable_get('language_count', 1);
+  }
+
+  /**
+   * Returns the array of language types.
+   */
+  protected function getLanguageTypes() {
+    return array_keys(variable_get('language_types', language_types_get_default()));
+  }
+
+  /**
+   * Returns an array of information representing the current default language.
+   */
+  protected function getLanguageDefault() {
+    return variable_get('language_default', array(
+      'langcode' => 'en',
+      'name' => 'English',
+      'direction' => 0,
+      'weight' => 0,
+      'locked' => 0,
+    ));
+  }

Missing @params/@returns.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -2,52 +2,145 @@
+  /**
+   * @todo Inject state once these variables have been converted to use the
+   *  state system, then remove these methods and replace calls to them with
+   *  $this->state->get('language_count') etc.

I'm not sure the two variables below are state: it's true that they can be derived, but it's also true that they aren't derived automatically. Hence they won't be updated by a config deployment.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -2,52 +2,145 @@
+        return new Language($this->getLanguageDefault() + array('default' => TRUE));
...
+      $this->languages[$type] = new Language($this->getLanguageDefault() + array('default' => TRUE));

Why $default = TRUE is not part of the array returned by getLanguageDefault? This is not consistent with what language_default() returns. I guess we should just return a Language object btw.

+++ b/core/lib/Drupal/Core/Path/AliasManager.php
@@ -78,11 +79,12 @@ class AliasManager implements AliasManagerInterface {
-    $this->state = $keyvalue->get('state');
-    $this->langcode = language(LANGUAGE_TYPE_URL)->langcode;
+    $this->state = $state;

What about lazy-initializing $this->langcode instead of repeating twice all the langcode resolution code below? Do we expect the value returned by the language manager to change?

+++ b/core/modules/language/language.negotiation.inc
@@ -209,11 +209,12 @@ function language_from_user($languages) {
  * @return
  *   A valid language code on success, FALSE otherwise.
  */
-function language_from_user_admin($languages) {
+function language_from_user_admin($languages, $request = NULL) {
   // User preference (only for authenticated users).

PHP docs need to be updated.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php
@@ -56,8 +56,8 @@ function testUninstallProcess() {
     drupal_language_initialize();
     $this->assertEqual(language(LANGUAGE_TYPE_INTERFACE)->langcode, $this->langcode, t('Current language: %lang', array('%lang' => language(LANGUAGE_TYPE_INTERFACE)->langcode)));

Shouldn't we call LanguageManager::init() here?

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UpgradePathTestBase.php
@@ -126,6 +126,13 @@ protected function setUp() {
+    // Load roles for the user object.
+    $roles = array(DRUPAL_AUTHENTICATED_RID => DRUPAL_AUTHENTICATED_RID);
+    $result = db_query('SELECT rid, uid FROM {users_roles} WHERE uid = :uid', array(':uid' => 1));
+    foreach ($result as $record) {
+      $roles[$record->rid] = $record->rid;
+    }
+    $user->roles = $roles;

Why is this needed?

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -2,52 +2,145 @@
+  /**
+   * Returns the number of currently enabled langauges.

Typo

Status:Needs work» Needs review
StatusFileSize
new6.73 KB
new35.04 KB
PASSED: [[SimpleTest]]: [MySQL] 48,629 pass(es).
[ View ]

Thanks for the review, @plach! I think I've dealt with everything you brought up, except for:

What about lazy-initializing $this->langcode instead of repeating twice all the langcode resolution code below? Do we expect the value returned by the language manager to change?

Yes, the language manager could get its $request property set on it after the first use of the alias manager.

Re the user roles stuff in UpgradePathTestBase - dammit, I was hoping nobody would ask about that one :-P Let's see if I can come up with a coherent explanation. The global user normally has the roles property set, and this matters for things like the LocaleLookup class, which uses the current user's role IDs as part of the cache key. Without this patch, the particular code path that leads to this roles-property-less user object being erroneously assumed to have a roles property by the LocaleLookup class simply never happens - and that's just an accident. Because we're slightly changing the code flow for language initialization, we expose this problem with the test, so I'm fixing it by adding the expected roles property.

Status:Needs review» Needs work
Issue tags:-D8MI, -language-base

The last submitted patch, 1899994.disentangle-language-init.15.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1899994.disentangle-language-init.15.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1899994.disentangle-language-init.15.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+D8MI, +language-base

StatusFileSize
new35.04 KB
PASSED: [[SimpleTest]]: [MySQL] 48,629 pass(es).
[ View ]
new711 bytes

Rerolled to fix the minor thing below:

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -2,52 +2,164 @@
+   *   The language type to reset, e.g. LANGUAGE_TYPE_INTERFACE, or NULL to reset

Comment not wrapping at column 80.

+++ b/core/lib/Drupal/Core/EventSubscriber/LanguageRequestSubscriber.php
@@ -0,0 +1,62 @@
+    if ($event->getRequestType() == HttpKernelInterface::MASTER_REQUEST) {

First of all I should say this looks ready to go to me. I was just wondering whether this check is actually needed: if we removed it we would have language negotiation for each subrequest, correct? If it's not a hard thing to achieve it would be a really nice feature as it would allow us to have a different language for each block. This would be really useful for instance to have the administrative toolbar always being displayed in the admin language instead of following the UI language which a site builder might not know.
This can totally be a follow-up, obviously :)

Btw, it seems Jose won't be available to review this for a while, hence I went ahead and manually tried the changes to locale config overrides and everything was looking good. Very nice work!

Just a coding style review. Some of these are just nits, (patch fixes them). Went ahead and did it since there were at least a few @param 's that would have to have been fixed anyway to meet the core gate of having types. http://drupal.org/core-gates#documentation-block-requirements

+++ b/core/includes/bootstrap.incundefined
@@ -2744,73 +2744,29 @@ function get_t() {
+    // This happens in rare situations when the container has not been built by
+    // a kernel and has no services e.g. when t() is called during unit tests for
+    // assertions.

This was over 80 chars, and needs a comma before the for example (e.g.) anyway.

+++ b/core/includes/bootstrap.incundefined
@@ -3016,6 +2972,21 @@ function language_default() {
+ * Store or retrieve the path derived during language negotiation.

Needs to be third person verb.

http://drupal.org/node/1354#classes

+++ b/core/lib/Drupal/Core/EventSubscriber/LanguageRequestSubscriber.phpundefined
@@ -0,0 +1,62 @@
+ * @file
+ * Contains Drupal\Core\EventSubscriber\LanguageRequestSubscriber.

Contains \Drupal

http://drupal.org/node/1354#namespaces

+++ b/core/lib/Drupal/Core/EventSubscriber/LanguageRequestSubscriber.phpundefined
@@ -0,0 +1,62 @@
+/**
+ * Request subscriber to set the $request proprerty on the language manager.
+ */
+class LanguageRequestSubscriber implements EventSubscriberInterface {

Needs to start with third person verb.

+++ b/core/lib/Drupal/Core/EventSubscriber/LanguageRequestSubscriber.phpundefined
@@ -0,0 +1,62 @@
+   * The language manager service
+   *
+   * @var \Drupal\Core\Language\LanguageManager
+   */
+  protected $languageManager;

Should end with a period.

+++ b/core/lib/Drupal/Core/EventSubscriber/LanguageRequestSubscriber.phpundefined
@@ -0,0 +1,62 @@
+  }
+}

classes have a empty line before the closing brace.
http://drupal.org/node/1354#classes

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -2,52 +2,164 @@
+ * Contains Drupal\Core\Language\LanguageManager.

If going to change to Contains, might as well change to Contains \Dr..

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -2,52 +2,164 @@
+   * @var boolean

type key word is bool
http://drupal.org/node/1354#functions

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -2,52 +2,164 @@
+   * Whether already in the process of language initialization.
+   *
+   * @todo This is only needed due to the circular dependency between language
+   *   and config. See http://drupal.org/node/1862202 for the plan to fix this.

was the @var left out on purpose? I'm guessing not. so adding in.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -2,52 +2,164 @@
+        // @todo Objectify the language system so that we don't have to do load
+        //   an include file and call out to procedural code. See

Reads strange with the "to do load an"

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -2,52 +2,164 @@
+   * @param string $type
+   *   The language type to reset, e.g. LANGUAGE_TYPE_INTERFACE, or NULL to
+   *   reset all language types.
+   */
   public function reset($type = NULL) {

add (optional) and |null in the type.
http://drupal.org/node/1354#functions

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -2,52 +2,164 @@
+   * @return boolean
+   *   TRUE if more than one language is enabled, FALSE otherwise.

bool keyword instead of boolean

+++ b/core/modules/language/language.negotiation.incundefined
@@ -206,14 +206,18 @@ function language_from_user($languages) {
+ * @param $request
+ *   The HttpRequest object representing the current request.
...
+function language_from_user_admin($languages, $request = NULL) {

needs type and (optional)

I had a hard time finding the type.

Q1

Is it:
\Symfony\Component\HttpFoundation\Request
?

+++ b/core/modules/language/language.negotiation.incundefined
@@ -273,15 +277,11 @@ function language_from_url($languages, $request) {
+      // Store the correct system path, i.e. minus the path prefix.

grammar of using i.e. (in other words) is a bit confusing, but I think it would be i.e.,
http://grammar.quickanddirtytips.com/ie-eg-oh-my.aspx

+++ b/core/modules/locale/lib/Drupal/locale/LocaleBundle.phpundefined
@@ -0,0 +1,28 @@
+ * @file
+ * Contains LocaleBundle

This is a bit strange to see a name space so simple.

Should be Contains \Drupal\locale\LocaleBundle.

(and with a period)

+++ b/core/modules/locale/lib/Drupal/locale/LocaleBundle.phpundefined
@@ -0,0 +1,28 @@
+ * Bundle for registering locale module's services to the container.

Registers ...

Patch coming right up.

StatusFileSize
new6.96 KB
new35.22 KB
PASSED: [[SimpleTest]]: [MySQL] 48,623 pass(es).
[ View ]

+++ b/core/modules/language/language.negotiation.incundefined
@@ -273,15 +278,11 @@ function language_from_url($languages, $request) {

there was no change to the function definition. But I noticed that the request parameter is optional. So it could be a separate issue to correct the @param's for language_from_url().

Status:Needs review» Needs work
Issue tags:-D8MI, -language-base

The last submitted patch, 1899994.disentangle-language-init.25.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+D8MI, +language-base

Minor nits on the nits, to be fixed on the next reroll.

+++ b/core/lib/Drupal/Core/EventSubscriber/LanguageRequestSubscriber.php
@@ -14,12 +14,12 @@
+ * Sets the $request proprerty on the language manager.

Typo

+++ b/core/modules/language/language.negotiation.inc
@@ -206,13 +206,14 @@ function language_from_user($languages) {
+function language_from_user_admin(array $languages, \Symfony\Component\HttpFoundation\Request $request = NULL) {

Let's use a "use" statement, please.

+++ b/core/modules/language/language.negotiation.inc
@@ -280,7 +281,7 @@ function language_from_url($languages, $request) {
-      // Store the correct system path, i.e. minus the path prefix.
+      // Store the correct system path, i.e., minus the path prefix.

This does not seems correct to me, at least in italian it wouldn't ;)

Oops!

In the improve comment on entity ng issue, it was recommended to just say "for example" instead of eg. We could do that.

How about (wrapped at 80 chars):
+ // Store the correct system path, in other words, the path minus the path prefix.

StatusFileSize
new2.57 KB
new36.04 KB
PASSED: [[SimpleTest]]: [MySQL] 48,672 pass(es).
[ View ]

if we removed it we would have language negotiation for each subrequest, correct?

The problem is we'd have no way to set the request back at the end of a subrequest because there's no event we can listen to for that. I brought this up over in #1862202-36: Objectify the language system. Apparently fabpot is working on a potential solution: https://github.com/symfony/symfony/issues/5300

Re the $request param in language_from_url(), I've made it optional as well, just for consistency - although the fact is this function will never get called without a request (same goes for language_from_user_admin()). So I was nearly going to make it non-optional for both of them, but then it seemed that would be inconsistent with what happens in language_negotiation_method_invoke() - that's where these functions get called from and *its* request param is optional. And I don't want to go messing with that code in this patch - we'll figure it out in #1862202: Objectify the language system.

As for the use of "i.e." it is perfectly correct here, but this comment is part of the temporary hack that we're removing in #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence so it's not worth worrying about anyway.

Fixed the other things mentioned in #29.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new36.05 KB
PASSED: [[SimpleTest]]: [MySQL] 48,716 pass(es).
[ View ]
new1.68 KB

Sorry, I missed that one. Aside from the minor things below, which I fixed myself, this looks awesome, thanks!

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -2,52 +2,168 @@
+use Drupal\Core\Config\ConfigFactory;

Unused "use" statement (we can add it back when injecting configuration in the other issue).

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -2,52 +2,168 @@
+   * Returns a langauge object representing the site's default language.

Typo.

+++ b/core/modules/language/language.negotiation.inc
@@ -257,31 +264,28 @@ function language_from_session($languages) {
+ *   (optional) The HttpRequest object representing the current request.

Missing "Defaults to".

+++ b/core/includes/bootstrap.inc
@@ -3016,6 +2972,21 @@ function language_default() {
+function _language_resolved_path($new_path = NULL) {

Missing @param.

interdiff looks good.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks!

Assigned:katbailey» Unassigned
Category:task» bug
Status:Fixed» Needs review
StatusFileSize
new2.33 KB
FAILED: [[SimpleTest]]: [MySQL] 49,043 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new3.17 KB
PASSED: [[SimpleTest]]: [MySQL] 49,033 pass(es).
[ View ]

Small follow-up: subrequests are now broken when using URL language negotiation.

Status:Needs review» Needs work

The last submitted patch, 1899994.disentangle-language-init.35.test.patch, failed testing.

Status:Needs work» Needs review

So in the patch that was committed, this is what seems to have been targeted to avoid this problem:

<?php
 
public function onKernelRequestLanguage(GetResponseEvent $event) {
    if (
$event->getRequestType() == HttpKernelInterface::MASTER_REQUEST) {
     
$this->languageManager->setRequest($event->getRequest());
    }
  }
?>

Is that not working (at the wrong place, should be removed?) or not enough? Adding comments there and in your added code would be useful to explain the situation.

The reason is this one:

The problem is we'd have no way to set the request back at the end of a subrequest because there's no event we can listen to for that. I brought this up over in #1862202-36: Objectify the language system.

We basically need the same check also when updating the request path after removing prefixes, otherwise subrequests will inherit the main request path and an infinite loop will be started. Will add a comment about this ASAP.

Anyway, this should be lo longer needed once #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence is committed. That one will only let us split the prefix out in a cleaner way.

Yes, good catch. The fix looks fine to me, just noticed one small thing in the test:

+++ b/core/modules/language/tests/language_test.moduleundefined
@@ -106,3 +109,27 @@ function language_test_store_language_negotiation() {
+  $request = drupal_container()->get('request');

This is not used.

StatusFileSize
new2.25 KB
FAILED: [[SimpleTest]]: [MySQL] 49,297 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.49 KB
new3.24 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1899994.disentangle-language-init.42.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And here it is.

This is not used.

Shameless copy paste gone wrong ;)

Status:Needs review» Reviewed & tested by the community

Assuming testbot approves, this is good to go.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1899994.disentangle-language-init.42.test.patch, failed testing.

Status:Needs work» Reviewed & tested by the community

Issue tags:+Needs change record

This issue removed hook_language_init().

+1 to RTBC :)

StatusFileSize
new3.29 KB
PASSED: [[SimpleTest]]: [MySQL] 49,383 pass(es).
[ View ]

Rerolled + run tests locally.

Status:Reviewed & tested by the community» Fixed

Thanks for the re-roll, committed/pushed to 8.x.

Title:Disentangle language initialization from path resolutionChange notice: Disentangle language initialization from path resolution
Category:bug» task
Priority:Normal» Critical

Thanks!

Status:Fixed» Active

Status:Active» Needs review

Here is a change record: http://drupal.org/node/1911596.

@plach: any way to hook into after the language process in Drupal 8 then? The changelog only says this was removed, period not what people can/should use instead.

I don't think there was a real use case for that, aside from variable localization. Moreover now language is initialized as soon as it is needed so there is no bootstrap phase to hook into.

Title:Change notice: Disentangle language initialization from path resolutionDisentangle language initialization from path resolution
Category:task» bug
Priority:Critical» Normal
Issue tags:-Needs change record

Ok, added this note: "There is no facility anymore to hook into the end of language initialisation.".

Looks like as far as languages go, we should go back to #1862202: Objectify the language system on this track unearthing @chx's initial patch and going from there :)

Category:bug» task
Status:Needs review» Fixed

With the change notice in place we can call this fixed.

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

Issue summary:View changes

Some minor corrections.