Problem/Motivation

Core Migration Source classes cannot be easily extended because of the use of short hand database field notation on query conditionals. Causing Fatals because of ambiguous field names.

Proposed resolution

Go through each module and update the source classes to have the verbose naming convention of "alias.field" for conditional statements.

Original report

Essentially, To be able to extend the query on the D7 User source, I have to Rebuild the query because uid is used all over the place. I suggest we make the conditions explicit and use the alias in the field name so the query can be extended. You can see this in core/modules/user/src/Plugin/migrate/source/d7/User.php:22

Raised from IRC

mikeryan1 No need to duplicate the D7 user query, do $query=parent::query() and add the role filtering to it
generalredneck mikeryan1: doesnn't work
because you do uid ono the condition in the main class
not u.uid
and uid is in both users_roles and users table
mikeryan1 generalredneck: Ugh, that’s a bug in the d7_user source plugin then, it should specify u.uid so it’s extendable

CommentFileSizeAuthor
#5 migrate-add_verbose_naming_to_migrate_source_plugins-2737607-5-D8.1.x.patch12.45 KBgeneralredneck
#3 migrate-add_verbose_naming_to_migrate_source_plugins-2737607-3-D8.1.x.patch12.45 KBgeneralredneck

Comments

generalredneck created an issue. See original summary.

generalredneck’s picture

generalredneck
him/his
Primary language English
Location Texas, USA 🇺🇸
commented
Title: User D7 Source Plugin is not extendable » Migration Source Plugins are not extendable because of ambiguous database field names
Issue summary: View changes
generalredneck’s picture

generalredneck
him/his
Primary language English
Location Texas, USA 🇺🇸
commented
Status: Active » Needs review
StatusFileSize
new migrate-add_verbose_naming_to_migrate_source_plugins-2737607-3-D8.1.x.patch12.45 KB

Status: Needs review » Needs work

The last submitted patch, 3: migrate-add_verbose_naming_to_migrate_source_plugins-2737607-3-D8.1.x.patch, failed testing.

generalredneck’s picture

generalredneck
him/his
Primary language English
Location Texas, USA 🇺🇸
commented
Status: Needs work » Needs review
StatusFileSize
new migrate-add_verbose_naming_to_migrate_source_plugins-2737607-5-D8.1.x.patch12.45 KB

Fixed the Parse Error. Not enough dots.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs review » Reviewed & tested by the community

There are already tests that cover this. This doesn't have any side-effects so putting this into 8.1 seems fine. Marking RTBC after reviewing the code. Everything seems fine.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented 
+++ b/core/modules/book/src/Plugin/migrate/source/d6/Book.php
@@ -23,7 +23,7 @@ public function query() {
+      $query->orderBy('ml.'.$field);

Coding standard - should be spaces around the . operator (can be fixed on commit).

As for tests... Well, I'm OK with it as-is, but the case might be made for a test demonstrating the use case (extended a source plugin and being unable to do a join due to the unaliased reference). But, adding such a test for each instance covered here seems like way overkill....

alexpott’s picture

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

Committed 806f35e and pushed to 8.1.x and 8.2.x. Thanks!

Fixing coding standards on commit.

  • alexpott committed 55707ed on 8.2.x 
    Issue #2737607 by generalredneck: Migration Source Plugins are not...

Status: Fixed » Closed (fixed)

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