Dispatch events at key points during migration

Add events to ID map save and delete operations - immediately, this allows us to remove statistics tracking entirely from core and let the contrib tools have complete control. #2443081: Make MigrateExecutable statistics publicly available

Nice!

1. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -139,8 +150,8 @@ class Sql extends PluginBase implements MigrateIdMapInterface {
   +    $this->eventDispatcher = $migration->getEventDispatcher();
   

   Conceptually I think this is wrong. THe event dispatcher should be part of the migration executable not of the migration entity itself.

2. +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapEnsureTablesTest.php
   @@ -194,6 +194,10 @@ protected function runEnsureTablesTest($schema) {
   +      ->will($this->returnValue($event_dispatcher));
   
   +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
   @@ -102,7 +102,11 @@ protected function getIdMap() {
   +      ->will($this->returnValue($event_dispatcher));
   

   You can use ->willReturnValue() instead

1. I agree with @dawehner that the event dispatcher does not belong on the Migration entity. Given the way it's used here, I don't see any reason why it should be. The Sql ID map plugin should take the event dispatcher from the container via ContainerFactoryPluginInterface.
2. This is not related, but I noticed it -- why does SqlBase have a $migration property? It already inherits one from SourcePluginBase.
3. In MigrateExecutable, calls to dispatch() should be all on one line for consistency.
4. Also in MigrateExecutable, why bother with having the event dispatcher in the migration? IMO, getEventDispatcher() should be moved wholesale into MigrateExecutable.
5. Can all the event types (and MigrateEvents) be in the Drupal\migrate\Event namespace?
6. MigratePostImportEvent and MigratePreImportEvent are identical. Can they be collapsed into a single class (MigrateImportEvent)?
7. MigratePreSaveEvent and MigratePostSaveEvent are confusingly named. They're row-level events. Can they be called MigratePreRowSaveEvent and MigratePostRowSaveEvent? They're also mostly identical; MigratePostSaveEvent could extend MigratePreSaveEvent.
8. In MigrateSqlIdMapTest::getIdMap():
   

   $migration->expects($this->any())
     ->method('getEventDispatcher')
     ->will($this->returnValue($event_dispatcher));
     

   This can be simplified to $migration->method('getEventDispatcher')->willReturn($event_dispatcher).

9. Ditto in MigrateTestCase::getMigration().

I had a quick look at the patch and I like it, in general i'm +1 for this. Will try do a more thorough review this week

1. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -174,44 +104,32 @@ class MigrateExecutable implements MigrateExecutableInterface {
   +  public function __construct(MigrationInterface $migration, MigrateMessageInterface $message, EventDispatcherInterface $event_dispatcher = NULL) {
   

   Why is the event dispatcher optional in the constructor? Is this being instantiated in such a way that we don't have access to the container outside of this class? Or is this a BC effort of some sort?

2. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -174,44 +104,32 @@ class MigrateExecutable implements MigrateExecutableInterface {
   +    if (!$event_dispatcher) {
   +      $event_dispatcher = \Drupal::service('event_dispatcher');
   

   Instead of doing this in the constructor, it seems better to do it in a corresponding getter if this needs doing at all. Logic in the constructor should be avoided if possible, and the rest of core does optional calls to \Drupal::service() in getters. Even if it's protected, I think that'll be a "better" approach.

All in all this looks pretty good, but it's hard to mentally divide the two competing patches here. I'd suggest one patch with the event stuff and a follow up to remove the various count/limit/etc stuff in this patch. I think that will make both easier to get in.

Eclipse

According to mikeryan, this blocks migrate Drush commands (e.g. the ability to do partial imports) from being completed. Escalating.

I have nitpicks, but they're just coding style and terminology-related. Beyond these, the patch looks fine to me.

1. +++ b/core/modules/migrate/src/Event/MigrateImportEvent.php
   +  /**
   +   * Migration object.
   +   *
   +   * @var \Drupal\migrate\Entity\MigrationInterface
   +   */
   +  protected $migration;
       

   Can this say entity, not object?

2. +++ b/core/modules/migrate/src/Event/MigrateImportEvent.php
   +  /**
   +   * Constructs an import event object.
   +   *
   +   * @param \Drupal\migrate\Entity\MigrationInterface $migration
   +   *  Migration object.
   +   */
   +  public function __construct(MigrationInterface $migration) {
   +    $this->migration = $migration;
   +  }
       

   $migration's description should be indented another space and be entity, not object.

3. +++ b/core/modules/migrate/src/Event/MigrateImportEvent.php
   +  /**
   +   * Gets migration object.
   +   *
   +   * @return \Drupal\migrate\Entity\MigrationInterface
   +   *   The migration object involved.
   +   */
   +  public function getMigration() {
   +    return $this->migration;
   +  }
       

   s/object/entity, everywhere it occurs.

4. +++ b/core/modules/migrate/src/Event/MigrateMapDeleteEvent.php
   +  /**
   +   * Map object.
   +   *
   +   * @var \Drupal\migrate\Plugin\MigrateIdMapInterface
   +   */
   +  protected $map;
       

   s/object/plugin

5. +  /**
   +   * Constructs a migration map delete event object.
   +   *
   +   * @param \Drupal\migrate\Plugin\MigrateIdMapInterface $map
   +   *  Map object.
   +   * @param array $source_id
   +   *  Array of source ID fields representing the object being deleted from the map.
   +   */
   +  public function __construct(MigrateIdMapInterface $map, array $source_id) {
   +    $this->map = $map;
   +    $this->sourceId = $source_id;
   +  }
       

   Parameter descriptions should be indented an extra space, and $map's should say 'plugin', not 'object'.

Obviously the indentation and terminology fixes should be applied to all of the event classes, not just the ones listed here. But you get the idea. :)

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -234,9 +246,23 @@ protected function getSource() {
+  protected function getEventDispatcher() {
+    if (!$this->eventDispatcher) {
+      $this->eventDispatcher = \Drupal::service('event_dispatcher');
+    }
+    return $this->eventDispatcher;
+  }

Sorry if I was unclear before. This looks great, and the NULL eventDispatcher in the __construct() in the previous patch was also great. So constructor injection is still totally ++, but if nothing was injected, this protected getter is also ++. Just don't put this logic in the __construct() method.

Hopefully that makes sense. I've not dug through the tests in depth, but the code changes are looking pretty good here. I'll try to give the tests a once over later today or tomorrow.

Patch looks good, nice work! Few small nitpicks and then +1 for RTBC.

1. +++ b/core/modules/migrate/src/Event/MigrateEvents.php
   @@ -0,0 +1,111 @@
   +   * listener method receives a \Drupal\migrate\MigrateMapDeleteEvent instance.
   

   Should be \Drupal\migrate\Event\MigrateMapDeleteEvent

2. +++ b/core/modules/migrate/src/Event/MigrateMapSaveEvent.php
   @@ -0,0 +1,65 @@
   +   * Gets map plugin.
   

   Gets the map plugin.

3. +++ b/core/modules/migrate/src/Event/MigratePreRowSaveEvent.php
   @@ -0,0 +1,64 @@
   +   * Gets migration entity.
   

   Gets the...

4. +++ b/core/modules/migrate/src/Event/MigratePreRowSaveEvent.php
   @@ -0,0 +1,64 @@
   +   * Gets row object.
   

   Gets the row object.

5. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -234,9 +249,23 @@ protected function getSource() {
   +   * Returns the event dispatcher.
   

   Gets the event dispatcher

6. +++ b/core/modules/migrate/tests/modules/migrate_events_test/src/Plugin/migrate/source/DataSource.php
   @@ -0,0 +1,56 @@
   +class DataSource extends SourcePluginBase {
   

   Shame we need this class and could we have re-used EmptySource?

EmptySource does have a row? It's just the id that is empty?

Other than that +1 for RTBC

Works for me.

Wow, great CR!!

Went through this patch with @mikeryan and @phenaproxima earlier today. The patch is very well-documented and restores functionality found in Migrate module in D7. Without this, modules such as Migrate Plus can't port.

I inquired about "why events vs. hooks?" and the answer was basically because we're trying to build this in a future-proof way. Since we're writing this in 2015 and not in 2012 and earlier, like most of D8's code, this makes sense to me. I know there have been some concerns about using events in places, but I think it was more when it created a mish-mash of approaches. This is consistent and well-documented.

Committed and pushed to 8.0.x. Thanks!

CR published, too.