Dispatch the PREPARE_ROW event. Deprecate prepare_row_alter hooks

This needs a CR. But I this is a drop-in copy/paste from contrib. Tests pass (and exist). But I think the only thing missing is a change record.

I've reviewed the CR. It looks good. My only question now after reviewing all this, is there any possibility to add a trigger_error deprecation warning when the hook is called? It looks like it is possible, so let's do that now too. And update the CR and link to it in the deprecation notice. https://www.drupal.org/core/deprecation#how-hook

Looks good now. Let's see if we can ship this with 8.5.1?

Events are good ☺

It looks like we'll need to add @legacy to the tests that are testing deprecations. And we probably should retain a test of the hooks, but convert all core usage to the events.

+1 for converting hooks to events wherever it makes sense.

I'm probably a dying breed of greybeard Drupal contributors/developers who generally find defining a single alter hook more intuitive than "define a custom class with a specially crafted annotation comment that implements interface X and extends class Y to implement method Z that listens to an event and then does something..."

-1.

I'm sure I'll be overruled with "But, Progress(tm)!". ;) *shrug*

Thanks for asking,
-Derek

Tagging

Also, to clarify: I *do* understand there are some cases where an event is better than an alter hook. But my understanding is those are where you have potentially lots of things trying to listen and react to the same event, and there are questions/problems about the order in which it happens.

How often are there going to be N different modules trying to alter the row during the prepare phase of a migration? In my experience, it seems like this a case where you have a single module, your custom migration module, trying to get things working for your special cases. I fail to see why all the DX overhead of having to listen to an event is worth it if there is likely to only ever be a single event listener.

And meta: wow, 23K of a patch to core that provides no new functionality and doesn't fix a bug. But is simply "Better"(tm). Because it results in something harder for developers to use but is the expected way to alter things in D8. The linked issue summary's "justification" for this change is:

hooks--
events++

Sorry, that's not a reason.

I should probably shut up before I'm kicked out of the project for being an infidel. ;)

p.s. Just saw @heddn's changes to the issue tags. Can you clarify in what way this is a blocker to anything in contrib? The only blocker I can see from a read of the linked issue is that "as we move things from migrate contrib into core, we need it". Sure, we need a way to alter. We have a way. What is this blocking? No further comment on the "DX" tag. *sigh*.

Good point about needing more context on why this is desired. I should have linked it: https://github.com/drush-ops/drush/pull/3402

And we do have things from contrib and custom wanting to alter rows. Commerce Migrate does it today I believe. And we also want to alter that in custom and override contrib. But the real reason is we need more context for drush. We could add the context to the alter hook, possibly break BC and require the nasty hack of drush needing to implement the row alter on behalf of system module. Or we could use an Event.

The pro to an event (it could also be a con) is that we can implement a single event per chunk of business logic. All the logic is self-contained. With the alter, we can have multiple migrations we want to alter. Think derived nodes. So then our single alter hook becomes a rather large switch statement. We have to use the single alter hook, not the per migration id hook, because we want to support migrate_upgrade where the id is rewritten. Now we have a huge, long alter hook with all our messy logic embedded in a single large alter hook.

Events are testable. For migrations especially, testing everything is super useful. Doing that with a hook is pretty hard.

Can we work around these things. Yes. But I prefer not to. Drupal console will spit me out an event subscriber in 5 seconds. And now I have unit testable, self-contained code that I can apply to one, two or 10 migrations based on tags or other criteria. And I can build any number of these event subscribers to apply where ever or whenever I want.

I don't expect I will convince anyone that events are better than hooks, but many folks like them. And once an event is introduced, it doesn't make sense to maintain two methods to alter the same thing. So we should at least consider deprecating the alter hook.

@heddn: Thanks! That's extremely helpful. I don't totally buy all the points, but that's vastly more to go on than "hooks--".

