Drupal\migrate\Plugin\migrate\source\SourcePluginBase::rewind() is rewinding database statements

Fixing the test to not result in a rewind is simple. See patch attached. This happens because we try to re-run the migration using an existing migration object...

So the only reason \Drupal\Tests\migrate\Kernel\TrackChangesTest was passing before we merged #3174662: Encapsulate \PDOStatement instead of extending from it was because migrate kernel tests are using sqlite as the source database and StatementPrefetch::rewind() no-ops. If they'd used another db engine the test would fail. And funnily enough the test is actually bogus because it does not run again here in HEAD. Because later in the test it does

    // Execute migration again.
    $this->executeMigration('track_changes_test');

To really re-run the migration as it's now made changes that would affect the result of running the migration.

#3 leads me to the conclusion that a hard break in \Drupal\Core\Database\StatementPrefetch::rewind() is preferable as it means that tests in contrib and custom will break if they really on the no-op and this is a good thing because it hides incorrect asssumptions.

Here's an even better fix than #2 and that would allow us to do #3265086-62: Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding and add a rewind test to prove all db drivers do the same no-op funkyness.

Include a patch that has this + what I think we should do on #3265086: Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding

LOL \Drupal\migrate\Plugin\migrate\source\SqlBase::rewind() is even more broken than we thought... it also does not account for batches...

Here's a test only patch to prove that we're testing the rewind here.

Also re-upping #6 to keep it the last patch on the issue.

+++ b/core/modules/migrate/tests/src/Kernel/TrackChangesTest.php
@@ -143,7 +143,7 @@ public function testTrackChanges() {
-    $this->executeMigration('track_changes_test');
+    $this->executeMigration($this->migration);

There's second place where executeMigration() is called and both places are suppose to instantiate migration from scratch, maybe it needs other test for rewind()?

@andypost - yes and now we've fixed the rewind bug doing $this->executeMigration($this->migration); works whereas before for the second execution we had to do $this->executeMigration('track_changes_test'); - this is exactly what the test only patch shows.

I.e. "suppose to instantiate migration from scratch" is not correct - we don't need to re-create the migrate plugin to re-execute now that this patch has fixed \Drupal\migrate\Plugin\migrate\source\SqlBase::rewind() is fixed.

Ah sure, it exactly ensures for duplicates!

Nit: could use to extend comment to point that we call it again to make sure rewind() works, OTOH there's git blame

Further info. In HEAD the test does

    // Save the original hash, rerun the migration and check that the hashes
    // are the same.
    $id_map = $this->migration->getIdMap();
    for ($i = 1; $i < 5; $i++) {
      $row = $id_map->getRowBySource(['tid' => $i]);
      $original_hash[$i] = $row['hash'];
    }
    $this->executeMigration($this->migration);
    for ($i = 1; $i < 5; $i++) {
      $row = $id_map->getRowBySource(['tid' => $i]);
      $new_hash[$i] = $row['hash'];
    }
    $this->assertEquals($original_hash, $new_hash);

But the $this->executeMigration($this->migration); silently fails do anything - and the test is not actually testing what it thinks it is. With the change in #6 it finally is.

At the moment all the testing is pretty implicit and we could add something more explicit if we like. That said all is very edge case and the changed test is in the correct location so leaving at RTBC.

Good find. Given the test-only patch fails I think this is enough new coverage, and it unblocks the other issue. Committed/pushed to 10.1.x and cherry-picked to 9.5.x, thanks!

It was not pushed to 9.5.x for reason?

That was a typo, I only meant to cherry-pick to 10.0.x, however it makes sense to backport to 9.5.x if 9.5.x tests pass.

However we've also got patch releases today, so will move back to RTBC and cherry-pick once those are out.

Cherry-picked to 9.5.x.