Upgrade path for Book 6.x and 7.x

Added organization support to Anexus IT

Will need to be submitted again to Core since moving from sandbox.

Changed the issue summary.

I'll get a test in for this within the next few days.

Added a test and made a tweak to migrate.migrations.d7_books_settings.yml to account for migration_groups becoming migration_tags.

Postponing on #2382985: Migration Files for Drupal 7 Menu & Menu Entries and #2423103: Migration Files for Drupal 7 Content.

#6 is a great start, but I should have done my research before posting this issue -- because we don't only need to migrate book settings, but book hierarchies (i.e., the {book} table). Books depend on nodes and menu links being migrated first, so those need to be in before we can proceed on this.

Erm -- yeah, status change. :)

Ok, that's in.

Attached patch is my work in progress.
Still todo:
* continue updating D7 MigrateBookTest (blocked by missing helper functions in MigrateDrupal7TestBase currently)
* check if the config migration code is still correct

Postponing on #2578263: Add helper functions to MigrateDrupal7TestBase

I would argue this is at least medium priority, book migrations are meaningless without the hierarchy, and book is pretty widely used (for instance on D.O.). I also don't think this is postponed on anything now.

OK I guess it is still postponed on https://www.drupal.org/node/2578263.

I couldn't get the previous patch to apply so I did a reroll and it worked great to migrate the book heirarchy for a D7 site with a fairly complex book structure. I don't have a D6 site to test so hopefully someone else can confirm that part. Then we just need the tests finalized.

EDIT: I finally got the original patch to apply, so the reroll may not be needed.

Now that #2382985 and #2423103 have landed, this can be worked on again.

I guess the patches here won't apply anymore.

Re-rolled.

D7 tests are a work in progress.

Setting back to NW for D7 tests (and probably other things too!)

Completed the tests for d7/MigrateBookConfigsTest, but I'm tying myself in knots with d7/MigrateBookTest so I'll leave that for someone else.

Setting back to NW for someone (else!) to work on d7/MigrateBookTest.

Patch in #27 no longer applies. Re-rolled prior to taking another run at d7/MigrateBookTest.

Wrote d7/MigrateBookTest.

Moved new migration templates to the correct directory.

Added source_module to d7_book_settings.

So, that patch applies to 8.5, but doesn't work for 8.4. Here's one for 8.4.

Fix the test failures.

Retest has passed, returning to NR.

Having trouble actually using this patch. Following the migration template, I get hundreds of these errors:

Missing bundle for entity type node (/var/www/build/html/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:73)                                          
Missing bundle for entity type node (/var/www/build/html/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:73)                                          
Missing bundle for entity type node (/var/www/build/html/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:73)                                          

That's running it stand-alone, after the migration for the book content type has been run.

Or, if I substitute 'book' as the source/destination plugin for the book content type, adding in the fields from the book migration template, that doesn't work either. I get these errors:
Missing bundle for entity type node (/var/www/build/html/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:73)

How is this supposed to be used?

I see very little to object to here. Once these minor fixes are made, let's get this in.

1. +++ b/core/modules/book/migrations/d7_book.yml
   @@ -0,0 +1,24 @@
   +    -
   +      plugin: migration
   +      migration: d7_book
   

   plugin should be migration_lookup.

