Migrate D6 and D7 language negotiation settings

working on this

This solution depends on #2222169: Add skip_row option to StaticMap to skip some rows without value.

This is a patch with the migration, the tests and dumps for language_negotitation variable migration.

Blocked by #2594263: Add translation data to Migrate Drupal's test fixtures.

Unblocked

WIP.

Started from scratch, as #4 is 2 years old now...
d6_language_negotiation_settings contains the required migration. I'm not contemplating language_types migration, which is needed for language negotiation to work, but #2130307: Variable to config: language.negotiation [d7] didn't neither and we will need an issue for doing that both for d6 and d7.

Tests fails because of schema issues, I've forgot about migrate for one year and need to catch up how sources schemas work when defining constants. Will find someone on IRC later.

Added tags.

@benjy figured out that I was defining my variable in schema in the wrong place.
I'm wondering how we could add more coverage now that we have concrete dumps for each version, so how can we check behavior having different source values?

Looks like \Drupal\Tests\comment\Unit\Migrate\d6\CommentVariablePerCommentTypeTest is a good example to follow for adding coverage for the different options of the mapping.

Increased testing coverage. Now I'm happy with this, please review.

+++ b/core/modules/language/migration_templates/d6_language_negotiation_settings.yml
@@ -0,0 +1,26 @@
+      # LANGUAGE_NEGOTIATION_NONE = 0

0 isn't actually mapped?

None its equivalent as no having value at all, so we just ignore it. Added tests assert on that.

Looks good.

Oh, I forgot about language prefixes.

@penyaskito asked for signoff on this. I think the patch is a good start but it does not migrate path prefixes and/or domains and neither it migrates which methods it would enable for negotiation AFAIS (it does not touch/set up language.types). So definitely needs work for those.

@Gábor, yes, I pinged you before noticing it. We have the same problem in D7 so I created #2625148: [PP-1] Migrate language prefixes and domains to language.negotiation [d7]

@penyaskito: still planning to work on this one?

Yes, we've been discussing last week and looks like the easiest solution is a custom process plugin.
I will get to this ASAP.

it does not migrate path prefixes and/or domains

The prefixes and domains appear to be independent of the language negotiation variable migration, which this patch does. If that is correct, the prefixes and domains are easier done in their own migration. They exist in a separate table and will, at least, need a custom source plugin. This is different that penyaskito suggestion in #26.

neither it migrates which methods it would enable for negotiation.

Please explain. What are the methods and where are they setup?

AFAIS (it does not touch/set up language.types).

Language types appear to be handled in #2225271: Migrate content type language settings from Drupal 6 & 7. Is that sufficient or are there more language.type settings that need to be migrated?

Needs reroll - minimally, the schema stuff needs to be removed from this patch.

Should we, as quietone suggests in #29, go ahead with this patch and handle prefixes and domains in a follow-up?

Rerolled the patch.

I'm pretty sure this should now be against 8.2.x-dev?

Should we try to reroll against 8.2.x-dev?

With no passing patch, this is Needs Work.

+++ b/core/modules/language/src/Tests/Migrate/d6/MigrateLanguageNegotiationSettingsTest.php
@@ -0,0 +1,89 @@
+  /**
+   * Tests migration of language negotiation variables to language.negotiation.yml.
+   */

Over 80 chars per line. Also, you have the same description for every function.

I will work on rerolling patch with 8.2.x version.

@Ashish.Dalvi: still working on it?

Patch still applied to 8.x-2.x, so not sure why the concern that it does not. Anyway, rolled a fresh copy. Let's see what does testbot have to say. Also unassigning.

Moved the MigrateLanguageNegotiationSettingsTest to Drupal\Tests\language\Kernel\Migrate\d6. Moved the test with language negotiation value of '1' to be the first test because that is the value of the language negotiation variable in the test fixture. This passed locally for 8.1.x and 8.2.x.

