Migrations fail due to missing dependency when dependency has skipped rows by the source plugin

I'm having the exact same issue on 8.2.2

vagrant@moduleupgrade:/var/www/drupalvm/web$ drush mi module_upgrade_migrate_project_releases --execute-dependencies
Processed 0 items (0 created, 0 updated, 0 failed, 0 ignored) - done with                   [status]
'module_upgrade_migrate_projects'
Migration module_upgrade_migrate_project_releases did not meet the requirements. Missing    [error]
migrations module_upgrade_migrate_projects. requirements: module_upgrade_migrate_projects.

Note the reason nothing was imported we because there was nothing updated... but running the migration with --execute-dependencies does in fact execute the migration that is a dependency, but then fails the requirements check.

I've attached the 2 migrations for study. You can download the tsv file from https://drupal.org/files/releases.tsv

I'm having this issue too in 8.3.2. Might be a cache problem?
The second migration requires the first one, and the first one is executed properly.

/** @var \Drupal\migrate\Plugin\MigrationPluginManagerInterface $manager */
    $manager = \Drupal::service('plugin.manager.migration');
    foreach ($migration_ids as $migration_id) {
      /** @var \Drupal\migrate\Plugin\MigrationInterface $migration */
      $migration = $manager->createInstance($migration_id);
      $log = new CustomMigrateMessage();
      $executable = new MigrateExecutable($migration, $log);
      $executable->import();
    }

I ran the same issue. I tried to import a migration that requires another migration but for some migration I encountered this issue

Processed 5 items (5 created, 0 updated, 0 failed, 0 ignored) - done with 'project_applications'                                                                   [status]
Migration project_product did not meet the requirements. Missing migrations project_applications. requirements: project_applications.                  [error]

I had a look at how migrate checks dependencies and this is done in checkRequirements() /core/modules/migrate/src/Plugin/Migration.php. First thing, Migrate checks if the required migration exists which is our case. Second things, for each required migration, the allRowsProcessed() method is run and this is where the test failed in my case. A count of available rows from the source file is compared to the number of imported rows which is not the same in my case due to the plugin I use that does not take care of duplicate values.

As a workaround, you can change your migration dependencies from required to optional. This will allow you to execute migration in a specific order without having the strong validation offered by migrate.

from :

migration_dependencies:
  required:
    - project_applications

to:

migration_dependencies:
  optional:
    - project_applications

Regarding this, the issue is not a bug from my point of view. You should maybe look at your source plugin and discover how it counts rows.

Probably related? #2878858: Migration did not meet the requirements when using cache.backend.null

Supports @edurenye thought that it might be a caching issue...

Experiencing the same issue. Similar as to reported in #10.

One of my migrations, which is a depency of another migration, skips specific source rows in prepareRow() (by returning FALSE). This however raises the issue mentioned inhere.

Use this patch for a migration requirement to be met if at least one row has been processed.

USE AT YOUR OWN RISK.

Some migration configurations may fail if some rows are not imported, specially when migration_lookup or translations are involved

Patch at #13 worked for me. It would be good to have a more precise error message. In my case the migrations were not "missing" but were "incomplete". It would be good to have error messages reflecting those as distinct scenarios.

In my case I had one dependent migration that skipped one row.

The patch in #13 would work for me but it would be problematic when my depending migration (Bar) fails, with the patch my dependent migration (Foo) would start eventhough (Bar) failed (but has one successful row for example).

If we also check the migration status we still have an issue for migrations that either have no content or skip all rows (status would be true, so in theory the dependency is met).

discussed with kim.kennof on slack:
better would be that a skipped row would also be in the mapping table, and countable etc
so a 95% imported + 5% skipped migration would result into a valid dependency

It also seems we have an API to indicate if a migration is COMPLETE so perhaps we could use that:
MigrationInterface::RESULT_COMPLETED

wrong issue

can confirm #13 worked for me.