2. +++ b/core/modules/book/tests/src/Kernel/Migrate/d7/MigrateBookConfigsTest.php
   @@ -0,0 +1,41 @@
   +/**
   + * Upgrade variables to book.settings.yml.
   

   Should be "Tests the migration of Book settings."

3. +++ b/core/modules/book/tests/src/Kernel/Migrate/d7/MigrateBookTest.php
   @@ -0,0 +1,70 @@
   +/**
   + * Upgrade book structure.
   

   Should be "Tests migration of book structures from Drupal 7."

Made changes recommended by @phenaproxima in #53.

Gah! I managed to not move a couple of files.

Correct test failure.

We're done here. Thanks, @Jo Fitzgerald et. al!

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7Test.php
@@ -154,7 +155,6 @@ protected function getAvailablePaths() {
diff --git a/patches/interdiff-56-58.txt b/patches/interdiff-56-58.txt

diff --git a/patches/interdiff-56-58.txt b/patches/interdiff-56-58.txt
new file mode 100644

new file mode 100644
index 0000000..0416e00

index 0000000..0416e00
--- /dev/null

--- /dev/null
+++ b/patches/interdiff-56-58.txt

+++ b/patches/interdiff-56-58.txt
+++ b/patches/interdiff-56-58.txt
@@ -0,0 +1,20 @@

@@ -0,0 +1,20 @@
+diff --git a/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7ReviewPageTest.php b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7ReviewPageTest.php
+index b8ea52b..be048f9 100644
+--- a/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7ReviewPageTest.php
++++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7ReviewPageTest.php
+@@ -34,6 +34,7 @@ protected function getAvailablePaths() {
+     return [
+       'aggregator',
+       'block',
++      'book',
+       'comment',
+       'contact',
+       'date',
+@@ -115,7 +116,6 @@ protected function getAvailablePaths() {
+    */
+   protected function getMissingPaths() {
+     return [
+-      'book',
+       'color',
+       'rdf',
+       'views',

some patch noise here?

Other than that, looks good

Remove noise.

And noise is now silent.

+++ b/core/modules/book/migrations/d7_book_settings.yml
similarity index 95%
rename from core/modules/book/src/Plugin/migrate/source/d6/Book.php

rename from core/modules/book/src/Plugin/migrate/source/d6/Book.php
rename to core/modules/book/src/Plugin/migrate/source/Book.php

Migrate API is in beta soon to be stable. I don't think we should be removing plugins because it will break existing migrations. Let's leave the old plugin in place but deprecate it and use the new plugin for both d6 and d7.

+++ b/core/modules/book/migrations/d6_book.yml
@@ -21,3 +21,4 @@ destination:
+    - d6_menu_links

Why this change to the D6 migration and how come no test changes because of it?

• Deprecated d6/Book.
• There appears to be no need for the addition of d6_menu_links as a required migration dependancy of d6_book so removed that (and the equivalent in d7_book).

The menu links dependency was added in #11 without explanation. It'd be good to try and reason why.

Re #65: It was probably added because of the comment in #7. @phenaproxima can you recall what caused you to state that?

Needs work to address the feedback in #65.

It was 3+ years ago, so I can't say I remember my reasoning. I say, let's try removing the menu links dependency, and if tests fail we can add it back again and do a little more detective work.

The latest patch removed the menu_links dependency (and passed the tests) so are we happy for this to be RTBC?

Nice work. We're migrating the book settings and book content. We have tests. Wonderful! This would be ready. Except, this needs a partial re-roll to account for https://www.drupal.org/node/2944527

Adding those tags seem easy enough. Tagging novice. The d6/d7_book yml files are content stuff. While d7_book_settings.yml is configuration.

Re-titling, since we seem to be doing both d6 and d7 in the same patch.

Does this need a change record or any release notes mention?

Working on this.

rerolled and applied the migration tags.

Probably a namespace thing. Check to make sure it includes the correct name to match the file system path of the class. Plus, it looks like we missed d6_book.yml as well.

Yes @heddn its a namespace issue. adding a updated patch for this issue.

changing status to Needs Review.

Thanks, @Yogesh Pawar! Can you post an interdiff so we can confirm the changes?

Alright, let's try this on for size...

1. +++ b/core/modules/book/migrations/book_settings.yml
   --- a/core/modules/book/migrations/d6_book.yml
   +++ b/core/modules/book/migrations/d6_book.yml
   
   +++ b/core/modules/book/migrations/d6_book.yml
   copy from core/modules/book/migrations/d6_book.yml
   copy to core/modules/book/migrations/d7_book.yml
   

   How come we cannot consolidate this down to a single migration as well? We did for the settings.

2. +++ b/core/modules/book/src/Plugin/migrate/source/d6/Book.php
   @@ -2,7 +2,9 @@
   +@trigger_error('Book is deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.x. Use \Drupal\book\Plugin\migrate\source\Book instead.', E_USER_DEPRECATED);
   
   @@ -11,54 +13,8 @@
   + * @deprecated in Drupal 8.6.x, to be removed before Drupal 9.0.x. Use
   + * \Drupal\book\Plugin\migrate\source\Book instead.
   

   Needs a link to a CR.

3. +++ b/core/modules/book/tests/src/Kernel/Migrate/d6/MigrateBookConfigsTest.php
   @@ -24,6 +24,9 @@ class MigrateBookConfigsTest extends MigrateDrupal6TestBase {
        $this->executeMigration('d6_book_settings');
   

   can we add another test the new name too? maybe a case for data provider?

How come we cannot consolidate this down to a single migration as well? We did for the settings.

I tried, but I don't think I can do it because of the migration dependencies. The D7 version has a dependency on d7_node, and the D6 version depends on d6_node, and that's that. There's no way, really, for me to define that dynamically except with a deriver, which would negate the entire purpose of consolidating the migrations (i.e., to remove code). Optional dependencies also won't work because, for example, the D6 migration should never depend on d7_node, but it will if d7_node exists and is set as an optional dependency.

I'll address the remaining feedback tonight.

Re #83: Good points. Perhaps a comment in the yml file would assist here then?

Tagging for a change record, as mentioned in #82.

Change record written: https://www.drupal.org/node/2947487

Addressed the feedback in #82 and then some.

Change record looks good to me.

Found one spelling error. Hopefully that can be fixed on commit?

+++ b/core/modules/book/book.module
@@ -539,3 +539,16 @@ function book_node_type_update(NodeTypeInterface $type) {
+  // book_setttings migration existed, so to maintain backwards compatibility,

s/setttings/settings

Looks like everything is done here. There is a deprecation notice for d6_book, complete with a link to the correct CR, and tests for that. The reason why the dependency - d6_menu_links has been lost in history, and its removal hasn't caused any test failures. The new 'Content' tag has been added to both migrations.

Actually there is one question not answered, #84, about whether a comment should be added to the yml files explaining why d6_book and d7_book aren't combined. I think the answer to that is that we have not been adding such comments to the migrations that are different between d6 and d7 (see aggregator) so it isn't necessary here.

i agree with #89's answer to #84.

diff --git a/core/modules/book/book.module b/core/modules/book/book.module
index f63a4596e4..1b40475c1d 100644
--- a/core/modules/book/book.module
+++ b/core/modules/book/book.module
@@ -546,7 +546,7 @@ function book_node_type_update(NodeTypeInterface $type) {
 function book_migration_plugins_alter(array &$migrations) {
   // Book settings are migrated identically for Drupal 6 and Drupal 7. However,
   // a d6_book_settings migration already existed before the consolidated
-  // book_setttings migration existed, so to maintain backwards compatibility,
+  // book_settings migration existed, so to maintain backwards compatibility,
   // ensure that d6_book_settings is an alias of book_settings.
   if (isset($migrations['book_settings'])) {
     $migrations['d6_book_settings'] = &$migrations['book_settings'];

Fixed on commit.

Committed and pushed b190c705bc to 8.6.x and a1d6ed6207 to 8.5.x. Thanks!

Credited people who reviewed the patch and suggested material changes.

Published change record