Re: splitting logic: that's what hook_migrate_MIGRATION_ID_prepare_row() is for, right? Or you're saying in some cases the MIGRATION_ID is just "d6_node" and the deriver magic (which I still haven't grokked) actually spawns multiple migrations depending on the different node types? In which case, if I only want to alter rows for a specific node type, I have to still have the hook fired on all node types and conditionally ignore the ones I don't care about...

Related question/point: how can anyone write reusable code for any of this when the migration IDs can change all the time? My migration might be "d6_node", or "upgrade_d6_node" or "upgrade_d6_node_[type]" or any # of other things depending on how I choose to setup my migration for a given site. Again, it seems like the only way to alter anything is to be the one in charge of the migration, know what migrations you're using, and alter their source rows accordingly. How can commerce alter anything about my migrations if it has no idea how any of them are identified? Would an event improve that situation? If so, how?

Thanks again for being willing to put up with my skepticism in a healthy way!

Cheers,
-Derek

In the event you have the full migration object. So you can filter on arbitrary migrate_tags, existence of fields, time of day, really anything. And early exit from the event if it doesn't meet the criteria. If you want to filter on nodes types, you can inspect the destination entity type. If you want to do it for certain bundles, then check that. Or you can do string preg_match comparisons on the id of the migration. But I tend to avoid that unless it is my own custom migrations that I know won't change names.

With the hook, we just have hook_migrate_MIGRATION_ID_prepare_row_alter() or they all dump into the hook_migrate_prepare_row_alter(). Most things dump into the fallback. And we're back to a huge old alter. With 150+ migrations on a typical migration, that can lead to very messy and long code in the alter hook.

Bumping priority to major since this is a blocker for drush core integration.

1. +++ b/core/modules/migrate/src/Event/MigrateEvents.php
   @@ -76,6 +76,23 @@
   +   * be used to add data to the row, manipulate the data into a canonical form,
   +   * or signal by exception that the row should be skipped. The event listener
   

   I have never liked the fact that we use exceptions as a signaling system. Can we add a method to MigratePrepareRowEvent (skip() or something) which can be used to mark the row as skipped?

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

   We don't need this class at all. Everything in it already exists in MigratePreRowSaveEvent. The only reason we might need a specialized class here is to implement a skip() method, in order to move away from using exceptions as a signaling system.

   getSource() is superfluous; people should just call $event->getMigration()->getSourcePlugin(), so let's remove that.

Sorry, but I think we still need to tinker with this :)

Re: #29: Alter hooks can check conditions and exit early, too. That's an unfair comparison.

The heart of this issue seems to be we want to pass more context to the row altering mechanism.

So, basically:

Dispatch an event so we can pass the full migration object as context. It's a new event, so it's not an API break, it's an API addition.

vs.

Pass the migration object as context to the alter hook. API change. Everyone freaks out.

Seems like a sort of backhanded way to get an API change into a "stable" release, under the guise of "events++".

If it were me, I'd title this issue:

"Pass more context to the things trying to alter migration source rows."

That's actually the problem you're trying to solve.

Side note: I ran head-first into drush's --idlist feature the other night. It's 100% broken that it cares about this issue at all. I happened to want to test some fancy migration logic on a specific node I knew from the source data was going to give it a hard time, so I did this:

drush mim upgrade_d6_node_story --idlist=38534

And waited. And waited. And waited. WTF? I killed that and tried at the beginning, and sure enough, it was cranking through --limit=10 in no time. I never actually dug into the code, but based on this issue, you're telling me that using --idlist tells drush to iterate over every single row from a given source, poll each one to see if it matches the idlist, and ignores/skips/whatever all the non-matching rows? I desperately hope I'm wrong about that, but that seems to be what this issue is saying is a "contrib blocker".

If we're going to be getting API changes into core to help --idlist, let's give it a way to modify the query for the source providers so that it limits the rows that the migration is trying to prepare in the first place, not iterate over every, single, one, until it finds a matching ID. I want --idlist to add WHERE nid IN ( :nids[] ) (or appropriate, depending on the source plugin) not give it another way to "elegantly" iterate over 10s of thousands of records.

Meta: I have no idea why I decided to care about this issue, or dig in my feet like this. Apparently, this is my frustration about how cryptic and difficult to debug I've found the migration code while trying to get a couple of D8 migrations working recently, coming out sideways. I made good use of this hook to solve a bunch of my problems, and before I can even launch the site, y'all want to deprecate the hook. It's just a bit soul crushing. Apologies for the misplaced energy. Please carry on...

We discussed this in the Migrate maintainer meeting this morning on IRC.