TODO: the issues raised in #38. Sorry, it is too late to do even that little bit more right now.

I was really tired last night, the patch in #44 is numbered #45.

Adjusted the comments to be within the 80 char limit.

Thanks! The issue summary mentions migrating the domains and paths out of the language objects, which would end up in the same language.negotiation config file. A sample file with 4 languages:

session:
  parameter: language
url:
  source: path_prefix
  prefixes:
    en: ''
    hu: hu
    es: es
    fr: fr
  domains:
    en: ''
    hu: ''
    es: ''
    fr: ''
selected_langcode: site_default

This patch does not migrate the paths and domains yet, it just migrates the setting itself as to whether to look at the path or domain.

The other thing misisng here is the creation of the language.types.yml file, which actually lists each negotiation method enabled and their priorities. That would be different for the different D6 language options as well and complete this migration.

BTW quietone said above that language types were migrated in #2225271: Migrate content type language settings from Drupal 6 & 7, but that deals with language setting for content types, not negotiation settings for language types. language.types.yml looks like this by default in core:

all:
  - language_interface
  - language_content
  - language_url
configurable:
  - language_interface
negotiation:
  language_content:
    enabled:
      language-interface: 0
  language_url:
    enabled:
      language-url: 0
      language-url-fallback: 1
  language_interface:
    enabled:
      language-url: 0

This lists which language types are available, which ones are configurable and then negotiation configuration for each with their priorities. That would need to be migrated here, given that differences between the D6 LANGUAGE_NEGOTIATION_PATH_DEFAULT and LANGUAGE_NEGOTIATION_PATH would make the language-url-fallback item either appear or not appear in the negotiation settings.

+++ b/core/modules/language/migration_templates/d6_language_negotiation_settings.yml
@@ -0,0 +1,26 @@
+  'session/parameter': 'constants/session_parameter'

So selected fallback langcode is missing, but that seems to be as simple as adding these:

source:
  constants:
    selected_langcode: 'site_default'
process:
  'selected_langcode' : 'constants/selected_langcode'

Because D6 does not make fallback configurable.

I wanted to add that new key manually and then figured out that the case where the setting was 0 also known as LANGUAGE_NEGOTIATION_NONE in Drupal 6, we should still migrate some default settings and not null them out like the setting expects/verifies. We should still migrate the default settings, because this config file does not have any effect on how the actual migration works if the migration options THERE are disabled. In fact LANGUAGE_NEGOTIATION_NONE should result in the site default fallback setting and only that one enabled.

Sorry did not want to step on your toes @Sonal.Sangale, if you are at DrupalCon now, you can come over to the general sprint room and discuss? :)

Hi Gábor Hojtsy,

I am glad that you asked to meet and discuss but I am not in DrupalCon now. :)

Hi @Sonal.Sangale, are you still working on this?
I was working on this at DrupalCon Dublin, but I forgot to assign it to myself.
After discussing this problem with @Gábor Hojtsy and @phenaproxima in Dublin, we found that migrating the prefixes and domains is a bit tricky, and we may have come up with a solution. I have a WIP patch that I may be uploading soon, but I don't want to step on your work.

Here's a WIP for migrating language prefixes and domains, based on a discussion with @Gábor Hojtsy and @phenaproxima in Dublin.

@phenaproxima: I'd love to have your input on the Array_map process plugin. Is that what you had in mind?

Looks fine for now to me (with my limited know-how of migrate). Also as per our discussion, it does need default handling for an empty domain too.

Unassign from @Sonal.Sangale to reflect reality. Note that your contributions would still be valuable. Sorry if we stepped on your toes at DrupalCon :/

Here's a new patch with the handling of an empty domain.
Also, I added a way to remove the protocol from the domain, since D6 expected it and D8 don't.
I also added tests for prefixes and domains.

About the possible empty domain, I had to assume that it was only the default language that was left empty, otherwise the migration will end up with duplicate domains. Should I check if the empty domain is for the default language? And leave empty non default empty domains?

