Support from Acquia helps fund testing for Drupal Acquia logo

Comments

brianV’s picture

Assigned: Unassigned » brianV

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

brianV’s picture

Status: Active » Needs review
FileSize
24.74 KB

First pass

brianV’s picture

With whitespace fixes dreditor hinted at.

brianV’s picture

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
FileSize
23.62 KB

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
FileSize
9.39 KB
25.04 KB

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
FileSize
905 bytes
25.04 KB

:/

Status: Needs review » Needs work

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

disasm’s picture

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

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
FileSize
20.24 KB

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.