D6/7->D8 migration: Menu_links migration is not working because option parameters were not found

Let's make sure the 8.2.x patch is last in the list here so the testbot doesn't fall over on us.

Thx Adita. I reckon this needs tests.

Seems to me this is about passing invalid data to Route. Therefor checks have been added to do basic validation on the input and throw MigrateExceptions if those checks fail. With that being done, I wasn't able to reproduce the original problem. However, the code from the patch #12 is still included in case those checks prove insufficient.

And tests have been added.

Could we please not die? If it's a string it's the link path and options is empty. Done?

Ps. thanks for the work, hope the reviewing is speedy enough to not block you :)

chx, thanks. Of course, just like me not to spot the obvious. I'll fix this tomorrow.

Well, it isn't tomorrow but I'm home earlier than expected from Drupal Meetup.

Forgot to remove the needs test tag.

I have nitpicks and one question, but apart from that I consider this RTBC.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Route.php
   @@ -77,10 +82,12 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +          $route['options'] = $route['options'] + $options;
   

   Someone please correct me if I'm wrong, but won't this preserve the keys in $route['options'] and throw out keys in $options? Don't we want $options to override $route['options'], which would mean array_merge()?

2. +++ b/core/modules/migrate/tests/src/Unit/process/RouteTest.php
   @@ -0,0 +1,147 @@
   +    $this->pathValidator = $this->getMockBuilder('Drupal\Core\Path\PathValidatorInterface')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   +    $this->row = $this->getMockBuilder('Drupal\migrate\Row')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   +    $this->migrateExecutable = $this->getMockBuilder('Drupal\migrate\MigrateExecutable')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   

   I'd rather we use Prophecy ($this->prophesize()) for this stuff.

3. +++ b/core/modules/migrate/tests/src/Unit/process/RouteTest.php
   @@ -0,0 +1,147 @@
   +  public function testRoute($value = NULL, $expected = NULL) {
   

   Why are both of these defaulted to NULL? I'd expect them to both be required.

4. +++ b/core/modules/migrate/tests/src/Unit/process/RouteTest.php
   @@ -0,0 +1,147 @@
   +
   +    $link_path = is_string($value) ? $value : $value[0];
   

   Please remove the empty line.

5. +++ b/core/modules/migrate/tests/src/Unit/process/RouteTest.php
   @@ -0,0 +1,147 @@
   +    $url = url::fromRoute($link_path);
   

   If $link_path is an internal path, this should be Url::fromInternalUri(), and $link_path should be prefixed with internal:/. For example, internal:/node/5. fromRoute() is only used when you know the actual route name, which I believe is the point of the Route process plugin to begin with :)

6. +++ b/core/modules/migrate/tests/src/Unit/process/RouteTest.php
   @@ -0,0 +1,147 @@
   +   *  An array of arrays, where the first element is the input to Route(), and
   

   Route() isn't a thing :) Let's change that to "input to the Route plugin".

7. +++ b/core/modules/migrate/tests/src/Unit/process/RouteTest.php
   @@ -0,0 +1,147 @@
   +      0 => "user/login",
   +      1 => [
   +        'attributes' => [
   +          'title' => 'Test menu link 1',
   +        ],
   +      ],
   

   It looks jarring to set numeric indexes on indexed arrays. Can those keys be removed?

8. +++ b/core/modules/migrate/tests/src/Unit/process/RouteTest.php
   @@ -0,0 +1,147 @@
   +    $values[1] = [
   +      0 => "user/login",
   +      1 => [],
   +    ];
   

   Same here.

9. +++ b/core/modules/migrate/tests/src/Unit/process/RouteTest.php
   @@ -0,0 +1,147 @@
   +    $values[3] = [
   +      0 => 'https://www.drupal.org',
   +      1 => [
   +        'attributes' => [
   +          'title' => '',
   +        ],
   +      ],
   +    ];
   

   And here.

Some progress on the review.