Thankyou, #7 JulianD,

In my case I had a migration that worked fine via the ui but failed when executing via cron. Changing from required to optional seemed to rectify this.

I wish I knew where this was documented.

The patch from #13 is now unnecessary. We can use the --force option with drush mim.

Oops

I'm having the same issue with some migrations from CSV.

I think the issue is that we check if all rows were processed.

There are numerous scenarios where migrations will never have all rows processed. In my case, the migration process actually remove duplicates from the CSV source. Therefore there will always be more rows than actual migrated content.

Is there any alternative/reliable way of checking if a migration have been processed successfully?

I have had the same issue. And the cause was similar to the previous comment. In my case, in depended migration, a custom source plugin produced duplicate items, that were skipped while processing. The problem was fixed in custom source plugin by removing duplicates.

The error message was misleading.

I met the same issue again, but the cause is more clear now. It can be reproduced by using the source like below, in dependent migration:

...
source:
  plugin: embedded_data
  data_rows:
    -   id: 1
        name: 'One'
    -   id: 1
        name: 'Two'
    -   id: 3
        name: 'Three'
  ids:
    id:
      type: integer
...

Note, two first records have the same id, so there are three records but were imported only two.

Here is the patch, now the error message should be more clear.

Simplified my previous patch.

Fix for the testing fail.

Needs issue summary update. It isn't clear if the latest patch, which adds text to the exception message is supported. Is this a bug? Or is documentation needed to explain how rows are counted and processed.

Rerolled patch to 9.1.x.

-    $this->assertEqual($this->migrateMessages['error'], [new FormattableMarkup('Migration @id did not meet the requirements. Missing migrations d6_aggregator_feed. requirements: d6_aggregator_feed.', ['@id' => $migration->id()])]);
+    $this->assertEqual($this->migrateMessages['error'], [new FormattableMarkup('Migration @id did not meet the requirements. Missing migration(s) or not all rows were processed in dependent migration(s) d6_aggregator_feed. requirements: d6_aggregator_feed.', ['@id' => $migration->id()])]);

Why aren't the requirements put in parentheses at the end of the first sentence?

The second sentence isn't a sentence at all.
Plus, since the list of depended migrations is hard-coded and it contains just an item, there isn't the need of using in dependent migration(s) d6_aggregator_feed. in the dependent migration d6_aggregator_feed is correct.

working on it.

Hi @kiamlaluno,

Just updated #35, Kindly check.

Needs work for #32.

The patch in #13 has the following change which was removed from #28 without comment. Anyone know why?

