Add message service to migrate events

It's likely many migrate events are going to need to be able to log. I wonder if we should just add a logMessage() method to a base migrate event class rather than exposing the service?

I added a common event base class for all migrate events and put the message interface on that directly. I also added a logMessage() method on this base class as described in #5. I lost the interdiff I created but it was around the size of the patch itself, so, I hope it's okay.

1. +++ b/core/modules/migrate/src/Event.php
   @@ -0,0 +1,52 @@
   +namespace Drupal\migrate;
   

   Should be in the Drupal\migrate\Event namespace.

2. +++ b/core/modules/migrate/src/Event.php
   @@ -0,0 +1,52 @@
   +class Event extends SymfonyEvent {
   

   I think it would be clearer to name this MigrationEvent.

3. +++ b/core/modules/migrate/src/Event.php
   @@ -0,0 +1,52 @@
   +  public function logMessage($message, $type = 'status') {
   

   I'm not sure it's really worthwhile to have a convenience function for this - $event->logMessage() isn't exactly a huge improvement over $event->getMessageService()->display(). And I think having a different function name (logMessage vs. display) obscures what it's doing - a migration developer familiar with the API will immediately recognize what display() is doing, and will hesitate over using a different name to do the exactly same thing just because the context is an event listener.

   Besides which, we're not logging here (which implies persisting the message somewhere such as the message table), we are displaying the message in a front end.

4. +++ b/core/modules/migrate/src/Event/MigrateImportEvent.php
   @@ -27,9 +28,12 @@ class MigrateImportEvent extends Event {
   +  public function __construct(MigrationInterface $migration, MigrateMessageInterface $message) {
   

   Best practice is to extend the parent class signature (the arguments being passed through should be in the order of the base constructor). Applies to all the events.

5. +++ b/core/modules/migrate/src/Event/MigrateImportEvent.php
   @@ -27,9 +28,12 @@ class MigrateImportEvent extends Event {
   +    $this->message = $message;
   

   Should call the parent constructor. Applies to all the events.

6. +++ b/core/modules/migrate/src/Event/MigrateMapSaveEvent.php
   @@ -7,8 +7,8 @@
   +use Drupal\migrate\Event;
   ...
   -use Symfony\Component\EventDispatcher\Event;
   

   The map events are quietly changed to use the new base class, but the message service is not being passed, so a consumer of the event trying to use the message service will blow up. The problem, of course, is these events are dispatched from the Sql id map plugin, which does not have access to the MigrateExecutable or its message service. So, if the map events are to use the message service, it will need to be passed to ID map plugins. If not, then they should continue to extend the Symfony event.

#7:

1. Actually, it should be in Drupal\migrate namespace itself so that FQCN is \Drupal\migrate\Event.
2. I am mixed on that. I think there is merit in it being \Drupal\migrate\Event rather than \Drupal\migrate\MigrateEvent. Also, consider that this base class would not be used in itself, only extended (we could mark it abstract).
3. I agree with you here. I just added it to see if it was worthwhile. It doesn't look like it.
4. and 5. Agreed.
6. I couldn't find where those events were being invoked. I will look for them again to make sure.

@mikeryan: I'd like to hear your thoughts on the above, especially points 1 and 2.

$event->logMessage() isn't exactly a huge improvement over $event->getMessageService()->display()

The point is that the consuming code is decoupled from the message service.

a migration developer familiar with the API will immediately recognize what display() is doing

I actually think we should rename to log() or logMessage() everywhere, they much better describe what we're doing than display(). Given the message service is swappable, it could be logged to syslog, or it could be logged to the screen. display() only works in the latter.

+++ b/core/modules/migrate/src/Event/MigratePreRowSaveEvent.php
@@ -33,10 +33,14 @@ class MigratePreRowSaveEvent extends Event {
+  public function __construct(MigrateMessageInterface $message, MigrationInterface $migration, Row $row) {

As I mentioned on the other issue, i'd rather the order of the params were the same as MigrateExecutable

Yes, i think a logMessage() method on the event and then a follow-up to rename that it the message system.

Shame about the constructor args, IMO the base class was wrong, we had MigrateExecutable as precedent for the order of those two args, and consistency is important for me. I'd vote for re-ordering those params in the base event class as well to match up with MigrateExecutable.

This is blocking #2545632: [PP1] Move memory reclamation out of migrate executable so tagging. However, that is also a normal task clean-up issue so I don't think we need any priority changes here.

+++ b/core/modules/migrate/tests/src/Unit/Event/MigratePostRowSaveEventTest.php
@@ -0,0 +1,76 @@
+    $event = new MigratePostRowSaveEvent($message_service, $migration, $row, [1,2,3]);

Shouldn't [1,2,3] be [1, 2, 3]? This appears twice in each Unit test.

The only comment that I don't see agreement on is mikeryan's second point in comment #7 about

I think it would be clearer to name this MigrationEvent.

I noticed the rollback events are not tested. Does that need to be done?

+++ b/core/modules/migrate/src/Event/EventBase.php
@@ -0,0 +1,65 @@
+class EventBase extends SymfonyEvent {

This this supposed to be an abstract base class?

+++ b/core/modules/migrate/src/Event/EventBase.php
@@ -0,0 +1,65 @@
+use Symfony\Component\EventDispatcher\Event as SymfonyEvent;
...
+class EventBase extends SymfonyEvent {

Why do we need the as SymfonyEvent here? and also I think @dawehners feedback is valid in #24 and then this looks good to me.

I think @neclimdul is right, I was getting confused with the abstract final class that provides the event names, not the event class itself.

I could apply the patch, so no reroll is needed.

Reroll.

Not sure why a comment test is failing, and this passes locally so retesting.

Thats better. Back to needs review.

1. +++ b/core/modules/migrate/src/Event/EventBase.php
   @@ -0,0 +1,60 @@
   +   * Migration entity.
   

   Migrations are no longer entities. :) This should also be fixed in the other events while we're at it.

2. +++ b/core/modules/migrate/src/Event/EventBase.php
   @@ -0,0 +1,60 @@
   +   *   Migration entity.
   

   Ditto.

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

   Ditto.

4. +++ b/core/modules/migrate/src/Event/EventBase.php
   @@ -0,0 +1,60 @@
   +   * @param string $type
   

   Should we perhaps define constants in MigrateMessageInterface to set the levels?

5. +++ b/core/modules/migrate/src/Event/MigratePostRowSaveEvent.php
   @@ -11,17 +12,24 @@
   +   * @var array|bool
   

   Needs a description. And why would bool ever be allowed here?

6. +++ b/core/modules/migrate/tests/src/Unit/Event/EventBaseTest.php
   @@ -0,0 +1,44 @@
   +  public function testLogMessage() {
   

   This method has no assertions in it.

7. +++ b/core/modules/migrate/tests/src/Unit/Event/MigrateImportEventTest.php
   @@ -0,0 +1,43 @@
   +  public function testLogMessage() {
   

   Neither does this one.

Re #5, how about something like "The row's destination ID"?

And regarding #6 and #7, that's fine as long as there is a comment explaining that.

I have nits. After these are fixed, this is RTBC from me if it passes Drupal CI.

1. +++ b/core/modules/migrate/src/Event/EventBase.php
   @@ -0,0 +1,60 @@
   +   * Related migration.
   

   Can this just be "The migration"?

2. +++ b/core/modules/migrate/src/Event/EventBase.php
   @@ -0,0 +1,60 @@
   +   * @var \Drupal\migrate\Plugin\\MigrationInterface
   

   Nit: extra slash before MigrationInterface

3. +++ b/core/modules/migrate/src/Event/EventBase.php
   @@ -0,0 +1,60 @@
   +   * The current message service.
   

   Let's remove the word "current".

4. +++ b/core/modules/migrate/src/Event/EventBase.php
   @@ -0,0 +1,60 @@
   +  /**
   +   * Constructs a migrate event object.
   

   Nit: s/migrate/Migrate

5. +++ b/core/modules/migrate/src/Event/EventBase.php
   @@ -0,0 +1,60 @@
   +   *   The related migration being run.
   

   I don't think we should say "related", it has strange connotations.

6. +++ b/core/modules/migrate/src/Event/EventBase.php
   @@ -0,0 +1,60 @@
   +   *   The current migrate message service.
   

   s/migrate/Migrate

7. +++ b/core/modules/migrate/src/Event/MigratePostRowSaveEvent.php
   @@ -11,17 +12,26 @@
   +   * @var array|bool
   

   Remove |bool type hint.

8. +++ b/core/modules/migrate/src/Event/MigratePostRowSaveEvent.php
   @@ -11,17 +12,26 @@
       * @param array|bool $destination_id_values
   

   Let's remove |bool here, I don't think this value will ever be boolean.

9. +++ b/core/modules/migrate/src/Event/MigratePostRowSaveEvent.php
   @@ -11,17 +12,26 @@
   +  public function __construct(MigrationInterface $migration, MigrateMessageInterface $message, Row $row, $destination_id_values) {
   

   Can $destination_id_values be typehinted as an array?

I assume this will pass Drupal CI. Thanks for putting up with my persnicketiness. Happily RTBC!

Committed e5908d6 and pushed to 8.2.x. Thanks!

+++ b/core/modules/migrate/src/Event/EventBase.php
@@ -0,0 +1,60 @@
+  /**
+   * Displays a Migrate message.
+   *
+   * @param string $message
+   *   The message to display.
+   * @param string $type
+   *   The type of message, for example: status or warning.
+   */
+  public function logMessage($message, $type = 'status') {

+++ b/core/modules/migrate/tests/src/Unit/Event/EventBaseTest.php
@@ -0,0 +1,44 @@
+

Well the documentation still has the "display" thing... I think we should change "display" to "log" here. For the reasons given earlier.

diff --git a/core/modules/migrate/src/Event/EventBase.php b/core/modules/migrate/src/Event/EventBase.php
index 7f158d8..b228474 100644
--- a/core/modules/migrate/src/Event/EventBase.php
+++ b/core/modules/migrate/src/Event/EventBase.php
@@ -46,10 +46,10 @@ public function getMigration() {
   }
 
   /**
-   * Displays a Migrate message.
+   * Logs a message using the Migrate message service.
    *
    * @param string $message
-   *   The message to display.
+   *   The message to log.
    * @param string $type
    *   The type of message, for example: status or warning.
    */

Made these changes on commit because I don't think this is controversial at all.

diff --git a/core/modules/migrate/tests/src/Unit/Event/MigratePostRowSaveEventTest.php b/core/modules/migrate/tests/src/Unit/Event/MigratePostRowSaveEventTest.php
index 97d6b66..f42cb06 100644
--- a/core/modules/migrate/tests/src/Unit/Event/MigratePostRowSaveEventTest.php
+++ b/core/modules/migrate/tests/src/Unit/Event/MigratePostRowSaveEventTest.php
@@ -20,8 +20,8 @@ public function testGetDestinationIdValues() {
     $migration = $this->prophesize('\Drupal\migrate\Plugin\MigrationInterface')->reveal();
     $message_service = $this->prophesize('\Drupal\migrate\MigrateMessageInterface')->reveal();
     $row = $this->prophesize('\Drupal\migrate\Row')->reveal();
-    $event = new MigratePostRowSaveEvent($migration, $message_service, $row, [1,2,3]);
-    $this->assertSame([1,2,3], $event->getDestinationIdValues());
+    $event = new MigratePostRowSaveEvent($migration, $message_service, $row, [1, 2, 3]);
+    $this->assertSame([1, 2, 3], $event->getDestinationIdValues());
   }

   /**
@@ -34,7 +34,7 @@ public function testGetRow() {
     $migration = $this->prophesize('\Drupal\migrate\Plugin\MigrationInterface')->reveal();
     $message_service = $this->prophesize('\Drupal\migrate\MigrateMessageInterface');
     $row = $this->prophesize('\Drupal\migrate\Row')->reveal();
-    $event = new MigratePostRowSaveEvent($migration, $message_service->reveal(), $row, [1,2,3]);
+    $event = new MigratePostRowSaveEvent($migration, $message_service->reveal(), $row, [1, 2, 3]);
     $this->assertSame($row, $event->getRow());
   }

Coding standards fixed on commit too.