I think multiple same domains are better than even a single empty domain, given that we are generating links and redirects based on these settings. At least the computed domain points to somewhere as opposed to an empty domain.

Here's the migration for the language.types configuration, and some tests.

What's missing now is the review for the new Array_map process plugin (@phenaproxima ;)). When the review is done we'll need to write some tests and documentation for it.

I'm not sure about the mapping I've made, but I think it's OK based on the D6 language negotiation help text. Here it is to help reviewers:

• None. The default language is used for site presentation, though users may (optionally) select a preferred language on the My Account page. (User language preferences will be used for site e-mails, if available.)
• Path prefix only. The presentation language is determined by examining the path for a language code or other custom string that matches the path prefix (if any) specified for each language. If a suitable prefix is not identified, the default language is used. Example: "example.com/de/contact" sets presentation language to German based on the use of "de" within the path.
• Path prefix with language fallback. The presentation language is determined by examining the path for a language code or other custom string that matches the path prefix (if any) specified for each language. If a suitable prefix is not identified, the display language is determined by the user's language preferences from the My Account page, or by the browser's language settings. If a presentation language cannot be determined, the default language is used.
• Domain name only. The presentation language is determined by examining the domain used to access the site, and comparing it to the language domain (if any) specified for each language. If a match is not identified, the default language is used. Example: "http://de.example.com/contact" sets presentation language to German based on the use of "http://de.example.com" in the domain.

Hmm, I think a better way to write the tests could be to combine them in four cases, the four possible cases of D6 language negotiation. What do you think?

Took a very brief look at this stood out.

