Move highwater field support to the source plugin, and do not expose its internals on MigrationInterface

1. +++ b/core/modules/migrate/config/schema/migrate.schema.yml
   @@ -19,6 +19,8 @@ migrate.migration.*:
   +    highWaterProperty:
   

   Coding standard is underscore-separated. Also, I think "field" is better than "property" here.

2. +++ b/core/modules/migrate/config/schema/migrate.schema.yml
   @@ -19,6 +19,8 @@ migrate.migration.*:
   +      type: mapping
   

   Needs to be fleshed out farther, defining 'name' as required and 'alias' as optional.

3. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -175,12 +175,13 @@ protected function initializeIterator() {
   +        $this->query->orderBy($high_water_field);
   

   In Migrate Classic™, properly ordering the query has been the responsibility of whoever defined the query. I'm a little concerned automatically forcing the order here might not work for all queries. Also, in some cases the query definer might want to use a second (or beyond) field for ordering when there are dupes in the highwater field.

getHighWater() on the first run returns NULL, not a string.

o, that never used to be the case in D7, from memory it was an empty string.

IMHO, NULL makes a lot more sense than an empty string for that...

Yes I agree, I was just pointing out before that it was likely un-changed code (or logic) from D7.

Made a few changes here.

First, I think that the high-water property should be defined as part of the source plugin's configuration, not as a top-level property of the migration. High-water properties don't apply to all migrations; and for migrations that do use them, they only have any meaning to the source plugin anyway. And on top of that, they really (so far, anyway) only have meaning for SQL-based sources. So this patch alters the config schema so that high_water_property is a property of the migrate_source_sql data type.

Secondly, there were a couple of PHPUnit tests which ostensibly use high-water properties, but they were not setting values which would actually be used by the SqlBase plugin (it was looking for $high_water_property['name'], and the tests were setting $high_water_property['field']). That's fixed now.

Whoops, forgot to adjust a few tests.

Looking at the Migration class, we might have to restore highWaterProperty to the config schema, because $highWaterProperty is a property of the Migration entity type, so methods probably depend on it. What do you think?

I totally agree this should have some real tests.

And on top of that, they really (so far, anyway) only have meaning for SQL-based sources.

Why? If you handle it in your custom source it should work just fine?

Re-added the high water property to config schema and fixed broken tests (which were using a nonsensical high-water property in their $migrationConfiguration, thus causing SQL errors -- another example of something that had never worked, but been covered up by age and bugs).

There already is at least one passing test which uses the high-water property (CommentSourceWithHighWaterTest), so I'm removing the "needs tests" tag.

This fixes the only real test of high-water property support (CommentSourceWithHighWaterTest) against HEAD, to prove the problem actually exists and is rectified by the patch in #18.

Derp, duplicate comment.

Fixing them thar test failures in #14.

All fixed.

Unblocked.

Hi, I've also been trying to fix high water property. My solution is very similar so I won't post it here. The Kernel test, however, could be useful (https://www.drupal.org/node/2690757#comment-10988135). Shall I create a separate issue containing just the test?

Attaching Kernel test.

Attached patch is an attempt to combine migrate_sqlbase_highwater_fix-2690757-7.patch and 2485385-21.patch (re-rolled).

This needs a re-roll after #2694009: MigrationInterface doesn't include get() was committed.

Also, if we rename the high water property, we might consider changing the name of the associated getter too.

The next step here is to replace all the

->get('foo')

calls to ->getFoo() calls.

We have a Major issue for documented functionality that is badly broken - how did we never set it to migrate-critical?

Looked at the last patch, it needs more than the get() calls changed (still based on migrations as configuration entities).

I'm working on rerolling the patch from #29 and stuck on errors from this test, core/modules/migrate/tests/src/Kernel/SQLBaseTest.php. The error is:

    Drupal\Tests\migrate\Kernel\SQLBaseTest::testSQLMigration
    Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException:
    You have requested a non-existent service
    "plugin.manager.migrate.cckfield".

Testing the patch as is to see what else is broken.

Requeued the test, I don't see why it would trigger that error.

Not a complete review, just trying to guess at what might be triggering the test failure...

+++ b/core/modules/migrate/tests/modules/migrate_sql_source_test/migrate_sql_test.module
--- /dev/null
+++ b/core/modules/migrate/tests/modules/migrate_sql_source_test/migration_templates/migrate.migration.sql_test.yml

Now that migrations are plugins rather than config entities, 'migrate.migration.' should not be part of the .yml filename.

Postponing on #2560795: Source plugins have a hidden dependency on migrate_drupal. @mikeryan and I think that it's the reason for the mysterious test failure in #40.

The blocker is committed, let's retest.

Even though this is still blocked I want to address the failures and be ready for when it is unblocked.

The error was a result of the source test table being created in the destination database. The table was being created by the test module and that is now being done in the test, HighWaterTest.php.

And there is a lot of renaming in this patch as well. The high water test has been renamed from SQLBaseTest to HighWaterTest since it is testing the high water functionality and it follows the naming convention. The test module has been renamed to migrate_high_water_test In a similar fashion the test source plugin, the test table and the migration have all had a name change. I think it is much clearer that these are for high water tests and nothing else.

In order to run these tests without the blocker patch, the modules 'migrate_drupal' and 'fields' are installed in HighWaterTest.php. It is marked with an @todo, that seemed the best way to mark it.

Now let's see if there are any other errors.

Not sure how that error slipped in.

Retesting. Failure in OptionsFieldUITest.php appears to be unrelated, and that test passes locally .

I tried to make the high water mark logic a bit simpler with a new method. Also, I am not sure why the patch was named as 'test-only'. I checked the patches before and it seemed to be the whole patch file.

The fails are due to #2749955: Random fails in UpdatePathTestBase tests.

The failure is resolved.

Thanks for writing this. This is a very nice patch. But if you are breaking BC anyways, then please understand the sorry state for highwater and everything else baked into SourcePluginBase::next() is the very reason I am advocating for breaking up source into an iterator, into row additions and filters in #2543552: Modernize migration source plugins. If people would have the time and the willingness to work on it, would be great. It is extremely hard for me to just go and code some core because it would be nicer. I don't have such willingness for that after all that transpired. I just don't. I know what needs to be done but I can't.

The test from this patch could mostly be reused, most of the highwater functionality could be reused as well.

@chx, thank you for pointing me to that issue. I will try to catch up on the discussion in the coming week.

I was also not very concerned about breaking BC because migrate is experimental and we have done things like this in the past.

> I was also not very concerned about breaking BC because migrate is experimental

Wish certain elements of the community agreed with you.

I think this looks great. My only complaints are nitpicks.

1. +++ b/core/modules/migrate/tests/src/Kernel/HighWaterTest.php
   @@ -0,0 +1,189 @@
   +      ->values(
   +        [
   +          'title' => 'Item 1',
   +          'changed' => 1,
   +        ])
   +      ->values(
   +        [
   +          'title' => 'Item 2',
   +          'changed' => 2,
   +        ])
   +      ->values(
   +        [
   +          'title' => 'Item 3',
   +          'changed' => 3,
   +        ]
   +      )
   

   Bit of a nitpick, but can we follow the formatting used in ->fields()? (i.e., the opening bracket of each array on the same line as the opening paren)

2. +++ b/core/modules/migrate/tests/src/Kernel/HighWaterTest.php
   @@ -0,0 +1,189 @@
   +  public function testHighWater() {
   +
   +    // Assert all of the nodes have been imported.
   

   Extra empty line.

3. +++ b/core/modules/migrate/tests/src/Kernel/HighWaterTest.php
   @@ -0,0 +1,189 @@
   +    // Update Item 1 setting it's high_water_property to value that is below
   

   Nit: it's should be its.

4. +++ b/core/modules/migrate/tests/src/Kernel/HighWaterTest.php
   @@ -0,0 +1,189 @@
   +      ->fields(
   +        [
   +          'title' => 'Item 3 updated',
   +          'changed' => 4,
   +        ])
   

   Should be ->fields([...

5. +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlSourceTestCase.php
   @@ -82,6 +82,14 @@ protected function setUp() {
   +        ->will($this->returnValue(static::ORIGINAL_HIGH_WATER));
   

   Let's use willReturnValue() for readability.

All nitpicks fixed.

Fixed typo.

Thanks, @quiteone. This looks pretty righteous to me, but I'd like to the RTBC to come from one of Migrate's expert plumbers (@benjy, @chx, or @mikeryan)...

Patch looks good to me.

One confusing aspect is we now have getHighWaterProperty(), getHighWater() and getHighWaterField() all on the migration. Probably out of scope here but it would have been nice to have a HighWater value object that encapsulated those methods, and just have the one getHighWater() method on the migration. Alternatively, MigrateExecutable is now the only thing calling getHighWaterProperty()like so:

      if ($high_water_property = $this->migration->getHighWaterProperty()) {
        $this->migration->saveHighWater($row->getSourceProperty($high_water_property['name']));
      }

We could easily change that so that the executable didn't need any knowledge of the high water property at all?

      if ($high_water_property = $this->migration->supportsHighWater()) {
        $this->migration->saveHighWater($row);
      }

Maybe a follow-up?

I tested the patch but in my humble opinion, only the test works but beside that patch is not working.

The problem is, that in the test the configuration for the property is set up:

  protected function setUp() {
    $this->migrationConfiguration['high_water_property']['name'] = 'timestamp';
    array_shift($this->expectedResults);
    parent::setUp();
  }

But outside of the test, the yml configuration never gets picked up and therefore the whole highwater stuff is not working. Any suggestions?

*edited* seems not to be a solution.

I try to debug the highwater functionality and while debugging, I found out, that the highwater value is saved for every row:

web/core/modules/migrate/src/MigrateExecutable.php

      if ($high_water_property = $this->migration->getHighWaterProperty()) {
        $this->migration->saveHighWater($row->getSourceProperty($high_water_property['name']));
      }

As there is only one field in the KeyValueStore, does it really make sense to save the date for every row?

Created a follow up issue as suggested in #69, #2785233: Encapsulate HighWater methods

Sorry I haven't looked closely at this for a while - the implementation looks good, but I agree with phenaproxima's comment back in #9 - this configuration belongs on the source plugin, not on the migration.

OK. Patch reworked to put the high water on the source plugin. Also, had to modify MigrateSqlSourceTestCase so that the expectations (is the right way to say that) are set only on testRetrieval. That is the only method that initializes the iterator which is where the high water checks are.

Hmm, the status changed to Needs work while the test in #76 was still running. It did in fact pass, so changing to Needs review.

1. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -703,17 +703,20 @@
   +    if (isset ($this->getSourceConfiguration()['high_water_property'])) {
   +      $high_water_property = $this->getSourceConfiguration()['high_water_property'];
   

   Why not simply call $this->getHighWaterProperty()?

2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -210,20 +210,19 @@
   +    if (isset($this->configuration['high_water_property'])) {
   +      $high_water_property = $this->configuration['high_water_property'];
   

   Ditto here.

Darn. Meant to fix that.

Looks good - and tested in the (semi-)real world, at #2609310: Add highwater/track_changes examples to beer migrations. However, one more thing - there's really no logical reason for the Migration plugin to know anything about highwater marks, it should all be entirely contained within the source plugin. Now, while we're already breaking BC by changing the highwater property name, that's not a big deal (especially since it didn't really work) - but changing MigrationInterface would be. So, my suggestion is:

1. Copy all the highwater stuff currently in Migration into SourcePluginBase, and interfaces from MigrationInterface to MigrateSourceInterface.
2. Deprecate the public highwater methods on MigrationInterface, to be removed in 8.3.x.
3. Make the Migration implementations of those public methods wrappers around the SourcePluginBase methods.
4. Remove the saveHighwater() call from MigrateExecutable - do it in SourcePluginBase::next() instead, at the end of the loop (after aboveHighwater() is used).

@mikeryan, thanks for breakdown of the tasks. I usually think in lists, so that was helpful, and clear. Everything has been addressed but these are my first deprecations and wrappers. And there are still failing tests. Hopefully, only MigrateSourceTest.php and CommentSourceWithHighWaterTest.php.

1. For item #4, there were two uses of saveHIghWater in MigrateExecutable, the one at the end of the import loop has been removed but the one in the rollback remains.
2. The spelling is HighWater in the methods and properties, except for aboveHighwater. Should we change that to aboveHighWater?
3. I think the unit tests fail becuase getHighWater always returns NULL. Whatever I tried was wrong so the original, now incorrect, setup of getHighWater is unchanged.

I am praying to Cthulhu that this passes the tests. It turns out we ran smack-dab into dependency injection hell with this one -- MigrateSqlSourceTestCase (upon which many if not all source plugin tests are based) is a unit test, and the patch in #82 was trying to pull the high water value directly out of \Drupal -- and understandably so, because adjusting the dependencies is quite tricky. This patch removes getHighWaterStorage() in favor of actual dependency injection, which involves changing base class constructors. That constitutes a BC break, but that's OK because this issue was already tagged as such.

MigrateSqlSourceTestCase is, in my opinion, a mess. It's doing a lot of mocking and set-up that would be totally unnecessary if it were a kernel test, and this patch does nothing to fix that -- in fact, by introducing a new mockContainer() method, it might even be exacerbating the problem. But rewriting MigrateSqlSourceTestCase as a kernel test is very far out of the scope of this patch, so we'll have to get to it later.

Screwed up the mocked keyvalue service. Derp.

Missed a spot or two. If nothing else, this should greatly reduce the number of failures.

Okay...as of this patch, SourcePluginBase itself now implements ContainerFactoryPluginInterface. Kind of a sweeping change, but this does open the door to proper dependency injection for all source plugins. So it's a win for best practices, but hopefully not so far out of scope that we can no longer commit it. (If it is, I have yet another trick up my sleeve.)

At long last, a shattered and shell-shocked phenaproxima (hopefully) crosses the finish line. Working on our source plugin tests is like stumbling blind through a thorny jungle in hell.

I'm not sure how I feel about about changing SourcePluginBase - that's a more far-reaching change than I had hoped. But, it may be necessary, I see...

Why are public functions added to MigrateSourceInterface? Highwater support should be entirely encapsulated within the source plugin, the internals shouldn't be exposed.

Okay. This patch fixes the tests without imposing DI on everything. It's messy as all hell, but that is because the source plugin tests are so awful -- they mock the universe, and they have to be incredibly careful about when they do things, or the whole house of cards implodes. It's a nightmare. Let's get #2791119: Write meaningful Migrate source tests in ASAP and we can start to undo the damage.

I still need to hide all the high-water stuff from outside code, but that should be relatively easy now that the tests are passing. On my local machine, anyway.

OK, this removes the changes to MigrateSourceInterface and makes the high-water methods of SourcePluginBase protected.

OK, so...the source plugin needs to be able to reset the high-water mark once rollback is complete. After discussion with @mikeryan on IRC, we agreed that the best way to do that was to allow source and destination plugins to be event subscribers and react to various events dispatched by MigrateExecutable. That's what I've done here. Dear lordy, let it pass the tests.

And that, kids, is why I don't like using invocation assertions in unit tests.

1. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -703,7 +662,7 @@ public function getSourceConfiguration() {
      public function getHighWaterProperty() {
   -    return $this->highWaterProperty;
   +    return $this->getSourcePlugin()->getHighWaterProperty();
      }
   

   Why weren't we able to remove this from the Migration plugin along with the rest of the highwater methods?

2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -60,6 +53,20 @@
   +  /**
   +   * @var \Drupal\Core\KeyValueStore\KeyValueStoreInterface
   +   */
   

   Description needed.

Both fixed. I ran the migrate and migrate_drupal test suites before posting this patch, so I have high hopes that it will pass Drupal CI.

Removed a couple of dead use statements.

okie dokie!

Nice to see another Migrate critical RTBC!

This issue is technically rc eligible according to https://www.drupal.org/core/d8-allowed-changes#rc. However, given the importance of this issue, I'm actually going to tag it as an RC target so that it is a top priority to add during RC in case it is not committed before the commit freeze Tuesday morning.

The title and summary of this issue describe various problems but not the proposed resolution or the API changes. Can we get a CR for this issue and if possible retitle/re-summarize it to describe the fix? (The word "Various" in a title always makes me nervous.) :)

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -300,9 +300,28 @@ public function import() {
   /**
+   * Registers the source and destination plugins as event subscribers.
+   */
+  protected function registerEventSubscribers() {
+    $event_dispatcher = $this->getEventDispatcher();
+
+    $source = $this->migration->getSourcePlugin();
+    if ($source instanceof EventSubscriberInterface) {
+      $event_dispatcher->addSubscriber($source);
+    }
+
+    $destination = $this->migration->getDestinationPlugin();
+    if ($destination instanceof EventSubscriberInterface) {
+      $event_dispatcher->addSubscriber($destination);
+    }
+  }

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -417,4 +432,92 @@ protected function getCache() {
+    $events[MigrateEvents::POST_ROLLBACK][] = ['postRollback'];

Re-using the event system seems pretty clever but I wonder if it could get us into trouble. This could cause a double registration if the plugins were registered a services and tagged with 'event subscriber'. The double registration might occur if the an import and rollback occurred in the same process. Do we have other use-cases for this functionality other than listening to MigrateEvents::POST_ROLLBACK?

Change record drafted and issue summary updated.

This could cause a double registration if the plugins were registered a services and tagged with 'event subscriber'.

Would a double registration cause the registered event handlers to be evoked multiple times, or is the dispatcher smart enough to dedupe them? I'll see if I can figure that out...

Yep, they get evoked multiple times.

@phenaproxima - what if we go back to calling $source->postRollback() directly, as was discussed in IRC along the way to this solution?

We could have an interface on the plugin's to say that they have a postRollback method and call it if the plugins implement it.

Okay, I changed the approach. I think this way will please everybody.

I've created a new event subscriber that subscribes to the pre-import, post-import, pre-rollback, and post-rollback events. It handles the events by...calling the relevant methods on the source and destination plugins, if they implement them (which it verifies using method_exists()). I don't like the idea of having a new interface to define the event handling, because that's pretty much what EventSubscriberInterface is for.

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -22,7 +21,7 @@
+abstract class SourcePluginBase extends PluginBase implements MigrateSourceInterface, RollbackAwareInterface {

RollbackAwareInterface is not a thing -- it's an artifact from another approach I tried. I could roll a new patch, but can it just be removed on commit?

Iced unused implementation of getSubscribedEvents() in SourcePluginBase.

Let's do this thing!

Retitling based on the CR. Thanks @mikeryan!

1. +++ b/core/modules/migrate/migrate.services.yml
   @@ -1,4 +1,8 @@
   +  migrate.plugin_event_subscriber:
   +    class: Drupal\migrate\Plugin\PluginEventSubscriber
   +    tags:
   +      - { name: event_subscriber }
   

   I really like the idea of a bridge service. Nice one.

2. +++ b/core/modules/migrate/src/Plugin/PluginEventSubscriber.php
   @@ -0,0 +1,88 @@
   +    $source = $migration->getSourcePlugin();
   +    if (method_exists($source, $method)) {
   +      call_user_func([$source, $method]);
   +    }
   +
   +    $destination = $migration->getDestinationPlugin();
   +    if (method_exists($destination, $method)) {
   +      call_user_func([$destination, $method]);
   +    }
   

   Look like interfaces are appropriate here. That way we also get documentation (which this patch is lacking) for free. We need an interface per method (MigratePluginPreImportInterface etc...) Also I think we should pass the event to the method.

3. +++ b/core/modules/migrate/src/Plugin/PluginEventSubscriber.php
   @@ -0,0 +1,88 @@
   +    $this->invoke('preImport', $event->getMigration());
   ...
   +    $this->invoke('postImport', $event->getMigration());
   ...
   +    $this->invoke('preRollback', $event->getMigration());
   ...
   +    $this->invoke('postRollback', $event->getMigration());
   

   here we could pass the invoke method the interface as well as the method to make checking it simple.

Interfaces are appropriate, but I had decided to skip them for now in the name of getting this damn patch done, since it's one of the final blockers to Migrate API stability. I guess I don't mind putting interfaces on the plugins, but having an interface for each event seems clunky. I'd rather define two interfaces: ImportAwareInterface (defining preImport and postImport methods), and RollbackAwareInterface (preRollback and postRollback). So that's what I've done in this patch.

Looks good - just one more thing, can you update the change record to document the new interfaces?

Since ImportAwareInterface and RollbackAwareInterface are not strictly related to the high-water fixes, I have opened a new change record.

Good call, thanks!

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -324,6 +335,10 @@ public function next() {
+      if ($this->getHighWaterProperty()) {
+        $this->saveHighWater($row->getSourceProperty($this->highWaterProperty['name']));
+      }

So what's odd to me is that we save the highwater mark row by row on the way in but if we're rolling back we just null it at the end. Is there anyway we can make it right all the time? Ie... if a rollback breaks there is no guarantee that a user would reset the migrate status.

Changing it would probably be a much bigger API break than this patch already constitutes. @mikeryan can confirm or deny that, but either way, it seems to me that changing migrations' highwater handling during rollback is well outside the scope of this issue. This patch is about getting highwater functionality to work the way it was always supposed to, but didn't due to a bug in SqlBase.

Is there anyway we can make it right all the time?

Consider if we rollback 10 items. To make the highwater "right" would require first that the 10 items rolled back would have been the last 10 imported. Actually, maybe the 10 with the highest source-side highwater field values would be strictly speaking the ideal. And then the highwater mark would need to be set to the source highwater field value of the 11th item - the next one to be rolled back. Keep in mind that the rollback process is an iterator (currently unsorted) over the map table, and does not use the source.

If someone wants to tackle this - good luck with that (in a followup - I think that's out of scope here). In the meantime, nulling the highwater mark is the safest recourse - the worst-case scenario if rollback is incomplete and for some reason the migrator decides to run an import rather than complete the rollback, the previously-imported items will be reimported. All that's lost is time, not data.

Fixing a TODO pointed out by @alexpott. Seeing as how 128 is a holy computer number, it will be very satisfying if this is the one that gets committed. =P

w00t! Back to RTBC.

Committed and pushed 0308eb7 to 8.3.x and 7c8ebc3 to 8.2.x. Thanks!

Whilst reviewing this patch I pondered if we could set the highwater properly during rollback (see #125 and #127). After thinking about @mikeryan's response some more I think we should more the NULL setting from postRollback() to preRollback() this means that is the rollback fatals for any reason then the highwater is correctly indeterminate rather than wrong.

diff --git a/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
index 4b3da85..f393dc0 100644
--- a/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -452,7 +452,7 @@ protected function getHighWaterStorage() {
    * run. If the mark is set, only content with a higher timestamp will be
    * imported.
    *
-   * @return int|NULL
+   * @return int|null
    *   A Unix timestamp representing the high water mark, or NULL if no high
    *   water mark has been stored.
    */
@@ -491,7 +491,7 @@ protected function getHighWaterProperty() {
    *
    * @see \Drupal\migrate\Plugin\migrate\source\SqlBase::initializeIterator()
    *
-   * @return string|NULL
+   * @return string|null
    *   The name of the field for the high water mark, or NULL if not set.
    */
   protected function getHighWaterField() {

Fixed on commit. Yep we have automated coding standards for that :)

@alexpott: Good point, followup created: #2800715: Reset highwater mark *before* rolling back.

Thanks!

I'm pretty sure this just broke HEAD. Having trouble confirming locally because InstallUninstallTest is not exactly laptop-friendly.

*knock, knock* is this on? Did anyone read #60? Why did this continue much less went in?

Confirmed. I had to roll this back. Here's the fail:
https://www.drupal.org/pift-ci-job/462223

Module.Drupal\system\Tests\Module\InstallUninstallTest
✓		- setUp
✗	
testInstallUninstall
fail: [Other] Line 210 of core/modules/system/src/Tests/Module/InstallUninstallTest.php:
Modules status has been updated.
✗	
rupal\migrate\Plugin\PluginEventSubscriber::getSubscribedEvents
exception: [Error] Line 86 of core/modules/migrate/src/Plugin/PluginEventSubscriber.php:
Class 'Drupal\migrate\Event\MigrateEvents' not found
✗	
nkno
fail: [Error] Line 86 of core/modules/migrate/src/Plugin/PluginEventSubscriber.php:
Class 'Drupal\migrate\Event\MigrateEvents' not found

@chx, #60 was answered. Migrate is in alpha. Until it is in beta, there is no BC promise. This issue is one of the last BC breaks listed as critical, so we are getting close to a beta that we all hope to have very soon.

Edit: Or if the question is "Why don't we break BC even more and block it on even better improvements," the answer is "because we want to get to a beta very soon".

So, MigrateEvents not found is something I've heard of before (without this patch) - people have reported it but I've never been able to reproduce it, nor figure out how that would happen - if migrate is enabled, the MigrateEvents class should be available. There's actually an open issue for this: #2776235: Cached autoloader misses cause failures when missed class becomes available

So, basically, there's nothing actually wrong with the highwater patch, it's that the event subscriber is triggering that existing autoloader issue. I'm curious, though, why it only surfaced after the commit?

@mikeryan, got me unfortunately. Usually I'd say something between the test run and commit but in this case that seems unlikely since the patch is right from this morning. It could be something @alexpott changed on commit causing a regression unexpectedly.

Retesting on 8.2.x and 8.3.x.

Here's the patch with @alexpott's coding standards fix; can't imagine this would have caused any difference. Maybe 8.2.x vs. 8.3.x.

I queued both branches and all three PHP versions. The fail was across DBs in HEAD.

Ugh, sorry, I failed to push 8.2.x. Retesting those now that I did. And of course the 5.6 fail is irrelevant because of #2762549: Drupal\field\Tests\Update\FieldUpdateTest, Drupal\views\Tests\Update\EntityViewsDataUpdateTest and Drupal\comment\Tests\CommentFieldsTest fail on 8.1.x.

Ignoring the 5.6 failure (as per @xjm's comment in #147), I'm seeing two things:

1. The patch fails absolutely consistently on PHP 7.
2. We never tested the patch on PHP 7 (or, indeed, 5.6) before committing it.

I'm testing it locally on PHP 7 to see if that is the source of the problem.

#138 let me clarify: I argued in #60 that this issue should be shuttered and the code reused in #2543552: Modernize migration source plugins which would break out highwater into its own plugin.

So the problem is that on PHP7 content_translation is install before migrate and the autoloader caches the missing class in \Drupal\content_translation\ContentTranslationUpdatesManager::getSubscribedEvents(). The only way to fix it in this issue is to do a module exists check instead. Other possibilities are to change multiple module install to be batched and do one by one (slow) or the way would be to wrap the classloader in something that would make it possible to remove cached misses from the classmap.

Interdiff looks good to me, pending reroll and test success - we can use #2776235: Cached autoloader misses cause failures when missed class becomes available for potentially following up with those other possibilities.

Thanks alexpott!

Maybe @alexpott can comment on this, but the diff stats from #151 seem to also include some stuff for the outside_in module. I didn't include them in the re-roll. The diff stats make more sense when compared to #128.

Oopsie my diff included some stuff it should not... thanks @heddn

Okay so the module exists check is not going to work because we're inside a container rebuild and all bets are off :(

Therefore we need to replace the class loader when we update modules on the kernel.

I did not mean to change the version of this issue in #157 - sorry.

New patch with the essential bits of #2776235: Cached autoloader misses cause failures when missed class becomes available just to see if it works...

Proper interdiff.

Heyo! Looks like @alexpott's fix corrects the problem. So it seems to me that we need to postpone this on #2776235: Cached autoloader misses cause failures when missed class becomes available which, I'm told, is going to be a critical.

Once this is unblocked, we'll need to re-roll this patch again, without the change to ContentTranslationUpdatesManager.

Just want to see what happens with this. Event classes are one of my many complaints with the Symfony event system. interdiff is vs. #155

So while the other bug is a real bug, we're also not doing ourselves any favours here either.

The hooks system allows modules to declare hook implementations regardless of whether another module is installed or not. The hook (like event listeners) only gets invoked when it's invoked anyway.

This works with event listeners, but the convention of putting the event name in an (arbitrary) constant on the event subclass discourages this, so we end up registering an event depending on whether the module that might invoke the event exists or not. What appears superficially to be more robust, turns out to be extremely fragile. Just using the string also means the subscriber would survive a class rename, which is if anything more likely than changing the string.

@catch: I see your point, but addressing the event constant class pattern seems out-of-scope here - that's been the practice throughout core, if we are to change it that should be an issue of its own.

I'm going to resubmit the tests on the patch as originally committed (#144) - if those pass, can we get this in?

Oops - I saw #2796953: [regression] Plugins extending from classes of uninstalled modules lead to fatal error during discovery was committed and confused it with #2776235: Cached autoloader misses cause failures when missed class becomes available, which is the issue we've been waiting on here, and which has not been committed.

Given it seems like RC2 is imminent and we really want to get in there, I'm willing to go ahead with catch's answer - RTBC on the assumption the full set of tests I'm about to launch passes.

Temporary to make it easy to launch all the 8.3.x tests.

Didn't actually help...

Random bot fail on PHP 5.5/SQLite 3.8 - passed on rerun, but for some reason the red one didn't get removed. Anyway, passing all tests now...

Looks like this is going to need a reroll for 8.2.x.

Rerolled against 8.2.x, accounting for the changes introduced by #2684567: Requiring a migration w/ a source plugin using a generator fatals. No interdiff due to rejected hunks.

+++ b/core/modules/content_translation/src/ContentTranslationUpdatesManager.php
@@ -95,9 +95,7 @@ public function onMigrateImport(MigrateImportEvent $event) {
-    if (class_exists('\Drupal\migrate\Event\MigrateEvents')) {
-      $events[MigrateEvents::POST_IMPORT][] = ['onMigrateImport'];
-    }
+    $events['migrate.post_import'][] = ['onMigrateImport'];

I don't think we should be making this change in this issue. We might very well decide to make a recommendation not to use events classes like this but that should be discussed in its own issue. I think we should do #2776235: Cached autoloader misses cause failures when missed class becomes available before this - so reviews there are welcome :)

The attached patch combines the latest patch here minus the MigrateEvents avoidance with the autoloading patch at https://www.drupal.org/node/2776235#comment-11659157, so the testbot can verify that this specific victim of the cached autoloading will be fixed by that patch.

Now that #2776235: Cached autoloader misses cause failures when missed class becomes available is in, I'm retesting the patch #144 (the patch that was originally committed here, without any later workarounds for the related issue).

Of course...