Harden migrations of Recipe 7.x-1.x

This is hardening the migration that #2908884: Migrate data from 7.x-2.x to 8.x-2.x introduced 👍🥳

Review of #12 (part one):

1. deleted file mode 100644
   index 8fcfa86..0000000
   
   index 8fcfa86..0000000
   --- a/migrations/recipe71_recipe.yml
   
   --- a/migrations/recipe71_recipe.yml
   +++ /dev/null
   
   deleted file mode 100644
   index 187c483..0000000
   
   index 187c483..0000000
   --- a/migrations/recipe71_recipe_translation.yml
   
   --- a/migrations/recipe71_recipe_translation.yml
   +++ /dev/null
   

   This entirely removes the previous recipe71_recipe node & recipe71_recipe_translation node translation migrations. Why?
   1. Because these were creating additional node revisions on the destination site (this breaks revision history).
   2. They didn't care of updating previous revisions.
   3. They were unaware that the preferred node migration method is executing d7_node_complete migrations (which are migrating every single node revision including all the revision translations as well).

2. +++ b/recipe.module
   @@ -415,3 +470,90 @@ function template_preprocess_recipe_view_recipeml(&$variables) {
   +
   +/**
   + * Implements hook_migration_plugins_alter().
   + */
   +function recipe_migration_plugins_alter(array &$migrations) {
   ...
   +  foreach ($node_migrations as $plugin_id => $plugin_definition) {
   +
   +    $migrations[$plugin_id]['process']['recipe_source'] = 'source';
   +    $migrations[$plugin_id]['process']['recipe_yield_amount'] = 'yield';
   +    $migrations[$plugin_id]['process']['recipe_yield_unit'] = 'yield_unit';
   +    $migrations[$plugin_id]['process']['recipe_description'] = 'description';
   +    $migrations[$plugin_id]['process']['recipe_instructions'] = 'instructions';
   +    $migrations[$plugin_id]['process']['recipe_notes'] = 'notes';
   +    $migrations[$plugin_id]['process']['recipe_prep_time'] = 'preptime';
   +    $migrations[$plugin_id]['process']['recipe_cook_time'] = 'cooktime';
   +
   +    $migrations[$plugin_id]['migration_tags'][] = 'Recipe node';
   +    $migrations[$plugin_id]['migration_dependencies']['required'] = array_unique(array_merge($migrations[$plugin_id]['migration_dependencies']['required']
   +        ?? [], ['recipe1x_ingredient']));
   +  }
   

   Instead of using the removed standalone migrations, the patch simply adds the required field process pipeline to recipe node migrations...

3. ...
   +/**
   + * Implements hook_migrate_prepare_row().
   + */
   +function recipe_migrate_prepare_row(Row $row, MigrateSourceInterface $source, MigrationInterface $migration) {
   ...
   +  $recipe_values = $source->getDatabase()->select('recipe', 'r')
   +    ->fields('r', [
   +      'source',
   +      'yield',
   +      'yield_unit',
   +      'description',
   +      'instructions',
   +      'notes',
   +      'preptime',
   +      'cooktime',
   +    ])
   +    ->condition('r.nid', $source_node_id)
   +    ->execute()
   +    ->fetchAll(\PDO::FETCH_ASSOC);
   +
   +  foreach (reset($recipe_values) as $property => $property_value) {
   +    $row->setSourceProperty($property, $property_value);
   +  }
   

   ...and adds the actual migration row source properties (representing recipe field values) in a migrate prepare hook.

4. +++ b/modules/ingredient/ingredient.module
   @@ -129,11 +138,87 @@ function ingredient_quantity_from_fraction($ingredient_quantity) {
   +/**
   + * Implements hook_migration_plugins_alter().
   + */
   +function ingredient_migration_plugins_alter(array &$migrations) {
   ...
   +  foreach ($node_migrations as $plugin_id => $plugin_definition) {
   +    $process = [
   +      'plugin' => 'sub_process',
   +      'source' => 'all_ingredients',
   +      'process' => [
   +        'target_id' => 'ingredient_id',
   +        'quantity' => 'quantity',
   +        'unit_key' => 'unit_key',
   +        'note' => 'note',
   +      ],
   +    ];
   +    $migrations[$plugin_id]['process']['recipe_ingredient'] = $process;
   ...
   +++ b/modules/ingredient/ingredient.module
   @@ -129,11 +138,87 @@ function ingredient_quantity_from_fraction($ingredient_quantity) {
   +/**
   + * Implements hook_migrate_prepare_row().
   + */
   +function ingredient_migrate_prepare_row(Row $row, MigrateSourceInterface $source, MigrationInterface $migration) {
   ...
   +  $ingredient_values = $source->getDatabase()->select('recipe_node_ingredient', 'r')
   +    ->fields('r', ['ingredient_id', 'quantity', 'unit_key', 'note'])
   +    ->condition('r.nid', $source_node_id)
   +    ->orderBy('r.weight', 'ASC')
   +    ->execute()
   +    ->fetchAll(\PDO::FETCH_ASSOC);
   +
   +  $row->setSourceProperty('all_ingredients', $ingredient_values);
   

   Same happens in ingredients submodule.

5. +++ b/modules/ingredient/ingredient.module
   @@ -129,11 +138,87 @@ function ingredient_quantity_from_fraction($ingredient_quantity) {
   +  $system_data = $node_source_plugin->getSystemData();
   +  $recipe_module_is_installed = !empty($system_data['module']['recipe']['status']);
   +
   +  $recipe_module_schema = (int) ($system_data['module']['recipe']['schema_version'] ?? NULL);
   +  if (!$recipe_module_is_installed || ($recipe_module_schema < 7000) || ($recipe_module_schema >= 7200)) {
   +    return;
   +  }
   

   But the patch only fixes the Recipe 7.x-1.x → Recipe 8.x-2.x migration path.

6. +++ b/tests/src/Kernel/Migrate/recipe71/MigrateRecipe71Test.php
   @@ -83,8 +73,18 @@ class MigrateRecipe71Test extends MigrateRecipe71TestBase {
   +   *
   +   * @dataProvider providerTestRecipe
   
   @@ -116,4 +116,23 @@ class MigrateRecipe71Test extends MigrateRecipe71TestBase {
   +  /**
   +   * Tests node migration.
   +   */
   +  public function providerTestRecipe() {
   +    return [
   +      'obsolete_node_migrations' => [
   +        'node_migrations' => [
   +          'd7_node',
   +          'd7_node_translation',
   +        ],
   +      ],
   +      'complete_node_migrations' => [
   +        'node_migrations' => [
   +          'd7_node_complete',
   +        ],
   +      ],
   +    ];
   +  }
   

   Test is updated with a @dataProvider which tests recipe node migrations with the obsolete (d7_node, d7_node_translation) and either with complete (d7_node_complete) migrations.

7. +++ b/tests/src/Kernel/Migrate/recipe71/MigrateRecipeDisplaySettings71Test.php
   @@ -33,8 +41,8 @@ class MigrateRecipeDisplaySettings71Test extends MigrateRecipe71TestBase {
   -    $this->assertSame($field_component['settings']['fraction_format'], '{%d }%d/%d');
   -    $this->assertSame($field_component['settings']['unit_display'], 1);
   +    $this->assertSame('{%d }%d/%d', $field_component['settings']['fraction_format']);
   +    $this->assertSame(1, $field_component['settings']['unit_display']);
   

   These are only simple argument order change:
   PHPUnit\Framework\Assert::assertSame()'s first argument should be the expected value, and the second one should be the actual value.

There are many-many coding standard fixes in the patch which are out of the scope of the current issue. Could you please remove them?

• +++ b/modules/ingredient/ingredient.module
  @@ -5,8 +5,15 @@
   /**
    * Implements hook_theme().
  @@ -93,10 +100,11 @@ function ingredient_is_page(Ingredient $ingredient) {
  
  @@ -93,10 +100,11 @@ function ingredient_is_page(Ingredient $ingredient) {
    * @return string
    *   The ingredient quantity formatted as a fraction.
    *
  - * @deprecated This function was deprecated in version 8.x-2.0 and will be
  - *   removed in version 3.0.0.  Use
  - *   \Drupal\ingredient\Utility\IngredientQuantityUtility::getQuantityFromDecimal()
  + * @deprecated in project:8.x-2.0 and is removed from project:3.0.0. Use
  + *   \Drupal\ingredient\Utility\IngredientUnitFuzzymatch::getUnitFuzzymatch()
    *   instead.
  + *
  + * @see https://www.drupal.org/project/recipe/issues/3156825
    */
   function ingredient_quantity_from_decimal($ingredient_quantity, $fraction_format = '{%d} %d⁄%d', $edit_mode = FALSE) {
  

  Keep the original.

• +++ b/modules/ingredient/ingredient.module
  @@ -104,17 +112,18 @@ function ingredient_quantity_from_decimal($ingredient_quantity, $fraction_format
  -
  + *
  

  Keep the original.

• +++ b/modules/ingredient/ingredient.module
  @@ -104,17 +112,18 @@ function ingredient_quantity_from_decimal($ingredient_quantity, $fraction_format
   function ingredient_quantity_from_fraction($ingredient_quantity) {
  

  Keep the original.

• +++ b/modules/ingredient/ingredient.module
  @@ -129,11 +138,87 @@ function ingredient_quantity_from_fraction($ingredient_quantity) {
  - * @deprecated This function was deprecated in version 8.x-2.0 and will be
  - *   removed in version 3.0.0.  Use
  + * @deprecated in project:8.x-2.0 and is removed from project:3.0.0. Use
    *   \Drupal\ingredient\Utility\IngredientUnitFuzzymatch::getUnitFuzzymatch()
    *   instead.
  + *
  + * @see https://www.drupal.org/project/recipe/issues/3156825
    */
   function ingredient_unit_fuzzymatch($subject) {
  

  Keep the original.

• +++ b/recipe.module
  @@ -153,6 +175,21 @@ function recipe_node_view(array &$build, EntityInterface $entity, EntityViewDisp
  +/**
  + * Calculates total time taken.
  + *
  + * @param array $build
  + *   A renderable array containing build information and context for an entity
  + *   view.
  + * @param Drupal\Core\Entity\EntityInterface $entity
  + *   Entity on which the function should operate upon.
  + * @param Drupal\Core\Entity\Display\EntityViewDisplayInterface $display
  + *   The entity view display holding the display options configured for the
  + *   entity components.
  + * @param string $view_mode
  + *   (optional) The view mode that should be used to display the entity.
  + *   Defaults to 'full'.
  + */
   function recipe_build_total_time(array &$build, EntityInterface $entity, EntityViewDisplayInterface $display, $view_mode) {
  
  @@ -174,11 +211,29 @@ function recipe_build_total_time(array &$build, EntityInterface $entity, EntityV
  -      '#total_time' => ['#theme' => 'recipe_duration', '#duration' => $total_time],
  +      '#total_time' => [
  +        '#theme' => 'recipe_duration',
  +        '#duration' => $total_time,
  +      ],
  

  Added a missing function doc...

• +++ b/recipe.module
  @@ -174,11 +211,29 @@ function recipe_build_total_time(array &$build, EntityInterface $entity, EntityV
  +/**
  + * Builds the recipe_yield field.
  + *
  + * @param array $build
  + *   A renderable array containing build information and context for an entity
  + *   view.
  + * @param Drupal\Core\Entity\EntityInterface $entity
  + *   Entity on which the function should operate upon.
  + * @param Drupal\Core\Entity\Display\EntityViewDisplayInterface $display
  + *   The entity view display holding the display options configured for the
  + *   entity components.
  + * @param string $view_mode
  + *   (optional) The view mode that should be used to display the entity.
  + *   Defaults to 'full'.
  + */
  

  ..and again.

• +++ b/recipe.module
  @@ -209,7 +264,7 @@ function recipe_build_yield(array &$build, EntityInterface $entity, EntityViewDi
  -function template_preprocess_recipe_total_time(&$variables, $hook) {
  +function template_preprocess_recipe_total_time(array &$variables, $hook) {
  
  @@ -232,7 +287,7 @@ function template_preprocess_recipe_total_time(&$variables, $hook) {
  -function template_preprocess_recipe_yield(&$variables, $hook) {
  +function template_preprocess_recipe_yield(array &$variables, $hook) {
  
  @@ -284,7 +339,7 @@ function recipe_entity_extra_field_info() {
  -function template_preprocess_recipe_duration(&$variables) {
  +function template_preprocess_recipe_duration(array &$variables) {
  
  @@ -300,7 +355,7 @@ function template_preprocess_recipe_duration(&$variables) {
  - * @param $duration
  + * @param int $duration
  
  @@ -401,7 +456,7 @@ function template_preprocess_recipe_view_plain_text(&$variables) {
  -function template_preprocess_recipe_view_recipeml(&$variables) {
  +function template_preprocess_recipe_view_recipeml(array &$variables) {
  

  A lot of minor fixes.

• +++ b/tests/src/Kernel/Migrate/recipe61/MigrateRecipe61Test.php
  @@ -56,7 +57,7 @@ class MigrateRecipe61Test extends MigrateRecipe61TestBase {
  -   * @param \stdClass $recipe
  +   * @param object $recipe
  

  ..as well as this one.

• +++ b/tests/src/Kernel/Migrate/recipe71/MigrateRecipe71Test.php
  @@ -53,7 +43,7 @@ class MigrateRecipe71Test extends MigrateRecipe71TestBase {
  -   * @param \stdClass $recipe
  +   * @param object $recipe
  

  ...

I want to ask for two things:

1. @srishti.bankar, it is clear that you have put a tremendous amount of work into fixing coding standard errors as well. But unfortunately they're making it very difficult to review the patch. 🤯

   Could you please open a new issue for fixing these CS violations? And then please undo every single unrelated coding standard fixes what you've made in #12. If you do this, then the reversed interdiff could be uploaded to the newly created issue, and the tremendous work you have put into it would not be lost 🥳.

2. 🤔 Patch #12 only fixes Recipe 7.x-1.x → Recipe 8.x-2.x migration path (see #15.5). Can we decrease the scope of this issue? Let's say "Harden migrations of Recipe 7.x-1.x". Doing so may help this patch to be committed much sooner 🥳.

1. +++ b/modules/ingredient/ingredient.module
   @@ -137,3 +144,78 @@ function ingredient_quantity_from_fraction($ingredient_quantity) {
   +function ingredient_migration_plugins_alter(array &$migrations) {
   +  $node_source_plugin = NULL;
   +  try {
   +    $node_source_plugin = MigrationDeriverTrait::getSourcePlugin('d7_node');
   +  }
   +  catch (PluginNotFoundException $e) {
   +  }
   +  catch (RequirementsException $e) {
   +  }
   
   +++ b/recipe.module
   @@ -415,3 +422,90 @@ function template_preprocess_recipe_view_recipeml(&$variables) {
   +  catch (PluginNotFoundException $e) {
   +  }
   +  catch (RequirementsException $e) {
   +  }
   

   It would be very nice tho add some comments here why we suppress these exceptions.

2. +++ b/modules/ingredient/tests/src/Kernel/Migrate/recipe72/MigrateIngredient72Test.php
   @@ -66,7 +66,6 @@ class MigrateIngredient72Test extends MigrateDrupal7TestBase {
   -    $this->assertSame(1, $field_component['settings']['unit_display']);
   

   Why was this assertion removed? It is definitely unrelated to 7.x-1.x recipe migrations.

3. +++ b/recipe.module
   @@ -415,3 +422,90 @@ function template_preprocess_recipe_view_recipeml(&$variables) {
   +  catch (\Exception $e) {
   +  }
   

   We shouldn't suppress any unrelated (unknown or unforeseen) exceptions. Please remove!

4. +++ b/recipe.module
   @@ -415,3 +422,90 @@ function template_preprocess_recipe_view_recipeml(&$variables) {
   +
   +  foreach ($node_migrations as $plugin_id => $plugin_definition) {
   +
   +    $migrations[$plugin_id]['process']['recipe_source'] = 'source';
   +    $migrations[$plugin_id]['process']['recipe_yield_amount'] = 'yield';
   +    $migrations[$plugin_id]['process']['recipe_yield_unit'] = 'yield_unit';
   +    $migrations[$plugin_id]['process']['recipe_description'] = 'description';
   +    $migrations[$plugin_id]['process']['recipe_instructions'] = 'instructions';
   

   Could you please remove the blank line at the first line of foreach loop's body?

For other reviewers:

The "new" coding standard error isn't new, it complains about this line: https://git.drupalcode.org/project/recipe/-/blob/8.x-2.x/recipe.module#L177

I guess this wasn't picked up when the dev branch was tested in January (9 months ago).

Addressing #18.2

This patch restores the original state of the migration tests for Recipe 6.x-1.x and Recipe 7.x-2.x.

Because of #3067979: Exclude test files from release packages, the patch in #20 does not apply to packaged releases!!!!!!!!!! 🤯

This forces me to upload a version of #20 without test changes.

This patch has been ready since #20 — everything in @huzooka's reviews has been addressed, and there is test coverage to prove it works!

When the module is tested on PHP 8.1 the 7.x-1.x migration starts causing one of those cryptic "Uncaught AssertionError: The container was serialized." errors. Since this patch eliminates that migration, I decided to review it finally.

1. +++ b/modules/ingredient/ingredient.module
   @@ -137,3 +144,80 @@ function ingredient_quantity_from_fraction($ingredient_quantity) {
   +  $node_source_plugin = NULL;
   +  try {
   +    $node_source_plugin = MigrationDeriverTrait::getSourcePlugin('d7_node');
   +  }
   +  catch (PluginNotFoundException $e) {
   +  }
   +  catch (RequirementsException $e) {
   +  }
   +
   +  if (!$node_source_plugin) {
   +    return;
   +  }
   +
   

   Using a trait outside the scope of a class is deprecated. So I'm copying the code from that trait into the try block. I think we can also simplify the logic here and just return if any exception is thrown.

2. +++ b/modules/ingredient/ingredient.module
   @@ -137,3 +144,80 @@ function ingredient_quantity_from_fraction($ingredient_quantity) {
   +    $migrations[$plugin_id]['migration_tags'][] = 'Recipe node';
   

   I'm slightly concerned about naming collisions here, so I'm making the tag name more specific. It probably doesn't matter at all, but it also won't hurt to tweak it.

3. +++ b/modules/ingredient/tests/src/Kernel/Migrate/recipe71/MigrateIngredient71Test.php
   @@ -16,7 +16,7 @@ class MigrateIngredient71Test extends MigrateIngredient71TestBase {
   -  protected static $modules = ['ingredient'];
   +  protected static $modules = ['ingredient', 'node'];
   

   I don't understand the reason for this change.

4. +++ b/modules/ingredient/tests/src/Kernel/Migrate/recipe71/MigrateIngredientSettings71Test.php
   @@ -12,7 +12,7 @@ class MigrateIngredientSettings71Test extends MigrateIngredient71TestBase {
   -  protected static $modules = ['ingredient'];
   +  protected static $modules = ['ingredient', 'node', 'text'];
   
   +++ b/tests/src/Kernel/Migrate/recipe71/MigrateRecipe71Test.php
   @@ -16,6 +16,7 @@ class MigrateRecipe71Test extends MigrateRecipe71TestBase {
   +    'comment',
   

   Or this change.

5. +++ b/tests/src/Kernel/Migrate/recipe71/MigrateRecipeDisplaySettings71Test.php
   @@ -24,6 +31,7 @@ class MigrateRecipeDisplaySettings71Test extends MigrateRecipe71TestBase {
   +    $this->installSchema('node', 'node_access');
   

   Or this change and the corresponding addition of the node_access table to the fixture.

6. And I think there might be one other that Dreditor didn't copy in properly.

Removing those changes didn't cause the tests to fail locally, so I'm taking them out of the patch.

I missed removing an additional fixture change for the node_access table.

Thank you all for your contributions!