+++ b/core/modules/language/src/Plugin/migrate/source/Language.php
@@ -49,4 +50,29 @@ public function query() {
+      $negotiation = $this->select('variable', 'v')
+        ->fields('v', array('value'))
+        ->condition('name', 'language_negotiation')
+        ->execute()
+        ->fetchField();

Why not use the variableGet method in DrupalSqlBase?

The name array_map for a process plugin is confusing. The corresponding Php function does a very different thing. What other use case are there for this function? Just wondering. Anyway, array_map can wait until a maintainer reviews.

@quietone: I wasn't aware of variableGet() in DrupalSqlBase, thanks!

About Array_map, the idea and the name came from a brief discussion with @phenaproxima at DrupalCon Dublin. I was going to make a specific plugin for prefixes & domains, but he suggested to make a generic one so it could be used elsewhere. I'm waiting for his review, it's only a minimal prototype to show him what is needed here.

Then, if we go this way, we'll need tests and documentation for this new plugin.

1. +++ b/core/modules/language/src/Plugin/migrate/source/Language.php
   @@ -49,4 +50,25 @@ public function query() {
   +    if (isset($this->configuration['negotiation']) && $this->configuration['negotiation']) {
   

   For brevity, let's make this if (!empty($this->configuration['negotiation'])).

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/Array_map.php
   @@ -0,0 +1,39 @@
   +class Array_map extends ProcessPluginBase {
   

   Should be named ArrayMap.

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/Array_map.php
   @@ -0,0 +1,39 @@
   +    foreach ($value as $old_key => $old_value) {
   +      $new_value[$old_value[$this->configuration['key']]] = $old_value[$this->configuration['value']];
   +    }
   

   It is being assumed here that $old_value is an array, or can be used as an array. Can we cast $value to array so that it will always be traversable?

4. +++ b/core/modules/migrate/src/Plugin/migrate/process/Array_map.php
   @@ -0,0 +1,39 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function multiple() {
   +    return TRUE;
   +  }
   

   I've never seen this before, why is it needed?

Patch updated.

This is really close to ready, but I'd like to see two things:

1) A unit test of ArrayMap; and
2) Extensive documentation in ArrayMap's doc comment about the purpose of the plugin, plus usage examples.

It might need a change record as well. @alexpott, what do you think?

+++ b/core/modules/migrate/src/Plugin/migrate/process/ArrayMap.php
@@ -0,0 +1,32 @@
+    foreach ((array) $value as $old_key => $old_value) {
+      $new_value[$old_value[$this->configuration['key']]] = $old_value[$this->configuration['value']];
+    }

One more thing I'd like to see -- can we add an array_key_exists() sanity check to make certain that both the key and value in $old_value actually exist, and throw an exception if not? I ask because if the key doesn't exist, the output array could potentially cause weird, hard-to-debug behavior further down the chain.

Thanks @phenaproxima for the review and @mitrpaka for the corrections.

@phenaproxima, about #69/4, I based this plugin on the Iterator plugin which contains this multiple() method, if it's not needed here I guess it's not needed there either.

I'm working on a unit test and documentation for the ArrayMap plugin.

Here's the unit test.
Documentation's next.

Of the code, I have only minor nitpicks at this point :) I'd still like to see documentation and usage examples though, per #73.2. Excellent work, @maxocub et. al.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/ArrayMap.php
   @@ -23,6 +24,21 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +        throw new MigrateException("The input should be an array of arrays.");
   

   I might be wrong about this, but I believe that we are not supposed to add periods at the end of exception messages in core...

2. +++ b/core/modules/migrate/tests/src/Unit/process/ArrayMapTest.php
   @@ -0,0 +1,89 @@
   +    $this->plugin = new ArrayMap($configuration, 'map', array());
   

   Let's switch to []. The core standard is to prefer [] over array(), and not to mix both in the same file.

3. +++ b/core/modules/migrate/tests/src/Unit/process/ArrayMapTest.php
   @@ -0,0 +1,89 @@
   +  public function testExtract() {
   

   Let's rename this testTransform().

@phenaproxima: Sure, I'm now working on the documentation and usage examples, but it's the first time I'll be doing that so I'm doing some reading on that subject first.

One thing I'm wondering though is why the other migrate process plugins have a link to the online handbook instead of documentation in their docblocks?

One thing I'm wondering though is why the other migrate process plugins have a link to the online handbook instead of documentation in their docblocks?

Because that is how it used to be done :) There is an issue underway to get all the plugin documentation moved into the docblocks instead: #2776179: [meta] Add process plugin documentation to the codebase. Rather than reroll that issue when ArrayMap is committed, I figure it's better to just document ArrayMap inline right now.

Here's a first version of the documentation and example usage. English is not my native language, so there may be better ways to formulate this. Also, there's a lot of usage of the words 'key' and 'value', so I hope it's at least a bit clear.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/ArrayMap.php
   @@ -8,7 +8,67 @@
   + * The array_map process plugin has two configuration keys, key and value.
   + * It will iterate over the source, which should be a list of associative
   + * arrays, and create a new associative array containing one element per
   + * array in the source. To create that array, it will use the key and value
   + * configuration to lookup the associated values in the current array from
   + * the iteration.
   

   I think I would phrase it this way:

   The array_map plugin builds a single associative array by extracting keys and values from each array in the input value, which is expected to be an array of arrays. The keys of the returned array will are determined by the 'key' configuration option, and the values are determined by the 'value' option.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/ArrayMap.php
   @@ -18,7 +78,22 @@
   +   * Performs the associated process.
   +   *
   +   * @param array $value
   +   *   The input array.
   +   * @param \Drupal\migrate\MigrateExecutableInterface $migrate_executable
   +   *   The migration in which this process is being executed.
   +   * @param \Drupal\migrate\Row $row
   +   *   The row from the source to process.
   +   * @param string $destination_property
   +   *   The destination property currently worked on. This is only used together
   +   *   with the $row above.
   +   *
   +   * @return array
   +   *   The created array.
   +   *
   +   * @throws \Drupal\migrate\MigrateException
   

   This should just be {@inheritdoc}, since transform() comes from the process plugin interface.

Also, I hate to do this...but I have realized that calling this plugin ArrayMap (and array_map as the plugin ID) is kind of a misnomer. Can we rename it to something else? I'm thinking ArrayBuild/array_build, but I'm open to other ideas if they will be clearer.

That's a much more clear way to phrase it, thanks @phenaproxima!
I don't have any other idea for the name of this plugin, so let's use ArrayBuild/array_build, which is better that ArrayMap/array_map.

Glorious.

@maxocub, thanks! Pleased to see the name change to ArrayBuild. And love the docs too.

Hmm, I just realized that I didn't add a unit test for the LanguageDomains process plugin, I guess I should, right?

Also, since I started to think about migrating prefixes and domains from D7 (#2625148: [PP-1] Migrate language prefixes and domains to language.negotiation [d7]), I realized that this plugin only works for D6, and should be able to do both, I think. Should I try to correct that here or do we wait till this gets in and I adapt it latter?

Better to get it done now. If we missed a unit test for LanguageDomains, let's add one now as well.

Agreed with @phenaproxima :)

Here's a new patch where I make the language prefix and domains migration work for D6 & D7 and add a unit test for the plugin, as requested in #86.

I also corrected the d7_language_negotiation_settings which didn't worked for sites using domain negotiation.

I forgot to update the D7 language negotiation settings test.

Looks fantastic, resolves the concerns raised since the last RTBC and nicely integrates Drupal 7 migration with corresponding tests :)

