MigrateTemplateStorage should have an interface, contrib code should not type hint against the implementation because as per DIP. This in turn prevents the service from being replace in the container.

CommentFileSizeAuthor
#10 interdiff.txt2.06 KBbenjy
#10 2626472-10.patch5.01 KBbenjy
#3 2626472-3.patch2.95 KBbenjy
#2 2626472-2.patch2.95 KBbenjy
template-storage-interface.patch1.71 KBbenjy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

benjy created an issue. See original summary.

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented
FileSize
2626472-2.patch2.95 KB

This time with the interface.

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented
FileSize
2626472-3.patch2.95 KB

Fixed a comment.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/migrate/src/MigrateTemplateStorageInterface.php
@@ -0,0 +1,45 @@
+   * @return array
+   *   Array of parsed templates, keyed by the fully-qualified id.

Nit: 'id' should be 'ID'. Can be fixed on commit.

RTBC otherwise, pending Drupal CI.

The last submitted patch, template-storage-interface.patch, failed testing.

The last submitted patch, template-storage-interface.patch, failed testing.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Version: 8.0.x-dev » 8.1.x-dev

This is needed, but it is an API improvement so it goes to 8.1.x.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: +Needs framework manager review

Migrate is experimental so this may potentially go into 8.0.x. A committer should confirm that, though.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs framework manager review

This seems fine to go into 8.0.x - it does not break exiting contrib migrations and other migrate projects are helped by having an interface.

However we need to fix Drupal\taxonomy\Plugin\migrate\builder\d6\TermNode to use the interface

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented
Version: 8.1.x-dev » 8.0.x-dev
Status: Needs work » Needs review
FileSize
2626472-10.patch5.01 KB
interdiff.txt2.06 KB

Done.

chx’s picture

chx CreditAttribution: chx at Smartsheet commented
Status: Needs review » Reviewed & tested by the community

Straightforward.

alexpott’s picture

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

Committed ef59d9b and pushed to 8.0.x and 8.1.x. Thanks! This is eligible for 8.0.x because migrate is experimental and this does not wildly break the existing functionality.

  • alexpott committed 4706f54 on 8.1.x 
    Issue #2626472 by benjy: MigrateTemplateStorage should have an interface

Status: Fixed » Closed (fixed)

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