When calling a migration's source plugin just to get the count (e.g. for a dashboard), you may end up with two instances of whatever initializeIterator() returns, because it's not an instance of Countable. This means you now have two iterators, one assigned to the $iterator property set by getIterator() and one immediately pulled through iterator_count().
The issue is that the iterator that was created by getIterator() and assigned to the property may have knock-on effects. For example, the CSV plugin from migrate_source_csv will open the CSV file upon iterator creation and keep it open until the iterator reaches the end of the file. Because the second iterator is used to actually run the count, the original stays in limbo.
But now you have a pointer to an iterator that will never be iterated, so the CSV file stays open until garbage collection runs and destroys the source plugin instance along with the iterator. However, garbage collection never runs before the end of the request because the source plugin points to the migration and the migration points to the source plugin.
So unless you manually call gc_collect_cycles(), the CSV file stays open. This can cause issues, for example, when trying to delete the file on some file systems. Linux allows this, but DDEV's private-files mount does not. So any test that creates CSV files to count will litter the disk with garbage over time because it can't delete them.
This is just one of many possible issues. We should really avoid instantiating an iterator that remains in memory unless we're actually going to use it.
Use the CSV plugin described above, try to create and count a dummy CSV file and then delete it. See that you can't.
Do not use the iterator meant for the migration, but always use a clean copy for counting.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 3633539-review-5.md | 1.66 KB | smustgrave |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
kristiaanvandeneyndeTried to be as resource-conservative as possible. So we only instantiate one if we really need to and keep the one we instantiated if we can.
Comment #4
kristiaanvandeneyndeSingle failure seems completely unrelated and goes green on my local.
Comment #5
smustgrave commentedHope you don't mind I've been trying to practice a "good" AI review example that could be shared in the community (currently getting ALOT of bad ones).
So I ran this through Claude Sonnet 4.6
Attaching the .md file as a comment would be spam.
The one part that made sense to me was the last line about test coverage "A unit test using a non-`Countable` iterator mock that asserts `$this->iterator` is null after `doCount()` (so the migration gets a fresh one) would make the fix verifiable."
Think something worth trying.
Comment #6
kristiaanvandeneyndeFYI: Discussed the above on Slack