Follow up for #1833022: Only display interface language detection options to customize more granularity

Updated: Comment #0

Problem/Motivation

Reading language_types_set() is still a bit confusing.

Proposed resolution

Improve the comments, add a has_default_settings variable

Remaining tasks

  • get feedback
  • get tests to pass

User interface changes

No.

API changes

No.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Status: Active » Needs review
FileSize
4.7 KB

With ideas from @plach @e0ipso and @kfritsche and lots of discussion about what the purpose is and how it works.

YesCT’s picture

Category: task » bug
+++ b/core/includes/language.incundefined
@@ -215,30 +219,37 @@ function language_types_set(array $configurable_language_types) {
-      // If we have a non-locked non-configurable language type without default
-      // language negotiation settings, we use the values negotiated for the
-      // interface language which should always be available.
-      if (!$configurable && !empty($info['fixed'])) {
+      if (!$configurable && !$has_default_settings) {
+        // If we have an unlocked non-configurable language type without default
+        // language negotiation settings, we use the values negotiated for the
+        // interface language which, should always be available.

Actually, I think this ! was a bug. And that's why there are changes to the tests.

Locally, what seems like an unrelated test is failing because a page is now access denied and the test was expecting 404 page not found. (for a page with an unknown langcode prefix in the url).

LanguageUILanguageNegotiationTest around line 237

// Unknown language prefix should return 404.
    variable_set('language_negotiation_' . Language::TYPE_INTERFACE, language_language_negotiation_info());
    $this->drupalGet("$langcode_unknown/admin/config", array(), $http_header_browser_fallback);
    $this->assertResponse(404, "Unknown language path prefix should return 404");
jhedstrom’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +needs-reroll
Gábor Hojtsy’s picture

Title: Improve code organization in language_types_set() » Improve code organization in LanguageNegotiator::updateConfiguration()
Issue tags: -medium, - +Novice

Updated the title based on where this code is now.

balagan’s picture

Assigned: Unassigned » balagan
balagan’s picture

I did not touch the /core/modules/language/lib/Drupal/language/Tests/LanguageNegotiationInfoTest.php file, it seems the changes in the patch in comment #1 are already there.

balagan’s picture

Assigned: balagan » Unassigned
Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/language/src/LanguageNegotiator.php
    @@ -322,30 +322,36 @@ function updateConfiguration(array $types) {
    +      // The default language negotiation settings, if
           // available, are stored in $info['fixed'].
    

    Should be wrapped so as much is on one line as possible.

  2. +++ b/core/modules/language/src/LanguageNegotiator.php
    @@ -322,30 +322,36 @@ function updateConfiguration(array $types) {
    +      // Check whether the language type is unlocked. Only the status of unlocked
    +      // language types can be toggled between configurable and non-configurable.
    ...
    +          // If we have an unlocked non-configurable language type without default
    ...
    +        // to be toggled between configurable and non-configurable, but that might
    

    These lines are exceed 80 chars.

  3. +++ b/core/modules/language/src/LanguageNegotiator.php
    @@ -322,30 +322,36 @@ function updateConfiguration(array $types) {
    +        // to be toggled between configurable and non-configurable, but that might
    +        // need to be overridden.
    

    What does this mean? Not sure this comment clarifies the code? The later logic explains what happens when, not sure this lead-in helps.

balagan’s picture

Broke the comments to be under 80 chars, and removed the ambiguous comment in 3. Not sure if I should write anything else, I have just copied text from patch in comment #1

balagan’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. I agree it is an improvement in some ways. Even if comments are too verbose here and there, it is clearly an improvement to me. Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I've spent time staring at and trying to grok this code - this is definitely better. Committed b971954 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/language/src/LanguageNegotiator.php b/core/modules/language/src/LanguageNegotiator.php
index 9cb6161..af2d952 100644
--- a/core/modules/language/src/LanguageNegotiator.php
+++ b/core/modules/language/src/LanguageNegotiator.php
@@ -322,8 +322,8 @@ function updateConfiguration(array $types) {
     foreach ($language_types_info as $type => $info) {
       $configurable = in_array($type, $types);
 
-      // The default language negotiation settings, if available, are stored
-      // in $info['fixed'].
+      // The default language negotiation settings, if available, are stored in
+      // $info['fixed'].
       $has_default_settings = !empty($info['fixed']);
       // Check whether the language type is unlocked. Only the status of
       // unlocked language types can be toggled between configurable and
@@ -340,11 +340,10 @@ function updateConfiguration(array $types) {
         }
       }
       else {
-        // The language type is locked.
-
-        // Locked language types with default settings are always considered
-        // non-configurable. In turn if default settings are missing, the
-        // language type is always considered configurable.
+        // The language type is locked. Locked language types with default
+        // settings are always considered non-configurable. In turn if default
+        // settings are missing, the language type is always considered
+        // configurable.
 
         // If the language type is locked we can just store its default language
         // negotiation settings if it has some, since it is not configurable.

I've reflowed a couple of comments.

  • alexpott committed b971954 on 8.0.x
    Issue #2033983 by balagan, YesCT: Improve code organization in...
Gábor Hojtsy’s picture

Yay, thanks all!

Status: Fixed » Closed (fixed)

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