1. +++ b/core/modules/language/migration_templates/d6_language_types.yml
   @@ -0,0 +1,52 @@
   +  configurable:
   +    plugin: default_value
   +    default_value:
   +      - 'language_interface'
   

   This is the extent core supports, so I think this looks good. We can update when/if an entity_translation module migration is built.

2. +++ b/core/modules/language/migration_templates/d6_language_types.yml
   @@ -0,0 +1,52 @@
   +    map:
   +      # LANGUAGE_NEGOTIATION_NONE = 0
   +      # LANGUAGE_NEGOTIATION_PATH_DEFAULT = 1
   +      # LANGUAGE_NEGOTIATION_PATH = 2
   +      # LANGUAGE_NEGOTIATION_DOMAIN = 3
   +      0:
   +        'language-selected': 0
   +      1:
   +        'language-url': 0
   +        'language-selected': 1
   +      2:
   +        'language-url': 0
   +        'language-user': 1
   +        'language-browser': 2
   +        'language-selected': 3
   +      3:
   +        'language-url': 0
   +        'language-selected': 1
   

   This is so beautiful :)

3. +++ b/core/modules/language/tests/src/Kernel/Migrate/d7/MigrateLanguageNegotiationSettingsTest.php
   @@ -2,37 +2,92 @@
   +  /**
   +   * Tests the migration with non-existent variables.
   +   */
   

   Woot, nice to see this tested :)

Some self nitpicking :)

Random test fail, re-tested and back to RTBC.

Now that the other patch has landed this needs a reroll.

The only conflicts to resolve were in core/modules/language/tests/src/Kernel/Migrate/d7/MigrateLanguageNegotiationSettingsTest.php due to the new test method added in #2353709: Variables to config: language.types [d7].

1. Sorry I've just spotted a few nits I missed when looking at this before

