Requiring a migration w/ a source plugin using a generator fatals

+1 on approach. Just skip counting, if

1. skip_count = TRUE
2. iterator isn't Countable

The downside, contrib and maybe core doesn't always implement \Countable, even though it has a count() method. It would be safer to check for count() on the object than trust that all classes implement the interface. For BC reasons at least.

migrate_source_csv might be the only one affected then. migrate_source_json seems to return \ArrayIterator. And I'm reworking it right now so we'll be sure to add \Countable to it. I don't know the status for xml, I seem to think it is less mature.

Oh, this is a pretty neat patch. Using a generator was always pratical already, just some admin features via drush didn't worked. It is great that we fix it.
IMHO we should add a test with a generator to ensure that generators continue to work from end to end.

Also still need to address the feedback in #9 regarding iterator_count messing the internal pointer of the iterator.

I really like #9. That's how I unknowingly got around the problem in csv, because there is/was an inherent (hidden) assumption that getIterator() returned an Iterator that was Countable. This approach just hides that.

The warning on iterator_count about not retaining the position of the pointer is a little scary. So we should do some testing in that area.

I'm going to work on this.

I'm still working on the tests.

Sorry this took me more time than I expected because in my country just finished the holiday season, hope be able to finish my tests this week.

Sorry, I have been having very hectic weeks and not sure if I'm gonna be able to work on this soon so I switch back the ticket to unassigned.

Ok I created a couple migration tests.

Both using a generator instead of an iterator and the second migration has as a dependency the first one.

I fixed some typos.

Hi gnuget, thanks for the tests. However, they don't test the actual thing we want to test. From the summary: "Create a migration where initializeIterator yields."

That means we want to use a PHP generator: http://php.net/manual/en/language.generators.overview.php

Hi gnuget, thanks for the tests. However, they don't test the actual thing we want to test. From the summary: "Create a migration where initializeIterator yields."

Actually, it does:

The initializeIterator use GeneratorObject which use:

  /**
+   * readFile.
+   */
+  private function readFile($file) {
+    $f = fopen($file, 'r');
+    try {
+      while ($line = fgets($f)) {
+        yield $line;
+      }
+    } finally {
+      fclose($f);
+    }
+  }
+

I had to do it in this way because the rewind method in the generators doesn't work as expected so I had to wrap the generator in another object and fix the rewind.

Also in the MigrateUsingGenerator I made two tests, one using the generator and a second one using as a dependency a migration which uses the generator plugin.

Thanks for your review!

1. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -141,10 +141,11 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   -    $this->cacheCounts = !empty($configuration['cache_counts']);
   -    $this->skipCount = !empty($configuration['skip_count']);
   -    $this->cacheKey = !empty($configuration['cache_key']) ? !empty($configuration['cache_key']) : NULL;
   -    $this->trackChanges = !empty($configuration['track_changes']) ? $configuration['track_changes'] : FALSE;
   +    foreach (['cacheCounts' => 'cache_counts', 'skipCount' => 'skip_count', 'cacheKey' => 'cache_key', 'trackChanges' => 'track_changes'] as $property => $config_key) {
   +      if (isset($configuration[$config_key])) {
   +        $this->$property = (bool) $configuration[$config_key];
   +      }
   +    }
   

   I don't see how reorganizing this code is in scope for this patch? Oh, now I do, so skipCount (and cacheCounts) is only overridden if explicitly in configuration. However, cacheKey is not supposed to be a boolean - that's being fixed in #2723115: Count cache configuration does not work as expected - so shouldn't be part of the loop here.

