Problem/Motivation

It looks like menu links that are localized are not migrated properly. I discovered this while working on #2975461: Convert query string to array for d6 menu_link migration and I was only looking at the because it came up as the triage issue for the day in #bugsmash earlier this week. So, well done bugsmash!

Steps to reproduce

Run a menu link migration from d6 and localized ones, not in the default language, get migrated with the default language instead of the menu language.

Proposed resolution

Add a migration for localized menu links, d6_menu_links_localized.

Remaining tasks

Patch
Tests
Review
Commit

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#26 3245553-16.patch7.75 KBquietone
#16 3245553-16.patch7.75 KBquietone
#16 diff.txt1.38 KBquietone
#10 3245553-10-fail.patch933 bytesquietone
#8 3245553-8.patch7.75 KBquietone
#8 interdiff-2-8.txt2.88 KBquietone
#7 menu_after.png48.95 KBdanflanagan8
#7 menu_before.png57.39 KBdanflanagan8
#7 menu_d6.png63.04 KBdanflanagan8
#2 3245553-2.patch7.85 KBquietone
#2 3245553-2-fail.patch4.43 KBquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
3245553-2-fail.patch4.43 KB
3245553-2.patch7.85 KB

And a fail patch and a working patch.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Active » Needs review

The last submitted patch, 2: 3245553-2-fail.patch, failed testing. View results

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Category: Task » Bug report
Issue tags: +Bug Smash Initiative

It could be argued that this is a task, migrate localized menu link for D6. However, it is also a bug because the localized links are migrated incorrectly. Switching to a bug.

For those with configured legacy d6_menu_links migrations localized menu links will no longer be migrated, they were wrong anyway. Before this they were migrated with an incorrect language so not very useful. To fix that they need to update d6_menu_links and add the new d6_menu_links_localized migration.

The changes to the process plugin will not alter the results for legacy configured migrations.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
CreditAttribution: danflanagan8 at Horizontal commented
Status: Needs review » Needs work
FileSize
menu_d6.png63.04 KB
menu_before.png57.39 KB
menu_after.png48.95 KB

@quietone, I've spent some time reviewing this one, though I'm starting to feel a little overmatched. Here's where I am...

First, I confirmed the bug doing a migration from the d6 fixture through the UI. After applying the patch and doing it all over, the menu link item was migrated correctly.

Here's the menu item in D6

Migrated into D9.4 without the patch

Migrated into D9.4 with the patch

So manual testing was good. Now for the code.

1. I'm a little confused by the naming of the new source properties skip_localized and not_localized. From an English standpoint, they look equivalent to me. We would skip localized migration if it is not localized.

2. Is there a reason that we are using two properties for this? Since they are always opposite, would we be able to use a single property? Or could we even just rely on the langcode property?

