Problem/Motivation

I'm working on updating migrate_source_csv to run with >8.1.x. One of the first steps is to get my unit tests passing again. I made the simple change of swapping out the namespace on MigrationInterface. But now I find that in the constructor for SourcePluginBase, we call get(). Since get() isn't in the interface (we used to implement ConfigEntityBase, and it did then), that makes unit testing tough.

SourcePluginBase calls get() in its constructor and now my unit tests cannot Prophecy using the interface.

Proposed resolution

Based on a quick conversation in IRC with benjy, we should remove the get() calls in favor of specific getters.

Remaining tasks

Do it. Code it.

User interface changes

API changes

Adds specific getters to MigrationInterface and Migration plugin. Marks get() on Migration as deprecated as of 8.2.x.

public function getDestinationConfiguration();

public function getSourceConfiguration();

public function getHighWaterProperty();

public function getTrackLastImported();

public function getDestinationIds();

Data model changes

CommentFileSizeAuthor
#12 interdiff_9-12.txt1.38 KBheddn
#12 drupal-migrationinterface_getters-2694009-12.patch14.14 KBheddn
#9 interdiff_7-9.txt624 bytesheddn
#9 drupal-migrationinterface_getters-2694009-9.patch13.54 KBheddn
#7 interdiff_5-7.txt1.64 KBheddn
#7 drupal-migrationinterface_getters-2694009-7.patch13.54 KBheddn
#5 drupal-migrationinterface_getters-2694009-4.patch12.73 KBheddn
#3 drupal-migrationinterface_getters-2694009-3.patch1.91 KBheddn

Comments

heddn created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Issue summary: View changes
heddn’s picture

heddn
Primary language English
Location Illinois
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new drupal-migrationinterface_getters-2694009-3.patch1.91 KB

Docs to come later. Here's a first cut at the interface update.

benjy’s picture

benjy commented

I suggested we deprecate get() since we inherited it previously from ConfigEntityBase and it breaks the encapsulation of the Migration class. So, +1 from me.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
StatusFileSize
new drupal-migrationinterface_getters-2694009-4.patch12.73 KB

And here's updates in all the locations, including docs.

Status: Needs review » Needs work

The last submitted patch, 5: drupal-migrationinterface_getters-2694009-4.patch, failed testing.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-migrationinterface_getters-2694009-7.patch13.54 KB
new interdiff_5-7.txt1.64 KB
vasi’s picture

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

Looks quite straightforward.

I assume this will get backported to 8.1 though, so maybe change the "@deprecated in Drupal 8.2.x" comment to say 8.1.x instead.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new drupal-migrationinterface_getters-2694009-9.patch13.54 KB
new interdiff_7-9.txt624 bytes
vasi’s picture

vasi commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/migrate/src/Plugin/MigrationInterface.php
@@ -332,4 +332,45 @@ public function setTrackLastImported($track_last_imported);
+  /**
+   * Get information on the high water mark.
+   * @return array
+   */
+  public function getHighWaterProperty();

This needs to document the @return value and probably describe the keys and values of this array.

Mind you

  /**
   * Information on the high water mark.
   *
   * @var array
   */
  protected $highWaterProperty;

Is not that helpful either. So we could just punt the detail to a followup - we still need to fix the @return though.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-migrationinterface_getters-2694009-12.patch14.14 KB
new interdiff_9-12.txt1.38 KB
benjy’s picture

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

Thanks

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 17bb1cd and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 47febbf on 8.2.x 
    Issue #2694009 by heddn: MigrationInterface doesn't include get()

Status: Fixed » Closed (fixed)

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

codebymikey’s picture

codebymikey commented

The deprecation of this method removes access to the $requirements property which is used by contrib modules such as migrate_tools.

Another property which doesn't have an equivalent getter is $dependencies.

Is there any upgrade plans for them in Drupal 9?