Problem/Motivation

Source::next() was copied from D7 and the logic is very convoluted and very hard understand. Probably as a consequence, it also has a logic mistake where a ! snuck in but since the whole class is untested this was not caught. Probably it was untested because it was so poorly understood by all. Other parts of Source also more look like contrib code than D8 core.

Proposed resolution

Simplify Source::next() (really big succeeded big time) and then move the simplified code into the SourcePluginBase class and remove the Source class. Also, add tests so that we do not break this again.

Remaining tasks

User interface changes

API changes

There are some but migrate is not frozen. Note that the main entry point, MigrateExecutable::import did not change. Using Source directly was never a good idea; it was poorly documented; poorly understood and I can't imagine anyone using it (especially since it was quite broken).

CommentFileSizeAuthor
#68 interdiff.txt4.26 KBbenjy
#68 2427335-68.patch54.19 KBbenjy
#66 2427335-66.patch52.79 KBbenjy
#66 interdiff.txt2.97 KBbenjy
#61 interdiff.txt2.54 KBbenjy
#61 2427335-62.patch52.38 KBbenjy
#55 interdiff.txt12.94 KBbenjy
#55 2427335-55.patch51.72 KBbenjy
#52 interdiff.txt575 bytesbenjy
#52 2427335-52.patch40.58 KBbenjy
#50 interdiff.txt2.26 KBbenjy
#50 2427335-51.patch40.86 KBbenjy
#42 2427335-42.patch38.78 KBbenjy
#40 2427335-35.patch38.78 KBbenjy
#35 interdiff.txt1.47 KBbenjy
#35 2427335-35.patch38.78 KBbenjy
#32 interdiff.txt1.11 KBbenjy
#32 2427335-32.patch37.3 KBbenjy
#31 interdiff.txt1.22 KBbenjy
#31 2427335-31.patch37.2 KBbenjy
#28 2427335-28-1.patch35.98 KBbenjy
#23 interdiff.txt4.39 KBbenjy
#23 2427335-23.patch18.93 KBbenjy
#22 interdiff.txt11.05 KBbenjy
#22 2427335-20.patch20.04 KBbenjy
#19 interdiff.txt519 bytesbenjy
#19 2427335-19.patch16.14 KBbenjy
#18 interdiff.txt1.4 KBbenjy
#17 interdiff.txt4.35 KBbenjy
#17 2427335-15.patch16.24 KBbenjy
#16 interdiff.txt2.67 KBbenjy
#16 2427335-14.patch15.09 KBbenjy
#14 interdiff.txt657 bytesbenjy
#14 2427335-13.patch14.73 KBbenjy
#12 interdiff.txt1.33 KBbenjy
#12 2427335-12.patch14.29 KBbenjy
#11 interdiff.txt5.15 KBbenjy
#11 2427335-11.patch14.37 KBbenjy
#10 interdiff.txt3.41 KBchx
#10 2427335_10.patch9.69 KBchx
#9 interdiff.txt1.47 KBchx
#9 2427335_9.patch10.95 KBchx
#8 interdiff.txt2.44 KBbenjy
#6 2427335_5.patch10.74 KBchx
#4 2427335_4.patch10.76 KBchx
#3 interdiff.txt5.83 KBdawehner
#3 2427335-3.patch8.66 KBdawehner
#2 2427335_2.patch5.38 KBchx
#1 2427335_1.patch5.42 KBchx
next.patch5.03 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

FileSize
5.42 KB

Small tweak.

chx’s picture

Issue summary: View changes
FileSize
5.38 KB

Slightly reworded comments.

dawehner’s picture

FileSize
8.66 KB
5.83 KB

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

chx’s picture

FileSize
10.76 KB

Sorry no interdiff there's very little that is unchanged.

chx’s picture

Issue summary: View changes
chx’s picture

FileSize
10.74 KB

$this->currentRow = NULL; is set already in the beginning.

benjy’s picture

+++ 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.

benjy’s picture

FileSize
2.44 KB

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

chx’s picture

FileSize
10.95 KB
1.47 KB
chx’s picture

FileSize
9.69 KB
3.41 KB

Scratch that. Well, what, even the id can be changed in prepare so I do not see a good way to sometimes skip it so don't skip it. This makes the double-call check on prepareRow unnecessary.

