i18n menu links translation in wrong directory [#2960013]

Problem/Motivation

I was excited this morning that the i18n menu links patch was committed but later I was getting this error when working on commerce migrate.
Warning: call_user_func() expects parameter 1 to be a valid callback, class 'Drupal\dynamic_page_cache\EventSubscriber\DynamicPageCacheSubscriber' does not have a method 'onRouteMatch' in Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch() (line 111 of core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php).
I tried enabling content_translation and, sure enough, the error went away.

During the long work to get i18n menu links done I often said to myself that the migration needed to move to content_translation, but I always didn't remember when at the keyboard. Sorry!

Just like d6_taxonomy_term_translation.yml d6_menu_links_translation.yml should be in content_translation, which should fix this problem.

There is a second part, in the the source plugin for both should also be in content_translation. The reason for that is because migrate_drupal_ui will use the source_module in the source plugin and then include that source_module in the list, which is should not.

Proposed resolution

At minimum, move d6_menu_links_translation.yml to content_translation

Remaining tasks

Write a patch
commit

Comment	File	Size	Author
#21	interdiff.txt	721 bytes	quietone
#21	2960013-21.patch	13.17 KB	quietone
#21	8.6.x: PHP 7 & MySQL 5.5 23,998 pass
#16	interdiff.txt	16.59 KB	quietone
#16	2960013-16.patch	13.2 KB	quietone
#16	8.6.x: PHP 7 & MySQL 5.5 24,016 pass
#12	interdiff.txt	1.48 KB	quietone
#12	2960013-12.patch	9.8 KB	quietone
#12	8.6.x: PHP 7 & MySQL 5.5 24,012 pass
#8	interdiff.txt	576 bytes	quietone
#8	2960013-8.patch	9.8 KB	quietone
#8	8.6.x: PHP 7 & MySQL 5.5 23,883 pass
#5	diff.txt	365 bytes	quietone
#5	2960013-5.patch	9.03 KB	quietone
#5	8.6.x: PHP 7 & MySQL 5.5 23,882 pass, 2 fail
#5	2960013-5-fail.patch	8.69 KB	quietone
#5	8.6.x: PHP 7 & MySQL 5.5 23,882 pass, 2 fail
#3	2960013-3.patch	351 bytes	jofitz
#3	8.6.x: PHP 7 & MySQL 5.5 23,880 pass, 2 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

11 April 2018 at 04:36

quietone created an issue. See original summary.

Comment #2

quietone CreditAttribution: quietone as a volunteer commented 11 April 2018 at 04:37

Title:

i18n translation in wrong directory

» i18n menu links translation in wrong directory

Add menu links to title

Comment #3

jofitz CreditAttribution: jofitz at ComputerMinds commented 11 April 2018 at 15:28

Status:

Active

» Needs review

File	Size
2960013-3.patch	351 bytes
8.6.x: PHP 7 & MySQL 5.5 23,880 pass, 2 fail

Moved d6_menu_links_translation.yml to content_translation.

Will this require any form of test?

Comment #4

11 April 2018 at 16:21

Status:

Needs review

» Needs work

The last submitted patch, 3: 2960013-3.patch, failed testing. View results

Comment #5

quietone CreditAttribution: quietone as a volunteer commented 11 April 2018 at 22:17

Status:

Needs work

» Needs review

File	Size
2960013-5-fail.patch	8.69 KB
8.6.x: PHP 7 & MySQL 5.5 23,882 pass, 2 fail
2960013-5.patch	9.03 KB
8.6.x: PHP 7 & MySQL 5.5 23,882 pass, 2 fail
diff.txt	365 bytes

Jo Fitzgerald, thanks. Yes, it probably does need a test. So, here is a test and a fail patch.

Comment #6

11 April 2018 at 23:09

The last submitted patch, 5: 2960013-5-fail.patch, failed testing. View results

Comment #7

11 April 2018 at 23:09

Status:

Needs review

» Needs work

The last submitted patch, 5: 2960013-5.patch, failed testing. View results

Comment #8

quietone CreditAttribution: quietone as a volunteer commented 11 April 2018 at 23:19

Status:

Needs work

» Needs review

File	Size
2960013-8.patch	9.8 KB
8.6.x: PHP 7 & MySQL 5.5 23,883 pass
interdiff.txt	576 bytes

And the migration test needs to enable content_translation.

Comment #9

jofitz CreditAttribution: jofitz at ComputerMinds commented 12 April 2018 at 08:56

@quietone You're a star!

Comment #10

mikeryan

he/him

English

Murphysboro, IL, USA

CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented 10 May 2018 at 21:23

Assigned:

Unassigned

» mikeryan

Comment #11

mikeryan

he/him

English

Murphysboro, IL, USA

CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented 14 May 2018 at 18:31

Assigned:	mikeryan	» Unassigned
Status:	Needs review	» Needs work

Just saw a couple minor comment issues.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradei18nReviewPageTestBase.php
@@ -0,0 +1,126 @@
+  public static $modules = ['migrate_drupal_ui'];
+  /**

Should be a blank line here.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradei18nReviewPageTestBase.php
@@ -0,0 +1,126 @@
+   * example modules. This means that we can test that the modules listed in the
+   * the $noUpgradePath property of the update form class are correct, since

Double "the".

Comment #12

quietone CreditAttribution: quietone as a volunteer commented 15 May 2018 at 08:13

Status:

Needs work

» Needs review

File	Size
2960013-12.patch	9.8 KB
8.6.x: PHP 7 & MySQL 5.5 24,012 pass
interdiff.txt	1.48 KB

@mikeryan, thanks for the review.

11.1 and 11.2 fixed.

Comment #13

mikeryan

he/him

English

Murphysboro, IL, USA

CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented 15 May 2018 at 15:01

Status:

Needs review

» Reviewed & tested by the community

Thanks!

Comment #14

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce commented 15 May 2018 at 20:24

Status:

Reviewed & tested by the community

» Needs review

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradei18nReviewPageTestBase.php
@@ -0,0 +1,127 @@
+  /**
+   * Tests the migrate upgrade review form.
+   *
+   * The upgrade review form displays a list of modules that will be upgraded
+   * and a list of modules that will not be upgraded. This test is to ensure
+   * that the review page works correctly for all contributed Drupal 6 and
+   * Drupal 7 modules that have moved to core, e.g. Views, and for modules that
+   * were in Drupal 6 or Drupal 7 core but are not in Drupal 8 core, e.g.
+   * Overlay.
+   *
+   * To do this all modules in the source fixtures are enabled, except test and
+   * example modules. This means that we can test that the modules listed in the
+   * $noUpgradePath property of the update form class are correct, since there
+   * will be no available migrations which declare those modules as their source
+   * module. It is assumed that the test fixtures include all modules that have
+   * moved to or dropped from core.
+   *
+   * The upgrade review form will also display errors for each migration that
+   * does not have a source_module definition. That function is not tested here.
+   *
+   * @see \Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeExecuteTestBase
+   */
+  public function testMigrateUpgradeReviewPage() {

This is copied from \Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeReviewPageTestBase::testMigrateUpgradeReviewPage() is there anyway we can refactor to share more code?

Comment #15

quietone CreditAttribution: quietone as a volunteer commented 15 May 2018 at 21:25

Status:

Needs review

» Needs work

Yes, it can be refactored but I was hoping to do that in a separate issue but can do here.

Comment #16

quietone CreditAttribution: quietone as a volunteer commented 17 May 2018 at 04:09

Status:

Needs work

» Needs review

File	Size
2960013-16.patch	13.2 KB
8.6.x: PHP 7 & MySQL 5.5 24,016 pass
interdiff.txt	16.59 KB

Did some refactoring to simplify the tests. The main points are that the declaration of all the modules to enable has been moved out of the test base classes. That should help anyone in contrib extending these tests. Hope it is enough.

Another was to extend the new MigrateUpgradei18nReviewPageTest from MigrateUpgradeReviewPageTest so that they could share a chunk of code in a new helper method, prepare(). Ideally that should go in setup() but it needs the source database which isn't loaded yet. I didn't work at that because I didn't want to get too far out of scope.

Let's see how this goes.

Edit: fix grammar and formatting

Comment #17

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce commented 17 May 2018 at 08:28

Looks good to me. Thanks @quietone.

Comment #18

mikeryan

he/him

English

Murphysboro, IL, USA

CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented 17 May 2018 at 14:30

Status:

Needs review

» Reviewed & tested by the community

Comment #19

mikeryan

he/him

English

Murphysboro, IL, USA

CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented 17 May 2018 at 14:31

Assigned:	Unassigned	» mikeryan
Status:	Reviewed & tested by the community	» Needs review

Comment #20

mikeryan

he/him

English

Murphysboro, IL, USA

CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented 17 May 2018 at 14:48

Assigned:	mikeryan	» Unassigned
Status:	Needs review	» Needs work

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradei18nReviewPageTestBase.php
@@ -0,0 +1,45 @@
+ * Provides a base class for testing Uprade review form without translations.

Typo: "Uprade". And, I think there should be a "the" before upgrade.

Not sure it should be capitalized?

Other than that comment, looks good.

Comment #21

quietone CreditAttribution: quietone as a volunteer commented 17 May 2018 at 21:19

Status:

Needs work

» Needs review

File	Size
2960013-21.patch	13.17 KB
8.6.x: PHP 7 & MySQL 5.5 23,998 pass
interdiff.txt	721 bytes

@alexpott, your welcome.

Added the suggested changes in #20 and the summary line was > 80 so I removed the beginning phrase.

Comment #22

mikeryan

he/him

English

Murphysboro, IL, USA

CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented 17 May 2018 at 22:45

Status:

Needs review

» Reviewed & tested by the community

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradei18nReviewPageTestBase.php
@@ -0,0 +1,45 @@
+ * Tests the uprade review form without translations.

Ummm...

But, that can be fixed on commit...

Comment #23

quietone CreditAttribution: quietone as a volunteer commented 18 May 2018 at 00:02

Yea, sorry I stared at it for a bit and nothing better came forth.

Comment #24

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce commented 18 May 2018 at 07:31

Status:

Reviewed & tested by the community

» Fixed

Fixed uprade on commit. I also fixed the capitalisation of i18n in class names to follow our camel case format. So MigrateUpgrade6i18nReviewPageTest becomes MigrateUpgrade6I18nReviewPageTest because if we not using the shorthand this would be Internationalisation.

Committed and pushed 9f76972c25 to 8.6.x and b80873dcf5 to 8.5.x. Thanks!

Comment #25

18 May 2018 at 07:31

alexpott committed 9f76972 on 8.6.x

Issue #2960013 by quietone, Jo Fitzgerald, mikeryan, alexpott: i18n menu...

Comment #26

18 May 2018 at 07:31

alexpott committed b80873d on 8.5.x

Issue #2960013 by quietone, Jo Fitzgerald, mikeryan, alexpott: i18n menu...

Comment #27

18 May 2018 at 09:59

alexpott committed 6e5d6e3 on 8.5.x

Revert "Issue #2960013 by quietone, Jo Fitzgerald, mikeryan, alexpott:...

Comment #28

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC, Acro Commerce commented 30 May 2018 at 11:59

Does this need to be back ported to 8.5? It is a bug and our test coverage between 8.5 and 8.6 is effected on commerce migrate.

Comment #29

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC, Acro Commerce commented 30 May 2018 at 12:01

See #2976114: Remove hack for test failures on HEAD for 8.6

Comment #30

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce commented 30 May 2018 at 13:12

@heddn yeah I originally backported this but it has BC implications - as your test proves.

Comment #31

13 June 2018 at 13:14

Status:

Fixed

» Closed (fixed)

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

i18n menu links translation in wrong directory

Problem/Motivation

Proposed resolution

Remaining tasks

Comments

Referenced by