+++ b/core/modules/migrate/src/Plugin/Migration.php
@@ -453,7 +453,7 @@ public function checkRequirements() {
-      if (!$required_migration->allRowsProcessed()) {
+      if ($required_migration->getIdMap()->processedCount() == 0) {

Confirming that this message is very confusing.

+++ b/core/modules/migrate/src/Plugin/Migration.php
@@ -444,7 +444,7 @@ public function checkRequirements() {
-      throw new RequirementsException('Missing migrations ' . implode(', ', $missing_migrations) . '.', ['requirements' => $missing_migrations]);
+      throw new RequirementsException('Missing migration(s) or not all rows were processed in dependent migration(s) ' . implode(', ', $missing_migrations) . '.', ['requirements' => $missing_migrations]);

+1 for the improved message.

Finally, one of the possible ways this problem can occur is if a migration that other migrations depend upon crashes mid-way (i.e. with a fatal PHP error). It then remains in the "importing" state, and so it gets automatically skipped when triggering again. Then the depending migrations get run and each of them throws this error.
For this particular scenario, #3167267: MigrateExecutable should catch not only exceptions, but also fatal errors should be very helpful.

Not migrating all the rows is an intentional thing. The reason I have returned FALSE in prepareRow() is because that row should not be imported. But this makes all subsequent migrations fail. I feel like this behaviour doesn't make sense. Skipping a row is not a failure of the migration.

I guess the idea is that a row that isn't processed means a migration plugin is missing. Probably, migration plugins should be allowed to skip a row, and make clear that was intentional.

I think a possible work around is setting a dummy source property in prepareRow and then using that property to skip in the processes. that should allow you to essentially skip a row in prepareRow without making subsequent migrations fail.

Jonas, do you mean use the skip_on_empty process plugin, with a source property that you set (or don't) in the prepareRow()?

Yes. Or skip_on_value etc if using the migrate_plus module.

patch does not apply any more

rerolled the patch

Adding credit from duplicates

@Meenakshi.g, when you reroll a patch and it is passing tests remember to remove the 'Needs reroll' tag. Thx.

This situation is caused when the prepareRow() method in a source plugin returns FALSE. When that happens the row is skipped but not recorded in the map row and thus the count of rows processed will be less that the count returned by the count() method of the source plugin. And when that happens you get 'migrations fail due to missing dependency ... '. This is distinctly different from the behavior when a row is skipped in a process plugin. In that case a MigrateSkipRowException is thrown and the row is recorded in the map.

joachim discovered this in #3048459-#9 and suggests that "The fix here is that prepareRow() should behave the same way as the catching of exceptions from processRow(): mark the row in the map as ignored." This seems reasonable but unfortunately it means that the source (extract) is preforming a process (transform) and that should not happen.

That made me wonder why prepareRow() behaves this way. I found the answer in #2522012: Remove broken idlist handling, replace with more robust exception handling where the idlist feature was removed. In comment #45 mikeryan explains "the main thing that I think is missing is a way to skip a row without recording it in the ID map - right now prepareRow(), when receiving a FALSE return from a hook, unconditionally records it as STATUS_IGNORED in the map table, which isn't appropriate in the idlist case. My thinking is to add a boolean to MigrateSkipRowException to indicate whether this particular row skippage should be recorded as "ignored", or the skip should simply be done silently." And skipping silently is what happens when FALSE is returned from prepareRow(). This is clearly by design.

Now, whether custom/contrib use that feature to figure out what rows to skip when '--idlist' is used, I don't know. And I can't test it because currently idlist feature is broken for me (https://github.com/drush-ops/drush/issues/4679).

Even so, the safest path is to not change the behavior of prepareRow() but improve the documentation. joachim opened #3196303: SourcePluginBase::prepareRow() should document that skipping a row here does not store it in the map for that but I've closed it in favor of this issue and, to keep this discussion in one issue. I do agree that documentation on at minimum prepareRow needs to be improved.

I have not reviewed the changes exception messages in the latest patch.

> joachim discovered this in #3048459-#9 and suggests that "The fix here is that prepareRow() should behave the same way as the catching of exceptions from processRow(): mark the row in the map as ignored." This seems reasonable but unfortunately it means that the source (extract) is preforming a process (transform) and that should not happen.

I don't really understand how the source affecting the map means it's performing a process.

But that architectural problem already exists:

1. If one of the hooks invoked in SourcePluginBase::prepareRow() throws a MigrateSkipRowException, then the row is saved to the map.
2. If one of the process plugins throws a MigrateSkipRowException, then the row is saved to the map
3. If SourcePluginBase::prepareRow() returns FALSE to skip a row, then the row is NOT saved to the map, and we get this bug.

1 is not compatible with the architectural principle of the source affecting the map.
1 and 3 are inconsistent.
Changing either 1 or 3 is an API change.

I suggested a workaround at https://www.drupal.org/project/drupal/issues/3048459#comment-13140656:

As a workaround, you can do the logic for skipping the row in SourcePluginBase::prepareRow(), then set a boolean for whether to skip the row to an arbitrary field name:

      $row->setSourceProperty('skip', $whether_to_skip);

and then use the skip_on_empty process plugin with an empty destination in the migration's config:

  null:
    plugin: skip_on_empty
    method: row
    source: skip
    message: 'Skipping!'

@quietone wrote about this:

> The 'workaround' suggested in #4 is not merely a workaround, it is, in my opinion, the correct way to skip rows.

If that is the case, then SourcePluginBase::prepareRow() returning FALSE to skip a row should be deprecated.

Migrating here from #3048459

Here's a concrete example - I think it highlights the issues @joachim is trying to raise.

I've got an issue where I'm trying to import a multi-lingual taxonomy from a larger table. I need to import the taxonomy first in both languages, then connect my parent entity to it. Ideally the taxonomy would be in it's own source data, but the source data is coming from a 3rd party and apart from pre-processing the data, I don't have an option to import from a "clean" table.

Consider the following:

ID,Product Name,Color_EN,Color_FR
1,Toyota Roadster,Red,Rouge
2,Toyota Lexus,Red,Rouge
3,Ford F150,Black,Noir

I'm running a taxonomy import against the Color_EN field. That works fine, but it skips a row because "Red" is there twice.

I'm then trying to import a translation Color_FR, tied to the same tids as the previous migration, using Color_EN as a key.

If I do it without dependencies, it works great: I can import color_en, then color_fr, and I get two translated taxonomy terms.

If I add dependencies or try to build a group, I get that rows were skipped on the Color_EN import and the FR import will fail (even though the keys are all there).

Is there a way around this issue?

EDIT: After a quick check-in with my team, we've decided to work around this problem by making two migration groups - one to import the English content, and a second migration group (not dependent on the first migration) to import the French translations.
Two migration import commands should be better than 20.

Still, the core issue remains. There are reasons why a dependency might skip rows but still be "complete".

EDIT2: The example above would even fail without the French translation - if you used the single source data to import taxonomies, importing your nodes afterward would break because your color taxonomy skipped rows.

I do think I understand your points. I am just being cautious.

I have written a test, which does not include the changes to the exception messages in earlier patches in this issue. The goal is to check that allRowsProcessed is TRUE for each way of skipping a row. A row is skipped in a process plugin, hook_prepare_row and prepareRow. It is only when prepareRow() returns FALSE that the migration counts differ and the allRowsProcessed is FALSE, which will prevent a dependent migration from running. Is the test sufficient?

Remove checking of the return value of prepareRow.

Just a few nitpicks.

Nice test! I think it's really clever, but overall I think it could do with some comments that explain the strategy.

1. +++ b/core/modules/migrate/tests/modules/migrate_count_test/migrate_count_test.module
   @@ -0,0 +1,25 @@
   +  if ($data == 'prepare_row_return_false') {
   

   Nitpick: mention in these strings it's the hook.

2. +++ b/core/modules/migrate/tests/modules/migrate_count_test/src/Plugin/migrate/source/CountTest.php
   @@ -0,0 +1,27 @@
   +    if ($row->get('data') === 'false') {
   

   Nitpick: I'd make this a phrase like 'source_prepare_row_returns_false' for clarity. The first time I read this line I got confused and started thinking of 'false' in YAML :/

3. +++ b/core/modules/migrate/tests/src/Kernel/MigrateCountTest.php
   @@ -0,0 +1,149 @@
   + * Tests when migrate messages are deleted.
   

   I don't think that's the right description?

4. +++ b/core/modules/migrate/tests/src/Kernel/MigrateCountTest.php
   @@ -0,0 +1,149 @@
   +   * Tests map message deletion.
   +   *
   +   * @dataProvider providerTestNext().
   +   */
   +  public function testNext($row_1, $expected) {
   

   Also looks like copypasta docs maybe? :)

5. +++ b/core/modules/migrate/tests/src/Kernel/MigrateCountTest.php
   @@ -0,0 +1,149 @@
   +    $this->definition['source']['data_rows'][1] = $row_1;
   

   I'm a bit confused about how here we add source rows from the test data provider, but the $definition created in setUp() already has a row. Does it need that row? It's confusing having row data come from two places.

   *reads some more*

   Ah right, there's one row in there to start with so the migration has one successful row first, so you can see that in the count and see it basically works?

   It would be good to explain that in comments.

6. +++ b/core/modules/migrate/tests/src/Kernel/MigrateCountTest.php
   @@ -0,0 +1,149 @@
   +    // Create the migration an execute.
   

   Typo: 'an'.

Thanks for your work everyone! We are currently facing issues with patch from #61.

We are currently using Drupal 9.2 and would like to migrate a Drupal 6 and 7 site to Drupal 9. Furthermore, we are skipping some specific migrations with the help of the "hook_migrate_prepare_row()". Since we installed the patch, some migrations throw interesting error messages and certain migrations show up as "failed".

Without the patch we are only getting our message "Skip Migration: widget_label because we don't want to migrate English fields" through the MigrateSkipRowException().
With the patch installed, we are getting our message + the message "Array index missing, extraction failed for 'array( '0', )'. Consider adding a default key to the configuration." for the same id.

Our dumbed down Skipping logic:

switch ($migration->id()) {
case 'upgrade_d6_field_instance_label_description_translation':
      $language = $row->getSourceProperty('language');
      if ($language === 'en') {
        throw new MigrateSkipRowException('Skip Migration: ' . $row->getSourceProperty('property') . ' because we dont want to migrate English fields', TRUE);
      }
      break;
}

I'm not sure what causes this, but we are experiencing the same bug for the d6 and d7 Migration.

EDIT: Might be related #3148959: Improve migrate messages from the extract plugin

Some more Screenshots to show the problem described in #65:

Proper skip message without the patch:

Skip message and error message with the patch:

From #2797505-57:

[ ... ]

But that architectural problem already exists:

1. If one of the hooks invoked in SourcePluginBase::prepareRow() throws a MigrateSkipRowException, then the row is saved to the map.
2. If one of the process plugins throws a MigrateSkipRowException, then the row is saved to the map
3. If SourcePluginBase::prepareRow() returns FALSE to skip a row, then the row is NOT saved to the map, and we get this bug.

1 is not compatible with the architectural principle of the source affecting the map.
1 and 3 are inconsistent.
Changing either 1 or 3 is an API change.

[ ... ]

From my point of view, I do not think that the behavior mentioned in point 1 is inconsistent. If the source plugin brings a row, the migration must have to do something with it at the process stage. A row migration might success of fail for some reason. Skipping it by the source plugin actually is a reason of failure, at an early stage of the processing: the row preparation.

Me too see here a clear bug as mentioned in point 3. With this patch, rows skipped by SourcePluginBase::prepareRow() are treated as skipped by process plugins. I also have tested it with drush mim --idlist argument with no unexpected behavior.

It implies an API change that might been avoided by enabling the new behavior by configuration o something similar.

Let's have some feedback before working more on it and adding tests.

@manuel.adan: Thanks, could you please describe the changes in #70 as the patch is quite different from #61 and there's no interdiff. We just ran into this issue again, so this is still existing and breaks functionality.

I have reviewed patches in commets #61 and #70.

If I understand things properly, #61 would be an API break. Per \Drupal\migrate\Plugin\MigrateSourceInterface::prepareRow, we allow returning FALSE to signal the row should be skipped. I tested this on a recent project. The error about skipped rows in dependent disappears, but that is because the rows where imported when they should have not. I do not have a Core example handy, but the ParagraphsItem source plugin relies on skipping on FALSE to intentionally prevent migrating paragraph entries that are no longer in use.

Patch #70 would mark the row as skipped effectively solving the issue. In this patch, this is a byproduct of calling throw new MigrateSkipRowException('Row skipped by source plugin.');. That is, MigrateSkipRowException signals that the row should be saved to the map. The actual saving is performed by MigrateExecutable in the import method. Namely, in this block of code:

        try {
          foreach ($pipeline as $destination_property_name => $plugins) {
            $this->processPipeline($row, $destination_property_name, $plugins, NULL);
          }
          $save = TRUE;
        }
        catch (MigrateException $e) {
          ...
        }
        catch (MigrateSkipRowException $e) {
          if ($e->getSaveToMap()) {
            $id_map->saveIdMapping($row, [], MigrateIdMapInterface::STATUS_IGNORED);
          }
          ...
        }

Throwing the exception probably suffices. I do not understand why the proposed changes to \Drupal\migrate\MigrateExecutable::processPipeline and \Drupal\migrate\Row are necessary. I propose a slightly different approach. Save to the map directly in \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::next Optionally, we can also include a message there. I have attached a patch with both, but only is saving to the map is strictly necessary.

This topic was discussed in this Slack thread some time ago. For references, copying part of the conversation below:

So, prepareRow row in my custom source plugin returned FALSE and the records was skipped. But, it failed to do something that \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::prepareRow does. In the method implementation for SourcePluginBase , which is where the migrate_prepare_row and migrate_MIGRATION_ID_prepare_row hooks are fired. If a record is marked to be skipped, it is saved to the idMap.

While there is a boolean to determine if the record should be saved or not ($save_to_map), saving to the map makes sure the record is accounted for when the migrate API checks if all records have been processed for a migration. This check is also performed in \Drupal\migrate\Plugin\Migration::checkRequirements So, if a migration dependency is set as required, it has to have all rows processed. A tangential note, but worth mentioning.

So, the solution was manually saving to the idMap in my custom source process' prepareRow method. This fixed the problem for me.

Writing to the idMap is accounted for by \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::prepareRow when the result comes from an implementation of the migrate_prepare_row and migrate_MIGRATION_ID_prepare_row hooks. But if you implement the method in your source plugin, you need to write to the map yourself.

The original issue asks for improving the documentation \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::prepareRow If we make this change in \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::next we keep BC. If we only improve the API, we would have to rely on developers to manually save to the map if they implement \Drupal\migrate\Plugin\MigrateSourceInterface::prepareRow in their custom source plugins.

I have updated the issue summary. Also changing state to Needs review for feedback on the proposed patch. No interdiff against previous patches as the approach is somewhat different.

This is affecting #3268555: Paragraph Migrations broken by archived flag

#74 helps a little but it's not enough.
For some unknown reason, i can't have all of my items.

So i found another workaround.
Put migrations in the same group, add an index before id (don't forget to rename tables to keep migration track) and launch import on group.

Btw, #70 have a good point.
By adding some intels to row, we may add reason for ignore.
Because #74 does not do that and it's pretty annoying.

Just ran into this once again. After trying to migrate a site using media_migration enabled and a dirty workaround to get it to work with D11 (#3494209: Drupal 7 to Drupal 11 migration runs forever, #14), I tried the latest 3 patches provided here, but nothing seemed to work.

I ended up simply manually removing the failing dependencies from the migration ymls. Luckily, they still migrated in the correct order, and the files migrated to media as expected. I think this is how I fixed the issue last time I ran into this.

I hope this might help someone.

In general, I like the approach discussed in #74. It might not solve all the problems, but it does solve a single problem and should help with some people's issues. Let's roll it into an MR and add tests.

Patch in #74 to MR: https://git.drupalcode.org/project/drupal/-/merge_requests/10925/diffs

MR!10925 Has PHPUnit test failures that seem related.

See: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/migra...
See: https://git.drupalcode.org/issue/drupal-2797505/-/jobs/4043797

---- Drupal\Tests\migrate\Kernel\MigrateSkipRowTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      phpunit-261.xml      0 Drupal\Tests\migrate\Kernel\Migrate
    PHPUnit Test failed to complete; Error: PHPUnit 10.5.38 by Sebastian Bergmann and contributors.
    
    Runtime:       PHP 8.3.15
    Configuration: /builds/issue/drupal-2797505/core/phpunit.xml.dist
    
    F                                                                   1 / 1 (100%)
    
    Time: 00:01.316, Memory: 8.00 MB
    
    Migrate Skip Row (Drupal\Tests\migrate\Kernel\MigrateSkipRow)
     ✘ Prepare row skip
       ┐
       ├ Failed asserting that an array is empty.
       │
       │ /builds/issue/drupal-2797505/core/modules/migrate/tests/src/Kernel/MigrateSkipRowTest.php:63
       ┴
    
    FAILURES!
    Tests: 1, Assertions: 3, Failures: 1.

And functional tests
See: https://git.drupalcode.org/issue/drupal-2797505/-/jobs/4043788
See: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/migra...
See: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/migra...

---- Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6Test ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      phpunit-3.xml        0 Drupal\Tests\migrate_drupal_ui\Func
    PHPUnit Test failed to complete; Error: PHPUnit 10.5.38 by Sebastian Bergmann and contributors.
    
    Runtime:       PHP 8.3.15
    Configuration: /builds/issue/drupal-2797505/core/phpunit.xml.dist
    
    F                                                                   1 / 1 (100%)
    
    
    
    Time: 02:35.228, Memory: 59.34 MB
    
    Upgrade6 (Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6)
     ✘ Upgrade and incremental
       ┐
       ├ Failed asserting that 56 is identical to 39.
       │
       │ /builds/issue/drupal-2797505/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php:157
       │ /builds/issue/drupal-2797505/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php:211
       ┴
    
    FAILURES!
    Tests: 1, Assertions: 4223, Failures: 1.

#74 worked for me. It allowed for archived paragraphs to be ignored and the migration to continue.

So the MR (#74 approach) works and solve this kind of issues.

However, I did pushed some updates for fixing the tests ... And those updates indicates the solution do not seem right or complete, at least.
interfering with exception MigrateSkipRowException->saveToMap

Then I reverted those changes and "extended" SourcePluginBase with a new property to be aware of saveToMap and to have control for "non-saving cases".

Now we should add proper tests for this issue/solution cases.
Of course if this trick would be considered "OK".

Temporary on Needs Review

it seems there were some tests in an issue patch by @quietone (#59)

And I applied (partially) and cleaned up ... the hook removed as it has already tests + I don't think the hooks should return "values".

However, now it seems the MR has a failure which I don't think it's related.
And I can't replicate locally.

@vasike, some tests fail randomly, so it's common practice to manually re-run those that fail. Most of the time, they pass on the second attempt.

In this case, the test kept failing, and I remembered having the same error with a CSS stylesheet larger than 90kB in another issue. I think I rebased to make it work, so I rebased the issue fork branch onto the latest 11.x commit. Now it passes 🙂 (I've had to re-run Nightwatch tests, which have failed on the first attempt)

Left some comments on the MR.

MR threads addressed ... so back to review.

Works for us! We were facing https://www.drupal.org/project/paragraphs/issues/3268555 and the current MR in this thread fixes the issue on Drupal 11.1.7.

Thanks!

I think we have a few things to address.

applied all @alexpott's suggestions, as "they make sense" (at least and thanks) ... back to review

There are still a few comments on the MR without any feedback. Also, for those facing this and don't want to apply a patch, you can always move dependencies from required to optional. I almost never use required because of reasons like what this issue causes.

@heddn:

I added that work-around to the issue summary. (Back when the issue-summary template included comments, it suggested adding work-arounds. I think it was under the "Proposed resolution" section.)

Any migration that is referenced by the migration_lookup or sub_process plugin is automatically added as a dependency. (See Drupal\migrate\Plugin\Migration::getMigrationDependencies().) I just checked: they are added as optional dependencies. So that should not trigger the problem in this issue.

@benjfisher came to review this but not sure I follow. You added a workaround so should this still be reviewed?

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.