1. todo. I had a reason for leaving it but unfortunately didn't make a note of it. Maybe it will come back to me while the other unfinished items are worked on.
2. Why use Prophecy in this case?
3. Fixed
4. Fixed
5. todo
6. Fixed
7-9. Fixed.

Why use Prophecy in this case?

For no better reason than that it's a lot easier to read. I'm not insisting on the use of Prophecy, it's just my preference. Consider it a nitpick :)

1. Changed to array_merged
2. Fixed.

That addresses all the feedback in #21.

1. +++ b/core/modules/migrate/tests/src/Unit/process/RouteTest.php
   @@ -0,0 +1,134 @@
   +    $this->pathValidator = $this->prophesize(PathValidatorInterface::CLASS);
   

   s/CLASS/class

2. +++ b/core/modules/migrate/tests/src/Unit/process/RouteTest.php
   @@ -0,0 +1,134 @@
   +    $url = parse_url($link_path, PHP_URL_SCHEME) ? Url::fromUri($link_path) : Url::fromRoute($link_path);
   

   Url::fromRoute() is only used if you know the route name. I think this should be Url::fromInternalUri().

3. +++ b/core/modules/migrate/tests/src/Unit/process/RouteTest.php
   @@ -0,0 +1,134 @@
   +      'route_name' => "user/login",
   

   Valid route names are never paths, so this is asserting an invalid value.

This should fix up #1/#2. #3 is going to need some more work. Do we really need to convert to a kernel test to call Url::fromUri?

1) Drupal\Tests\migrate\Unit\process\RouteTest::testRoute with data set #0 (array('internal:/user/login', array(array('Test menu link 1'))), array('user/login', array(), array(array('Test menu link 1')), null))
Drupal\Core\DependencyInjection\ContainerNotInitializedException: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.

drupal/core/lib/Drupal.php:129
drupal/core/lib/Drupal.php:691
drupal/core/lib/Drupal/Core/Url.php:413
drupal/core/lib/Drupal/Core/Url.php:306
drupal/core/modules/migrate/tests/src/Unit/process/RouteTest.php:45

