Combine legacy Source class into SourcePluginBase

Note: This is "just" work on the docs.

+++ b/core/modules/migrate/src/Source.php
@@ -297,152 +311,110 @@ public function rewind() {
-      elseif (!empty($this->highWaterProperty['field'])) {
-        if ($this->trackChanges) {
-          if ($this->prepareRow($row) !== FALSE) {
-            if ($row->changed()) {
-              // This is a keeper

I actually got highwater to work without setting the field property and just bypassing this block entirely. The block that checks for the 'name' property on highwater seems to work fine.

+++ b/core/modules/migrate/src/Source.php
@@ -297,152 +311,110 @@ public function rewind() {
- if ($this->prepareRow($row) !== FALSE) {
- if ($row->getSourceProperty($this->highWaterProperty['name']) > $this->originalHighWater) {

This has removed a feature, eg. prepareRow now no longer runs before the access to the highwater property on row, this broke my tests because I was calculating the highwater in prepareRow.

I think from here we need to start fleshing out some unit tests because there seem to be a lot of outcomes in Source that currently exist without any coverage.

This interdiff fixed that issue for me. Basically lets prepareRow run earlier than anything else?

Some basic unit tests. Also note the change to Source, I don't think there is any need to check originalHighwater at all to see if we're using highwater. Lets just check for the existence of the property.

Removed the useHighwater() function.

Note, we need to add highwaterProperty to the schema as well. Not sure if we add camel cased properties to schema?

Fix namespace.

More refinements.

We no longer allow trackChanges and highwater together. We also now initialise them in the constructor.

Correct interdiff for 17

We now no longer need to check highwater in rowChanged. Will write some more unit tests for the new exception.

1. Some clean-up in the constructor, also moved it to the top of the file.
2. Additional tests for the highwater/trackchanges exception.
3. Moved t() into a method on Source and then override that with a TestSource for the tests.
4. Moved prepareRow() after the idList check.

Thanks to @chx for pointing out no translation is required for exception messages.

More fixes to count() and more unit tests to improve the overall coverage of the class. Screenshot shows the coverage for next().

Great improvements!

1. +++ b/core/modules/migrate/src/Source.php
   @@ -286,139 +310,99 @@ public function rewind() {
      public function next() {
   

   Sadly we lost the explanation of what highwater actually is. The one patch from me had some documentation around that area.

2. +++ b/core/modules/migrate/src/Source.php
   @@ -286,139 +310,99 @@ public function rewind() {
   +      if ($this->prepareRow($row) === FALSE) {
   

   do we really want to === FALSE or simply use if !? From a readability POV the later one seems much better.

3. +++ b/core/modules/migrate/src/Source.php
   @@ -286,139 +310,99 @@ public function rewind() {
   +      // 1. Explicitly specified IDs.
   +      // 2. This row has not been imported yet.
   +      // 3. Explicitly set to update.
   +      // 4. The row is newer than the current highwater mark.
   +      // 5. If no such property exists then try by checking the hash of the row.
   

   It would be really nice to add a bit of "why" to the documentation pointing out "what".

4. +++ b/core/modules/migrate/src/Source.php
   @@ -286,139 +310,99 @@ public function rewind() {
   +  protected function aboveHighwater(Row $row) {
   

   really nice method name!

5. +++ b/core/modules/migrate/src/Source.php
   @@ -286,139 +310,99 @@ public function rewind() {
   +   * Check if the row has changed.
   

   Can we make clear that it changed in the destination?

6. +++ b/core/modules/migrate/tests/src/Unit/MigrateSourceTest.php
   @@ -0,0 +1,199 @@
   + * @coversDefaultClass \Drupal\migrate\Source
   ...
   +class MigrateSourceTest extends MigrateTestCase {
   

   It would be cool if the Test would be named SourceTest, as this allows you to use https://www.jetbrains.com/phpstorm/help/navigating-between-test-and-test...

1. Brought back the description for highwater, sorry about that.
2. Open to this API change, I think this has always been the case since D7.
3. Not entirely sure what you're asking for here. Is it just more docs about this if statement?
4. √
5. Expanded docs.
6. See below description.

I've taken a different direction. Drupal\migrate\Source seems to be left over from Drupal 7. It's a class that we can iterate. That made sense in Drupal 7 because the source data was gathered up in your migration class, this is no longer the case. So, attached patch merges Source into SourcePluginBase.

Source only brought us rewind(), next(), count() and prepareRow(). prepareRow() did very little and then called SourcePluginBase::prepareRow(), count() is coming to the SourcePluginBase anyway, #2429097: Optionally cache migration source counts and that just leaves next() and rewind(). Moving them across gets rid of Source.php altogether.

Plus, nearly everything in the Source constructor was configuration passed on from the SourcePlugin.

TODO, The source plugin currently needs the executable because of the way Source was saving some map messages. I've left a couple of TODO's to fix that but want to get a test run.

Just fixed the unit tests for now.

I added a @see, was holding off for the tests to finish on the last patch but all the bots seem to be down.

Fix for the last unit test.

Just uploading the same patch again, not sure what the failures are meant to be.

Regenerated the same patch after a re-roll, not sure what is going on with the bot just terminating.

I tracked this down to a nasty little issue with the iterators. Now we've combined Source which was previously an iterator into SourcePluginBase the iterator wasn't been cached for EmptySource which in turn led to an infinite loop for migrate tests using the EmptySource. Ouch!

New patch changes EmptySource to implement runQuery instead which makes much more sense. I've also added runQuery to the interface to ensure that all sources implement this method. SqlBase provides a base implementation anyway.

SourcePluginBase currently provides a default implementation of getIterator() but i've opened [#] to remove this function altogether anyway, we should be able to cache these results in either runQuery or rewind().

Removed runQuery from the interface for now.

From talking to benjy today, it sounds like this is a blocker for the finalization of the Migrate API, which is in turn a blocker to the D7 -> D8 migration path. Marking as "major."

Oh, and adding the blocker tag.

The solution for runQuery was to rename to initializeIterator(). It didn't make much sense outside of Sql sources anyway, and this is in SourcePluginBase.

We don't want it on the public interface but we do need to enforce that it exists so I added an abstract method to SourcePluginBase.

Some testing changes.

I think this is a great improvement! Let's unblock stuff.

Might we not have some of the same issues solved in #2431379: LazyPluginCollection should not implement \Iterator - should we be using \IteratorAggregate instead of \Iterator

@alexpott, could question. I think we have a different use case but just confused things by having a public getIterator() method. We can't return a new iterator every time because that would send us into an infinite loop as shown in #50.

I discussed with @chx today and we decided to remove getIterator() from the public API and renamed to getRawIterator() to try make it clearer that we are not implementing a PHP API such as IteratorAggregate.

This is a WIP, just a suggestion. The problem we have is that the LoadEntity was using the raw iterator but it only needs the bundles. I've added a getBundles() method to SourcePluginBase but i'm thinking maybe a public method on either SourceEntityInterface or a new interface for sources that are "bundle migrations" for an entity type.

LoadTermNode also uses the raw iterator directly, can't see an easy way around that other than making it public again?

That would make #61 RTBC again then. Created #2452217: Rename SourcePluginBase::getIterator() and try make protected as a follow-up.

Migrate is not frozen in beta. Committed 260f936 and pushed to 8.0.x. Thanks!