Problem/Motivation

Over in #2427335: Combine legacy Source class into SourcePluginBase we did some clean up for SourcePluginBase and @alexpott pointed out that the public getIterator() method is easily confused with IteratorAggregate.

It's also not something we particularly want people calling but right now the LoadEntity classes use the iterator directly.

Proposed resolution

  1. Renamed getIterator() to getRawIterator().
  2. Try and find a way to make them protected and still give the LoadEntity what they need.

Remaining tasks

Write patch, discuss

User interface changes

n/a

API changes

Possible that we remove getIterator() from the public API.

CommentFileSizeAuthor
#12 interdiff.txt968 bytesmikeryan
#12 rename-2452217-12.patch3.03 KBmikeryan
#10 rename-2452217-10.patch1.94 KBmikeryan
#6 2452217-6.patch4.72 KBsdstyles
#3 2452217-3.patch2.26 KBsdstyles

Comments

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented

With load plugins gone, we should just be able to make it protected now, right?

benjy’s picture

benjy commented
Issue tags: +Novice

Yes, I think we can, lets just try it.

sdstyles’s picture

sdstyles commented
Status: Active » Needs review
StatusFileSize
new 2452217-3.patch2.26 KB

Status: Needs review » Needs work

The last submitted patch, 3: 2452217-3.patch, failed testing.

The last submitted patch, 3: 2452217-3.patch, failed testing.

sdstyles’s picture

sdstyles commented
Status: Needs work » Needs review
StatusFileSize
new 2452217-6.patch4.72 KB

Renamed also method in interface and tests.

Status: Needs review » Needs work

The last submitted patch, 6: 2452217-6.patch, failed testing.

The last submitted patch, 6: 2452217-6.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented 
+++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
@@ -39,7 +39,7 @@ public function fields();
+  public function getRawIterator();

We don't need to do the getRawIterator() thing - all we need to do is make getIterator() protected and remove it from the interface.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs work » Needs review
StatusFileSize
new rename-2452217-10.patch1.94 KB

Status: Needs review » Needs work

The last submitted patch, 10: rename-2452217-10.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs work » Needs review
StatusFileSize
new rename-2452217-12.patch3.03 KB
new interdiff.txt968 bytes

TermNode builder was still using getIterator() for no good reason...

The last submitted patch, 10: rename-2452217-10.patch, failed testing.

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community

This is a good clean-up, I grepp'd core, couldn't find anymore usages.

Patch looks good to me if the bot agrees.

+++ b/core/modules/taxonomy/src/Plugin/migrate/builder/d6/TermNode.php
@@ -78,11 +78,10 @@ protected function getVocabularyIdMap(array $source) {
-    foreach ($migration->getSourcePlugin()->getIterator() as $source_row) {
...
+    foreach ($migration->getSourcePlugin() as $source_row) {

Ah, nice catch!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Looks good. I asked if we needed a change record, but this was only public for (I quote) "load plugin hacky nonsense" and so there's no reason any "real world" migrations would be calling this at all.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 643dc7a on 8.0.x 
    Issue #2452217 by mikeryan, sdstyles, benjy: Rename SourcePluginBase::...

Status: Fixed » Closed (fixed)

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