benjy’s picture

FileSize
14.37 KB
5.15 KB

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.

benjy’s picture

FileSize
14.29 KB
1.33 KB

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?

The last submitted patch, 11: 2427335-11.patch, failed testing.

benjy’s picture

FileSize
14.73 KB
657 bytes

Fix namespace.

The last submitted patch, 12: 2427335-12.patch, failed testing.

benjy’s picture

FileSize
15.09 KB
2.67 KB

More refinements.

benjy’s picture

FileSize
16.24 KB
4.35 KB

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

benjy’s picture

FileSize
1.4 KB

Correct interdiff for 17

benjy’s picture

FileSize
16.14 KB
519 bytes

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

The last submitted patch, 17: 2427335-15.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 19: 2427335-19.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
20.04 KB
11.05 KB

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.

benjy’s picture

FileSize
18.93 KB
4.39 KB

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

benjy’s picture

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

chx’s picture

Title: Source::next highwater support is broken » Clean up Source.php
Issue summary: View changes
chx’s picture

Issue summary: View changes
dawehner’s picture

Title: Clean up Source.php » Source::next highwater support is broken
Issue summary: View changes

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...

benjy’s picture

FileSize
35.98 KB
  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. Expanded docs.
  5. 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.

benjy’s picture

Title: Source::next highwater support is broken » Combine legacy Source class into SourcePluginBase

Status: Needs review » Needs work

The last submitted patch, 28: 2427335-28-1.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
37.2 KB
1.22 KB

Just fixed the unit tests for now.

benjy’s picture

FileSize
37.3 KB
1.11 KB

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

chx’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 32: 2427335-32.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
38.78 KB
1.47 KB

Fix for the last unit test.

The last submitted patch, 31: 2427335-31.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 35: 2427335-35.patch, failed testing.

benjy queued 35: 2427335-35.patch for re-testing.

The last submitted patch, 35: 2427335-35.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
38.78 KB

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

Status: Needs review » Needs work

The last submitted patch, 40: 2427335-35.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
38.78 KB

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

Status: Needs review » Needs work

The last submitted patch, 42: 2427335-42.patch, failed testing.

Status: Needs work » Needs review

isntall queued 42: 2427335-42.patch for re-testing.

isntall queued 40: 2427335-35.patch for re-testing.

isntall queued 35: 2427335-35.patch for re-testing.

The last submitted patch, 40: 2427335-35.patch, failed testing.

The last submitted patch, 35: 2427335-35.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 42: 2427335-42.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
40.86 KB
2.26 KB

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().

Status: Needs review » Needs work

The last submitted patch, 50: 2427335-51.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
40.58 KB
575 bytes

Removed runQuery from the interface for now.

webchick’s picture

Priority: Normal » Major

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."

webchick’s picture

Issue tags: +blocker

Oh, and adding the blocker tag.

benjy’s picture

FileSize
51.72 KB
12.94 KB

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.

PaponSifat.Com queued 1: 2427335_1.patch for re-testing.

PaponSifat.Com queued 52: 2427335-52.patch for re-testing.

PaponSifat.Com queued 2: 2427335_2.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 55: 2427335-55.patch, failed testing.

The last submitted patch, 52: 2427335-52.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
52.38 KB
2.54 KB

Some testing changes.

chx’s picture

I like where this is now but I can't RTBC it because it contains quite a bit of my own code.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

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

chx’s picture

I think the confusion arises from getIterator being public. No need for that. Perhaps we could rename it to something like getRawIterator or getUnfilteredIterator -- after all , the iterator returns all rows and the rest of the class -- next() mostly -- filters them.

benjy’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.97 KB
52.79 KB

@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.

Status: Needs review » Needs work

The last submitted patch, 66: 2427335-66.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
54.19 KB
4.26 KB

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.

Status: Needs review » Needs work

The last submitted patch, 68: 2427335-68.patch, failed testing.

benjy’s picture

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

chx’s picture

Let's rename and postpone the protected part to another issue.

benjy’s picture

Status: Needs work » Reviewed & tested by the community

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

chx’s picture

Maybe. Or we could do the renaming here but that's not a big deal , really.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 260f936 on 8.0.x
    Issue #2427335 by benjy, chx, dawehner: Combine legacy Source class into...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.