Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Menu_links migration is not working because option parameters were not found.
Proposed resolution
Verify if menu_links migration have option parameters.
Comment | File | Size | Author |
---|---|---|---|
#57 | interdiff.txt | 2 KB | quietone |
#57 | d6_7_d8_migration-2707387-57.patch | 8.9 KB | quietone |
#52 | interdiff.txt | 2.54 KB | quietone |
#52 | d6_7_d8_migration-2707387-52.patch | 8.9 KB | quietone |
#49 | interdiff.txt | 2.76 KB | quietone |
Comments
Comment #2
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedComment #3
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedThis is a patch for the 8.0.6 core because the code is different in 8.2.x, are different lines.
Comment #4
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedComment #6
heddnLet's make sure the 8.2.x patch is last in the list here so the testbot doesn't fall over on us.
Comment #7
quietone CreditAttribution: quietone as a volunteer commentedComment #8
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedThis a new error for menu links migration, with this patch is fixed.
Error: Unsupported operand types in Drupal\migrate\Plugin\migrate\process\Route->transform() (line 83 of core/modules/migrate/src/Plugin/migrate/process/Route.php). [error]
Comment #9
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedAn interdiff for here...
Comment #10
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedAn interdiff for here...
Comment #11
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedAn interdiff for here...
Comment #12
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedThis is a better patch for menu links migrations.
Comment #13
quietone CreditAttribution: quietone as a volunteer commentedThx Adita. I reckon this needs tests.
Comment #14
quietone CreditAttribution: quietone as a volunteer commentedComment #15
quietone CreditAttribution: quietone as a volunteer commentedSeems 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.
Comment #16
chx CreditAttribution: chx at Smartsheet commentedCould 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 :)
Comment #17
quietone CreditAttribution: quietone as a volunteer commentedchx, thanks. Of course, just like me not to spot the obvious. I'll fix this tomorrow.
Comment #18
quietone CreditAttribution: quietone as a volunteer commentedWell, it isn't tomorrow but I'm home earlier than expected from Drupal Meetup.
Comment #19
quietone CreditAttribution: quietone as a volunteer commentedForgot to remove the needs test tag.
Comment #21
phenaproximaI have nitpicks and one question, but apart from that I consider this RTBC.
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()?
I'd rather we use Prophecy (
$this->prophesize()
) for this stuff.Why are both of these defaulted to NULL? I'd expect them to both be required.
Please remove the empty line.
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 :)Route() isn't a thing :) Let's change that to "input to the Route plugin".
It looks jarring to set numeric indexes on indexed arrays. Can those keys be removed?
Same here.
And here.
Comment #22
quietone CreditAttribution: quietone as a volunteer commentedSome 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.
Comment #23
phenaproximaFor 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 :)
Comment #24
quietone CreditAttribution: quietone as a volunteer commented1. Changed to array_merged
2. Fixed.
That addresses all the feedback in #21.
Comment #25
phenaproximas/CLASS/class
Url::fromRoute() is only used if you know the route name. I think this should be Url::fromInternalUri().
Valid route names are never paths, so this is asserting an invalid value.
Comment #26
heddnThis 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
Comment #27
quietone CreditAttribution: quietone as a volunteer commentedDon'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.
Comment #29
mpp CreditAttribution: mpp as a volunteer and at AmeXio commentedThe patch seems to solve the issue in #2813989.
Comment #30
quietone CreditAttribution: quietone as a volunteer commentedLooks like the tests passed so setting to NR.
Comment #32
mpp CreditAttribution: mpp as a volunteer and at AmeXio commentedComment #33
chx CreditAttribution: chx at Smartsheet commented- $route['options'] = $route['options'] + $options;
+ $route['options'] = array_merge($route['options'], $options);
that swaps the order of overwrite, are you sure...?
Comment #34
mpp CreditAttribution: mpp as a volunteer and at AmeXio commentedI'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?
Comment #35
mariacha1 CreditAttribution: mariacha1 at ThinkShout commentedI'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.
Comment #36
heddnPlease open a different issue for 34/35. I don't think it is related.
Comment #37
mpp CreditAttribution: mpp as a volunteer and at AmeXio commentedReopened #2813989, sorry for the noise.
Comment #38
heddnGood 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.
Comment #40
quietone CreditAttribution: quietone as a volunteer commentedHow about adding the options to the Url method? In regards to #33, it is reverted to the original.
Comment #41
heddnThe 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.
Comment #42
quietone CreditAttribution: quietone as a volunteer commented@heddn, which patch attempted to grab URL parameters?
Comment #43
quietone CreditAttribution: quietone as a volunteer commentedAdded another test. Although I still feel like I'm missing something about this.
Comment #44
heddnConvert 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.
Comment #45
quietone CreditAttribution: quietone as a volunteer commentedThanks to dman and larowlan at DrupalSouth the unit test has been converted to a kernel test.
Comment #47
quietone CreditAttribution: quietone as a volunteer commentedOops, wrong patch. This should be better.
Comment #48
phenaproximaPretty thorough test coverage. I like it!
It's not entirely clear why this is being done. Can there be a comment explaining this line?
This is not necessary and should be removed.
Comment #49
quietone CreditAttribution: quietone as a volunteer commentedThx, 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.
Comment #50
heddnDo 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.
Comment #51
quietone CreditAttribution: quietone as a volunteer commentedDiscussed at the migrate meeting and agreed to leave it alone, that is restore MigrationInterface $migration in the constructor.
Comment #52
quietone CreditAttribution: quietone as a volunteer commentedRestored the MigrationInterface $migration and now BC is not an issue.
Comment #53
heddnLooks good to me now. All my concerns are met.
Comment #54
catchI'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?
Comment #55
alexpott@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...
Comment #56
alexpottSome coding standards stuff to fix in the new test.
Comment #57
quietone CreditAttribution: quietone as a volunteer commentedAnd fix the IDE settings as well...
Fixed errors found in #57.
Comment #58
heddnAnd this is ready again.
Comment #60
quietone CreditAttribution: quietone as a volunteer commentedFailures are in outside_in, which is not related, so retesting.
Comment #61
quietone CreditAttribution: quietone as a volunteer commentedTests are passing. Back to RTBC.
Comment #64
catchCommitted/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!