Files: 
CommentFileSizeAuthor
#25 2086485-25.patch20.24 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 60,117 pass(es).
[ View ]
#19 drupal8.language-module.2086485-19.patch25.32 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 59,123 pass(es), 57 fail(s), and 0 exception(s).
[ View ]
#19 interdiff.txt5.44 KBjibran
#16 drupal8.language-module.2086485-16.patch25.04 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,778 pass(es).
[ View ]
#16 interdiff.txt907 bytesdisasm
#14 drupal8.language-module.2086485-14.patch25.04 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 58,805 pass(es), 6 fail(s), and 2 exception(s).
[ View ]
#14 interdiff.txt905 bytesjibran
#12 drupal8.language-module.2086485-12.patch25.04 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 59,175 pass(es), 0 fail(s), and 5 exception(s).
[ View ]
#12 interdiff.txt9.39 KBjibran
#6 drupal8.language-module.2086485-6.patch23.62 KBbrianV
PASSED: [[SimpleTest]]: [MySQL] 59,129 pass(es).
[ View ]
#4 drupal8.language-module.2086485-4.patch24.71 KBbrianV
FAILED: [[SimpleTest]]: [MySQL] 58,912 pass(es), 14 fail(s), and 4 exception(s).
[ View ]
#3 drupal8.language-module.2086485-3.patch24.74 KBbrianV
FAILED: [[SimpleTest]]: [MySQL] 58,908 pass(es), 14 fail(s), and 4 exception(s).
[ View ]
#2 drupal8.language-module.2086485-2.patch24.74 KBbrianV
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

brianV’s picture

Assigned:Unassigned» brianV

I'll take a crack at this in the morning!

brianV’s picture

Status:Active» Needs review
StatusFileSize
new24.74 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

First pass

brianV’s picture

StatusFileSize
new24.74 KB
FAILED: [[SimpleTest]]: [MySQL] 58,908 pass(es), 14 fail(s), and 4 exception(s).
[ View ]

With whitespace fixes dreditor hinted at.

brianV’s picture

StatusFileSize
new24.71 KB
FAILED: [[SimpleTest]]: [MySQL] 58,912 pass(es), 14 fail(s), and 4 exception(s).
[ View ]

Sorry, one final niggling whitespace fix.

Status:Needs review» Needs work

The last submitted patch, drupal8.language-module.2086485-4.patch, failed testing.

brianV’s picture

Status:Needs work» Needs review
StatusFileSize
new23.62 KB
PASSED: [[SimpleTest]]: [MySQL] 59,129 pass(es).
[ View ]

This patch backs off the Local Tasks changeover as that's still in flux and was causing test failures.

The patch has two failures left that I can't figure out. Apparently, somehow, my changes to the language negotiation form controller cause the site to redirect incorrectly after the a different default language is selected on the regional settings page. It's not clear how changes to this route and controller are affecting that one.

Open to any ideas about these failures.

Status:Needs review» Needs work

The last submitted patch, drupal8.language-module.2086485-6.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review
brianV’s picture

jibran - thanks for updating the status.

It was inexplicably failing tests; now it works! Must have been a bad test or something that got fixed...

dawehner’s picture

  1. +++ b/core/modules/language/lib/Drupal/language/Form/NegotiationConfigureForm.php
    @@ -0,0 +1,241 @@
    +   * Implements \Drupal\Core\Form\FormInterface::getFormID().
    ...
    +   * Implements \Drupal\Core\Form\FormInterface::buildForm().
    ...
    +   * Implements \Drupal\Core\Form\FormInterface::submitForm().

    @inheritdoc

  2. +++ b/core/modules/language/lib/Drupal/language/Form/NegotiationConfigureForm.php
    @@ -0,0 +1,241 @@
    +    $configurable = \Drupal::config('system.language.types')->get('configurable');
    ...
    +    $stored_values = \Drupal::config('system.language.types')->get('configurable');

    Isn't there $this->config() available?

  3. +++ b/core/modules/language/lib/Drupal/language/Form/NegotiationConfigureForm.php
    @@ -0,0 +1,241 @@
    +    // Update non-configurable language types and the related language negotiation
    +    // configuration.
    ...
    +      // If there is an active language switcher for a language type that has been
    +      // made not configurable, deactivate it first.

    >80 chars.

  4. +++ b/core/modules/language/lib/Drupal/language/Form/NegotiationConfigureForm.php
    @@ -0,0 +1,241 @@
    +      \Drupal::service('plugin.manager.block')->clearCachedDefinitions();

    Is there a reason to not inject it?

  5. +++ b/core/modules/language/lib/Drupal/language/Form/NegotiationConfigureForm.php
    @@ -0,0 +1,241 @@
    +    drupal_set_message(t('Language negotiation configuration saved.'));

    All of the t() should be $this->t()

jibran’s picture

Status:Needs review» Needs work

NW as per #10.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new9.39 KB
new25.04 KB
FAILED: [[SimpleTest]]: [MySQL] 59,175 pass(es), 0 fail(s), and 5 exception(s).
[ View ]

Some fixes.

Status:Needs review» Needs work

The last submitted patch, drupal8.language-module.2086485-12.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new905 bytes
new25.04 KB
FAILED: [[SimpleTest]]: [MySQL] 58,805 pass(es), 6 fail(s), and 2 exception(s).
[ View ]

:/

Status:Needs review» Needs work

The last submitted patch, drupal8.language-module.2086485-14.patch, failed testing.

disasm’s picture

StatusFileSize
new907 bytes
new25.04 KB
PASSED: [[SimpleTest]]: [MySQL] 58,778 pass(es).
[ View ]

slight typo, this should fix it.

disasm’s picture

Status:Needs work» Needs review
disasm’s picture

+++ w/core/modules/language/lib/Drupal/language/Form/NegotiationConfigureForm.php
@@ -0,0 +1,296 @@
+  /**
+   * Stores the configuration configuration object for system.language.types.
+   *
+   * @var \Drupal\Core\Config\Config
+   */
+  protected $config;
...
+    $this->config = $config_factory->get('system.language.types');

I'd prefer to store the entire configFactory so if another module extends this, and needs a different config, they'd be able to get it still, without reinjecting the configFactory. If you aren't going to store the factory, at least specify what config this is instead of just $config.

jibran’s picture

StatusFileSize
new5.44 KB
new25.32 KB
FAILED: [[SimpleTest]]: [MySQL] 59,123 pass(es), 57 fail(s), and 0 exception(s).
[ View ]

Some fixes and tested manually works fine.

disasm’s picture

I can live with that. If it passes, RTBC.

disasm’s picture

disasm’s picture

Issue tags:+D8MI

just can't win...

Status:Needs review» Needs work

The last submitted patch, drupal8.language-module.2086485-19.patch, failed testing.

Hydra’s picture

Issue tags:+Needs reroll

Seems like the patch no longer applies

pwolanin’s picture

Issue summary:View changes
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new20.24 KB
PASSED: [[SimpleTest]]: [MySQL] 60,117 pass(es).
[ View ]

Here's a re-roll and minor tweaks around the handling of the block manager.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

I manually tested the page and it worked as expected. The code looks fine as well.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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