2. +++ b/core/modules/migrate/tests/modules/migrate_generator_test/src/GeneratorObject.php
   --- /dev/null
   +++ b/core/modules/migrate/tests/modules/migrate_generator_test/src/Plugin/migrate/source/generator.php
   
   +++ b/core/modules/migrate/tests/modules/migrate_generator_test/src/Plugin/migrate/source/generator.php
   @@ -0,0 +1,111 @@
   + * Contains \Drupal\migrate_source_generator\Plugin\migrate\source\generator.
   ...
   +class generator extends SourcePluginBase {
   

   s/generator/Generator/

3. +++ b/core/modules/migrate/tests/src/Kernel/MigrateUsingGenerator.php
   @@ -0,0 +1,152 @@
   +  protected $countries_file;
   ...
   +  protected $states_file;
   

   Coding standard for member variables is $countriesFile/$statesFile.

The test seems more elaborate than necessary to test the actual problem being fixed (counts) - presumably in response to dawehner's comment "IMHO we should add a test with a generator to ensure that generators continue to work from end to end." Maybe that should be a follow-up.

Speaking of which... I don't see the counts being tested?

Hmm. I've tried making migrations with just a generator in initializeIterator(), eg:

  public function initializeIterator() {
    $data = [
      ['title' => 'foo'],
      ['title' => 'bar'],
      ['title' => 'iggy'],
    ];
    foreach ($data as $row) {
      yield $row;
    }
  }

I didn't need to wrap it in an iterator. Did you try that? What went wrong?

I think it could make your tests much simpler. The simpler the better :)

Hi Vasi.

Hmm. I've tried making migrations with just a generator in initializeIterator(), eg:

  public function initializeIterator() {
    $data = [
      ['title' => 'foo'],
      ['title' => 'bar'],
      ['title' => 'iggy'],
    ];
    foreach ($data as $row) {
      yield $row;
    }
  }

I didn't need to wrap it in an iterator. Did you try that? What went wrong?

I think it could make your tests much simpler. The simpler the better :)

As far as I remember when I wrote this the bug is only reproducible when the migration depends on another migration which uses generators.

I will fix the points of #26 and try to make this simpler

Thanks for your time and review!

Yes, in my testing I was able to make migrationA use a generator (like the code I pasted above), and then migrationB depend on it, and that triggered the bug from the summary. So it would make a great test!

I didn't see any secondary issue with rewind().

Alright, I will start fresh here, I will move my end-to-end test to a follow up, and here I will just test the counts value.

So, I attach a reroll of #14. and in my next patch I will include the test.

Hi Vasi, you were right, I was overthinking this, This time I was able to write a much simpler patch, thanks for your help!

Tests and the patch.

I improved a little the comments and renamed the generator class.

OK, looks good to me...

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -417,4 +419,15 @@ protected function getCache() {
+  protected function doCount() {
+    $iterator = $this->getIterator();
+    return $iterator instanceof \Countable ? $iterator->count() : iterator_count($this->initializeIterator());

Just did a quick run through this again and refreshed myself why the fancy checking is necessary. It is because SourcePluginBase implements \Countable, therefore absolving any modules that extend from it to implement the count() method. But not all iterators are Countable, therefore the need to iterator_count() instead. A source plugin that yields would also be in that same camp.

+1 on RTBC.

Here a #34 reroll.

Noting here (as chx did in the other issue) this overlaps with #2598670: Source count caching does not work for SQL sources, except it names the new method doCount rather than computeCount. If this patch renamed SqlBase::count() to SqlBase::doCount() the other issue could be closed as a duplicate.

Committed and pushed ae1f599 to 8.3.x and 1d5f0f4 to 8.2.x. Thanks!

Committed to 8.2.x since this is a migrate patch and the module is still experimental.

diff --git a/core/modules/migrate/tests/src/Unit/MigrateSourceTest.php b/core/modules/migrate/tests/src/Unit/MigrateSourceTest.php
index 2d669c0..4cc91fd 100644
--- a/core/modules/migrate/tests/src/Unit/MigrateSourceTest.php
+++ b/core/modules/migrate/tests/src/Unit/MigrateSourceTest.php
@@ -532,4 +532,5 @@ protected function initializeIterator() {
       yield $row;
     }
   }
+
 }

Fixed on commit.

publish change record