[PP-1] Dispatch new events while importing data

6 years later and a need for this came up again.

I think I have addressed most of the issues from #19. I added additional tests for everywhere the migration fails and ensure an event is dispatched. In addition, I passed the original exception to the event since in a few cases MigrateExecutable catches the generic Exception event, which means it will catch all exceptions giving classes further up the stack no chance to react to the event. In my case, a source plugin can throw a SuspendQueueException but this was being caught by MigrateExecutable and not the queue runners. So I needed to re-throw it in my event subscriber.

Removed the additional ideas from the issue summary as they are outside the scope of the core issue. I guess they can be opened in new issues if they're still wanted.

Left some feedback, mostly small stuff.

Will still need a change record.

Have added a change record and made some adjustments based on the comments and also made the getRow method consistent with regards to type hinting/annotations and the rest of the code. Will wait and see regarding the annotations issue.

smustgrave → changed the visibility of the branch 11.x to hidden.

smustgrave → changed the visibility of the branch drupal-2756269-10.1.x to hidden.

smustgrave → changed the visibility of the branch drupal-2756269-10.2.x to hidden.

Cleaning up patches and unused branches

Sorry this fell off my radar. Everything is looking really good but think the annotations now need to be replaced with attributes. Will keep an eye out for this one.

I've converted those annotations to php attributes. Unfortunately when I've merged in the latest code from 11.x there is a test failure in testRowSkippedEvent however the error message is obscured by the annoying "Exception: Serialization of 'Closure' is not allowed" message which doesn't actually show what's wrong. When I run MigrateEventsTest.php locally I don't get any errors so this one is hard to debug. Apparently this is quite a common issue faced in Migrate tests (see related issue).

I do have a stack trace:

    /builds/issue/drupal-2756269/core/lib/Drupal/Core/Cache/MemoryBackend.php:119
    /builds/issue/drupal-2756269/core/lib/Drupal/Core/Cache/CacheCollector.php:275
    /builds/issue/drupal-2756269/core/lib/Drupal/Core/State/State.php:102
    /builds/issue/drupal-2756269/core/modules/migrate/tests/src/Kernel/MigrateEventsTest.php:481
    /builds/issue/drupal-2756269/vendor/symfony/event-dispatcher/EventDispatcher.php:206
    /builds/issue/drupal-2756269/vendor/symfony/event-dispatcher/EventDispatcher.php:56
    /builds/issue/drupal-2756269/core/modules/migrate/src/MigrateExecutable.php:247
    /builds/issue/drupal-2756269/core/modules/migrate/tests/src/Kernel/MigrateEventsTest.php:354

That shows that it is trying to serialize the cached data (which would contain an exception). My guess would be that the exception data has a closure in there somewhere but like I said very hard to debug if I can't reproduce it locally.

Hopefully someone can help pinpoint the issue!

Still not sure why it only failed in ci only and not locally but changed the test to just need the exception name rather than the whole object since the serialization error was only happening in the test state recording. Tests passing now.

https://git.drupalcode.org/issue/drupal-2756269/-/jobs/1927487 shows the extensive test coverage

Believe all feedback from the rounds of reviews has been addressed

LGTM

Postponing this on 📌 Allow process plugins to flag a row to be skipped Needs review , as there are a number of things in here that don't make sense in the context of the changes we are trying to make to skipping rows, such as doing away with using Exceptions to do so.

Hi @mikelutz

For my personal use case I actually don't need to catch an exception when a row is skipped, I am more interested in when the migration fails. The only reason I have included it is because the original issue had it in scope. I would happily reduce the scope and remove changes to row skipping if that would help simplify things?

That's an option, but let's hold back. Even with reduced scope, this can't go in before 11.1, as we are already in RC for 11.0 and this is unlikely to be backported to the 11.0 series as it's a new feature and not a bug fix. If we can't make progress on the parent issue in the next couple of months we can revisit the reduced scope here to try to get it into 11.1, but in general, I like the scope and functionality, the timing is just bad with the other API changes we are working on.