+++ b/core/modules/migrate/tests/src/Unit/process/RouteTest.php
@@ -0,0 +1,134 @@
+    $values[0] = [
+      "internal:/user/login",
+      [

Don't understand why this is here. This is the source link_path from D6 or D7 and they don't have the internal scheme.

Url::fromInternalUri() isn't available here, it is protected. So I took a different approach, if link_path is external then use Url::from Uri otherwise just make a new url with the expected route name.

The patch seems to solve the issue in #2813989.

Looks like the tests passed so setting to NR.

- $route['options'] = $route['options'] + $options;
+ $route['options'] = array_merge($route['options'], $options);

that swaps the order of overwrite, are you sure...?

I'm seeing weird behaviour with this. I have a migration script that runs after installation which adds a link to the contact form (/contact path).
In the database this appears as menu_tree.url = 'base:contact' instead of menu_tree.route_name = 'contact.site_page'.

After a manual cache clear (in the UI, note: doesn't work with drush cr..), the route_name is magically filled in to 'contact.site_page'.

How is this possible?

I'm seeing the same behavior as comment #34 without applying any patches. Might be a different issue? I'm on Drush 8.1.5 or with Drupal 8.2.

Please open a different issue for 34/35. I don't think it is related.

Reopened #2813989, sorry for the noise.

Good point in #33. Because we are mocking out pathValidator, we aren't actually extracting options. So the tests aren't doing a very good job of testing anything. This needs to convert to a kernel test and we need to parse those paths correctly. This should so the problem with the tests.

How about adding the options to the Url method? In regards to #33, it is reverted to the original.

The original patch attempted to grab URL parameters. So, we need to handle those as well as a specific test case. I think the solution is valid, but the test doesn't test it. It only grabs non-url stuffs to test. We need to parse the URL and see how it gets stored too.

@heddn, which patch attempted to grab URL parameters?

Added another test. Although I still feel like I'm missing something about this.

Convert the tests from unit to a kernel test so you can call a real PathValidator service, instead of the mock. That way we get some valid errors and exceptions to trap and handle.

Thanks to dman and larowlan at DrupalSouth the unit test has been converted to a kernel test.

Oops, wrong patch. This should be better.

Pretty thorough test coverage. I like it!

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Route.php
   @@ -80,7 +84,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +          $options['query'] = isset($route['options']['query']) ? $route['options']['query'] + $old_query : $old_query;
   

   It's not entirely clear why this is being done. Can there be a comment explaining this line?

2. +++ b/core/modules/migrate/tests/src/Kernel/process/RouteTest.php
   @@ -0,0 +1,284 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function setUp() {
   +    parent::setUp();
   +  }
   

   This is not necessary and should be removed.

Thx, I wondered if I was going overboard with the testing.

1) After all this, it isn't clear to me either. AFAIKT $route['options']['query'] will always be set. getUgetUrlIfValidWithoutAccessCheck calls getUrl which returns a new Url with this line,
return new Url($route_name, $route_parameters, $options + ['query' => $request->query->all()]);
Because of that I have restored the original code.

2) Done.

And, there was a mixture of double and single qutoes in the RouteTest. There are all single quotes now.

+++ b/core/modules/migrate/src/Plugin/migrate/process/Route.php
@@ -30,21 +29,19 @@ class Route extends ProcessPluginBase implements ContainerFactoryPluginInterface
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, PathValidatorInterface $path_validator) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, PathValidatorInterface $path_validator) {
...
-  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration = NULL) {
+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {

Do we need to add a BC layer here? If we do need a BC layer, please tag as novice and provide some suggestions on naming of classes for the BC layer.

I much appreciate the added tests. Thanks for the extra effort there.

Discussed at the migrate meeting and agreed to leave it alone, that is restore MigrationInterface $migration in the constructor.

Restored the MigrationInterface $migration and now BC is not an issue.

Looks good to me now. All my concerns are met.

I'm a bit confused why we have the route process plugin at all, when menu links are generally stored with entity:// or internal:// in 8.x - what am I missing?

@catch it used to process the link information as part of something. For example, when migrating a menu_link_content entity. See https://api.drupal.org/api/drupal/core%21modules%21menu_link_content%21m...

FILE: ...upal/core/modules/migrate/tests/src/Kernel/process/RouteTest.php
----------------------------------------------------------------------
FOUND 8 ERRORS AND 2 WARNINGS AFFECTING 10 LINES
----------------------------------------------------------------------
  45 | ERROR   | [x] Return comment indentation must be 3 spaces,
     |         |     found 2 spaces
  46 | ERROR   | [x] Return comment indentation must be 3 spaces,
     |         |     found 2 spaces
  52 | ERROR   | [x] Array indentation error, expected 6 spaces but
     |         |     found 5
  96 | ERROR   | [x] Inline comments must end in full-stops,
     |         |     exclamation marks, colons, question marks, or
     |         |     closing parentheses
 100 | ERROR   | [x] Inline comments must end in full-stops,
     |         |     exclamation marks, colons, question marks, or
     |         |     closing parentheses
 122 | WARNING | [x] A comma should follow the last multiline array
     |         |     item. Found: NULL
 216 | ERROR   | [x] Return comment indentation must be 3 spaces,
     |         |     found 2 spaces
 217 | ERROR   | [x] Return comment indentation must be 3 spaces,
     |         |     found 2 spaces
 237 | WARNING | [x] A comma should follow the last multiline array
     |         |     item. Found: '1'
 278 | ERROR   | [x] Expected 1 newline at end of file; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 10 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Some coding standards stuff to fix in the new test.

And fix the IDE settings as well...
Fixed errors found in #57.

And this is ready again.

Failures are in outside_in, which is not related, so retesting.

Tests are passing. Back to RTBC.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!