2. +++ b/core/modules/language/tests/src/Kernel/Migrate/d7/MigrateLanguageNegotiationSettingsTest.php
   @@ -64,4 +49,77 @@ public function testLanguageTypes() {
   +  /**
   +   * Tests the migration with prefix negotiation.
   +   */
   +   public function testLanguageNegotiationWithPrefix() {
   +     $this->executeMigrations([
   +       'language',
   +       'd7_language_negotiation_settings',
   +       'language_prefixes_and_domains',
   +     ]);
   +
   +     $config = $this->config('language.negotiation');
   +     $this->assertSame($config->get('session.parameter'), 'language');
   +     $this->assertSame($config->get('url.source'), LanguageNegotiationUrl::CONFIG_PATH_PREFIX);
   +     $this->assertSame($config->get('selected_langcode'), 'site_default');
   +     $expected_prefixes = [
   +       'en' => '',
   +       'is' => 'is',
   +     ];
   +     $this->assertSame($config->get('url.prefixes'), $expected_prefixes);
   +  }
   +
   +  /**
   +   * Tests the migration with domain negotiation.
   +   */
   +   public function testLanguageNegotiationWithDomain() {
   +     $this->sourceDatabase->update('variable')
   +       ->fields(array('value' => serialize(1)))
   +       ->condition('name', 'locale_language_negotiation_url_part')
   +       ->execute();
   +
   +     $this->executeMigrations([
   +       'language',
   +       'd7_language_negotiation_settings',
   +       'language_prefixes_and_domains',
   +     ]);
   +
   +     global $base_url;
   +     $config = $this->config('language.negotiation');
   +     $this->assertSame($config->get('session.parameter'), 'language');
   +     $this->assertSame($config->get('url.source'), LanguageNegotiationUrl::CONFIG_DOMAIN);
   +     $this->assertSame($config->get('selected_langcode'), 'site_default');
   +     $expected_domains = [
   +       'en' => parse_url($base_url, PHP_URL_HOST),
   +       'is' => 'is.drupal.org',
   +     ];
   +     $this->assertSame($config->get('url.domains'), $expected_domains);
   +   }
   +
   +   /**
   +    * Tests the migration with non-existent variables.
   +    */
   +   public function testLanguageNegotiationWithNonExistentVariables() {
   +     $this->sourceDatabase->delete('variable')
   +       ->condition('name', ['local_language_negotiation_url_part', 'local_language_negotiation_session_param'], 'IN')
   +       ->execute();
   +
   +     $this->executeMigrations([
   +       'language',
   +       'd6_language_negotiation_settings',
   +       'language_prefixes_and_domains',
   +     ]);
   +
   +     $config = $this->config('language.negotiation');
   +     $this->assertSame($config->get('session.parameter'), 'language');
   +     $this->assertSame($config->get('url.source'), LanguageNegotiationUrl::CONFIG_PATH_PREFIX);
   +     $this->assertSame($config->get('selected_langcode'), 'site_default');
   +     $expected_prefixes = [
   +       'en' => '',
   +       'is' => 'is',
   +     ];
   +     $this->assertSame($config->get('url.prefixes'), $expected_prefixes);
   +  }
   

   All this code is not indented correctly.

3. +++ b/core/modules/language/tests/src/Unit/process/LanguageDomainsTest.php
   @@ -0,0 +1,59 @@
   + * @coversDefaultClass \Drupal\language\Plugin\migrate\process\LanguageDomains
   ...
   +
   +  public function testTransform() {
   

   Method is missing a comment with an @covers

4. +++ b/core/modules/migrate/src/Plugin/migrate/process/ArrayBuild.php
   @@ -0,0 +1,107 @@
   +      // Checks that the value exists
   

   Needs a full stop.

5. +++ b/core/modules/migrate/tests/src/Unit/process/ArrayBuildTest.php
   @@ -0,0 +1,89 @@
   +   * @expectedException \Drupal\migrate\MigrateException
   +   * @expectedExceptionMessage The key 'foo' does not exist
   ...
   +   * @expectedException \Drupal\migrate\MigrateException
   +   * @expectedExceptionMessage The key 'bar' does not exist
   

   See #2822837: Replace @expectedException @expectedExceptionMessage with $this->setExpectedException - we do this differently now.

Here you go!

Looks good to me, pending passing tests :)

Committed and pushed 1380ab9 to 8.3.x and aa8ed51 to 8.2.x. Thanks!

Amazing, thanks all especially @maxocub :)