3. I'm struggling to parse the tests, probably in part because I'm just not that strong in translation and localization. (I don't even know if those are different things!) But I think something that would help me would be if the test-only patch would simply demonstrate the existing bug. Right now it fails with this message:

No migrations created for id 'd6_menu_links_localized'.

Not a surprise that migration doesn't exist since it's being added in the next patch! Do you think you could come up with something that just shows d6_menu_links failing? It looks like the existing tests were trying to assert translated menu links. Why were those assertions not catching this problem?

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
FileSize
interdiff-2-8.txt2.88 KB
3245553-8.patch7.75 KB

#7.1 and 2. It looks like I just did something quick and dirty. This should be cleaner.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs work » Needs review
quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
FileSize
3245553-10-fail.patch933 bytes

I know what you mean about localization and translation. Despite working on all the translation migrations I still don't feel like I know the system well.

7.3. Here is a failing test. Hope it helps.

Status: Needs review » Needs work

The last submitted patch, 10: 3245553-10-fail.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs work » Needs review

Returning to Need Review because I made a sample failing test after posting the success patch.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
CreditAttribution: danflanagan8 at Horizontal commented
Status: Needs review » Needs work

@quietone, thank you for the updated fail test. That totally cleared this up for me. The existing assertions are testing the langcode attribute on the link, which is not the same as the langcode of the entity. That is the biggest thing I wasn't putting together.

And I think I get the localization/translation difference here. The menu link we are looking at only exists in French. It's not an English entity that also has a French translation. Is that right?

I like the `is_localized` flag a lot. But I wonder if rather than setting it to `FALSE` we could just keep it `NULL`. 

+++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
@@ -141,6 +141,20 @@ public function prepareRow(Row $row) {
+    $row->setSourceProperty('is_localized', FALSE);

And instead of `is_bool` we could just use `empty` as the callable, which is a more common function and reads more easily to me.

process:
  skip_localized:
    -
      plugin: callback
      callable: empty
      source: is_localized
    -
      plugin: skip_on_empty
      method: row

I think we could do that even if you prefer setting `is_localized` to `FALSE`.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs work » Needs review

@danflanagan8, your welcome. I'm glad it helped. Yes, the link is not a translation, it only exists in the French language.

Glad you like 'is_localized', it does read better. As for the suggested change it won't work because empty is not callable.

php > var_dump(is_callable("is_bool"));
bool(true)
php > var_dump(is_callable("is_string"));
bool(true)
php > var_dump(is_callable("empty"));
bool(false)
danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
CreditAttribution: danflanagan8 at Horizontal commented

@quietone, right you are! It looks like is_null is callable. What do you think about relying on NULL and is_null instead of FALSE and is_bool?

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
FileSize
diff.txt1.38 KB
3245553-16.patch7.75 KB

In core, both a boolean and NULL are used for setting source properties that appear to be used for 'skips'. So, there is no precedence or preferred way. Personally, I do not think the change is necessary, this works and is readable, and is for Drupal 6 source which ideally no one should be using anymore.

The attached patch changes is_localized to be NULL when the menu link is not localized in the source plugin and changes the callable used in d6_menu_links from is_bool to is_null.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
CreditAttribution: danflanagan8 at Horizontal commented
Status: Needs review » Reviewed & tested by the community

Thanks, @quietone! Sorry for putting you through the wringer. I know the tests are still running but there's no reason they should fail. I'm going to RTBC this, and hopefully I won't get embarrassed by a failed test.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

Yay, neither of use are embarrassed by a failing test!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 3245553-16.patch, failed testing. View results

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
CreditAttribution: danflanagan8 at Horizontal commented
Status: Needs work » Reviewed & tested by the community

Random fails in FunctionalJavascript tests. Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 3245553-16.patch, failed testing. View results

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
CreditAttribution: danflanagan8 at Horizontal commented
Status: Needs work » Reviewed & tested by the community

More random fails in FunctionalJavascript tests. Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 3245553-16.patch, failed testing. View results

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
CreditAttribution: danflanagan8 at Horizontal commented
Status: Needs work » Reviewed & tested by the community

29,412 pass, 64 fail

Holy moly! This can't have been related to this patch. Back to RTBC.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Version: 9.4.x-dev » 10.0.x-dev
quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
FileSize
3245553-16.patch7.75 KB

Re-uploading patch so it will test with 10

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 3245553-16.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs work » Reviewed & tested by the community

Unrelated fail
1) Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest::testLinkability

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 3245553-16.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs work » Reviewed & tested by the community

Failure in 1) Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest::testRemove, restoring RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 10.0.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 53783642d7 to 10.0.x and ef28407352 to 9.5.x and a92ee9af4e to 9.4.x. Thanks!

  • alexpott committed 5378364 on 10.0.x 
    Issue #3245553 by quietone, danflanagan8: Fix migration of localized D6...

  • alexpott committed ef28407 on 9.5.x 
    Issue #3245553 by quietone, danflanagan8: Fix migration of localized D6...

  • alexpott committed a92ee9a on 9.4.x 
    Issue #3245553 by quietone, danflanagan8: Fix migration of localized D6...

Status: Fixed » Closed (fixed)

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