I agree with @heddn's opinion that we should throw our weight behind events and deprecate the hooks. Events are better from a DX and testing perspective, and it makes things much cleaner where the Drush-based migration tools are concerned. Migrate already takes advantage of the event system, and it's poor form to have two ways to do the same thing (in this case, influence the "preparedness" of a row). Migrate is cryptic enough, and we don't need to have even more ways to confuse developers.

One concern I have, to that end, is that I'd like to make sure that our documentation is in line with the changes introduced in this patch, when it is committed.

But, all that said, +1 for events.

Tagging for docs.

+1 from me (unsurprisingly;).

I had this on my review list, but not sure I'm the one to RTBC it given it's partly based on my migrate_plus code. Although, the only thing directly taken is the event class which is trivial, so maybe I can? Anyway, the last patch is a couple of months old so I've submitted tests for 8.5.x and 8.6.x.

Hi!

I quickly checked our handbooks on what we have on the hooks and events.

We currently have this page:
https://www.drupal.org/docs/8/upgrade/customize-migrations-when-upgradin...

I also reviewed the chane record draft. It looks good to me. Once this issue lands, the text on the change record can be used for updating the handbook page.

I'll open a follow-up documentation issue so that we remember to do that once this lands.

I did not have time to review and test the patch thoroughly so leaving this is to NR, but at least this covers the 'Needs documentation' tag.

Cheers,
Markus

Documentation follow-up opened, see #2972795: Update hooks / events page in Upgrading to Drupal 8 Handbook

getSource() is superfluous; people should just call $event->getMigration()->getSourcePlugin(), so let's remove that.

from #32 by phenaproxima still makes sense

1. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -240,8 +249,10 @@ protected function getModuleHandler() {
   -      $result_hook = $this->getModuleHandler()->invokeAll('migrate_prepare_row', [$row, $this, $this->migration]);
   -      $result_named_hook = $this->getModuleHandler()->invokeAll('migrate_' . $this->migration->id() . '_prepare_row', [$row, $this, $this->migration]);
   +      $this->getEventDispatcher()->dispatch(MigrateEvents::PREPARE_ROW, new MigratePrepareRowEvent($row, $this, $this->migration));
   +      $deprecation_message = 'Replace hook implementations with \Drupal\migrate\Event\MigrateEvents::PREPARE_ROW event subscribers. In order to skip the row, throw \Drupal\migrate\MigrateSkipRowException in the event subscriber. See https://www.drupal.org/node/2952459';
   +      $result_hook = $this->getModuleHandler()->invokeAllDeprecated($deprecation_message, 'migrate_prepare_row', [$row, $this, $this->migration]);
   +      $result_named_hook = $this->getModuleHandler()->invokeAllDeprecated($deprecation_message, 'migrate_' . $this->migration->id() . '_prepare_row', [$row, $this, $this->migration]);
   

   Maybe move hook execution to default subscriber so for 9.x hook will be just removed from event subscriber?

2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -599,4 +610,19 @@ public function getSourceModule() {
   +   * Returns the event dispatcher service.
   ...
   +   * @todo Properly inject this service in Drupal 9.x.
   ...
   +  protected function getEventDispatcher() {
   

   needs link to postponed issue

Also this subscriber can cache discovery of hooks and do not fire hooks when no hooks defined

NW for #42.

This might actually help with the skip row exception request from @phenaproxima. Feedback in #42 is addressed now.

Some code cleanup.

Ran out of time converting the tests in MigrateSourceTest. We should probably evaluate how we are doing all the skip row logic in that test and replace it with an event dispatcher stub instead of the module handler. And then test the event subscriber more directly.

OK, I got some time to convert the tests. Let's see what I missed.

Fixed version numbers in the deprecation notices.

I have read through the comments and the patch and I agree that this makes sense and that it's quite ready. I only have 2 questions before I can RTBC this:

1. Since the linked blocked drush issue was closed (https://github.com/drush-ops/drush/pull/3402), we still want this into core anyway, right?
2. Can you please update the part of the IS about the API changes? Right now it says "None" but I think we should mention the API addition and the hook deprecation. It's already mentioned in the CR, but let's also be clear here in case someone comes back here.

Back to NW for the IS change.

58.1: Yes, I think we would like to proceed here. Contrib and other's would like to use the events. Migrate runner, for example.
58.2: Fixed.

Thanks @heddn, RTBCed.

Did anyone open a bug report for drush mim --idlist being broken? Is that in a migrate project here on d.o or has it all been merged into drush core itself at this point? Do drush core bug reports only happen via GitHub issues now? I searched https://github.com/drush-ops/drush/issues and couldn't find anything about idlist. Should I open an issue there and link it here?

Leaving at RTBC (although I haven't looked at recent patches). No real remaining objections.

Thanks,
-Derek

@dww, do you mean that this patch broke drush mim --idlist or that it is currently broken with or without this patch?

If it was this patch that broke it, then we should definitely investigate it more here before this lands.

Cheers,
Markus

1. +++ b/core/modules/migrate/src/Plugin/PluginEventSubscriber.php
   @@ -78,6 +98,33 @@ public function postRollback(MigrateRollbackEvent $event) {
   +    $skip = $result_hook && in_array(FALSE, $result_hook);
   ...
   +    $skip = $result_named_hook && in_array(FALSE, $result_named_hook);
   

   should we be using the third argument to in_array here?

   https://3v4l.org/qKRNs makes me think we should

2. +++ b/core/modules/migrate/src/Plugin/PluginEventSubscriber.php
   @@ -78,6 +98,33 @@ public function postRollback(MigrateRollbackEvent $event) {
   +    if ($skip) {
   ...
   +    if ($skip) {
   

   do we need the intermediate local variable here?

3. +++ b/core/modules/migrate/src/Plugin/PluginEventSubscriber.php
   @@ -87,6 +134,7 @@ public static function getSubscribedEvents() {
        $events[MigrateEvents::POST_ROLLBACK][] = ['postRollback'];
   +    $events[MigrateEvents::PREPARE_ROW][] = ['hookPrepareRow'];
   

   any reason to retain the hook in the naming, no other methods do?

4. +++ b/core/modules/migrate/tests/src/Unit/MigrateSourceTest.php
   @@ -289,14 +294,14 @@ public function testPrepareRow() {
   +    $reflection = new \ReflectionClass($source);
   +    $reflection_property = $reflection->getProperty('eventDispatcher');
   +    $reflection_property->setAccessible(TRUE);
   +    $reflection_property->setValue($source, $mock_event_dispatcher->reveal());
   

   ick, surely a setter is better than this

5. +++ b/core/modules/migrate/tests/src/Unit/MigrateSourceTest.php
   @@ -307,110 +312,65 @@ public function testPrepareRow() {
   -    $row2->rehash()
   -      ->shouldBeCalled();
   ...
   +    $row2->rehash();
   

   intended change?

6. +++ b/core/modules/migrate/tests/src/Unit/MigrateSourceTest.php
   @@ -446,6 +406,18 @@ protected function getMigrateExecutable($migration) {
   +  protected function setEventDispatcher() {
   

   do we need this and the reflection nastiness?

re: #62: the bug in drush mim --idlist I'm talking about is my "side note" from comment #35:

I never actually dug into the code, but based on this issue, you're telling me that using --idlist tells drush to iterate over every single row from a given source, poll each one to see if it matches the idlist, and ignores/skips/whatever all the non-matching rows? I desperately hope I'm wrong about that, but that seems to be what this issue is saying is a "contrib blocker".

If we're going to be getting API changes into core to help --idlist, let's give it a way to modify the query for the source providers so that it limits the rows that the migration is trying to prepare in the first place, not iterate over every, single, one, until it finds a matching ID. I want --idlist to add WHERE nid IN ( :nids[] ) (or appropriate, depending on the source plugin) not give it another way to "elegantly" iterate over 10s of thousands of records.

Meanwhile, looks like this needs work based on #63.

Does this stand a chance of committing in 8.8 as we prep for 9.0? I don't want to keep the prepare_row_alter in 9.0. But this has been bogged down on the politics of an event vs hook so I don't want to clean things up if it won't land.

It needs to address #63 and https://www.drupal.org/core/deprecation#how-hook

Rerolled against 10.1.x

Rerolled against 10.1.x

Great, now hook implementation replacements are required to do in the scope of this issue.