The migrate module is still considered an experimental module. As a result, its code and tests have been reviewed less than other more mature code. Attach are changes noted during a review of some of the files in core/modules/migrate/src/Plugin/*.php. These changes include adding docblocks, missing @param/@var/@return descriptions and a few coding style changes.

CommentFileSizeAuthor
#10 2624660-10-migrate-module-src-plugin-d8.patch36.01 KBlars toomre
#10 interdiff-2624660-10-7.txt908 byteslars toomre
#7 2624660-7-migrate-module-src-plugin-d8.patch35.96 KBlars toomre
#2 2624660-2-migrate-module-src-plugin-d8.patch36.79 KBlars toomre

Comments

Lars Toomre created an issue. See original summary.

lars toomre’s picture

lars toomre commented
Status: Active » Needs review
StatusFileSize
new 2624660-2-migrate-module-src-plugin-d8.patch36.79 KB

Here is an initial patch covering nineteen files. There are approximately another fifteen files in modules/migrate/scr/Plugin/* that PHP_CodeSniffer suggests have one or more documentation and/or coding standards shortfalls. To keep the patch sizes manageable, those will be added in another issue.

lars toomre’s picture

lars toomre commented
Related issues: +#2624888: More fixes to migrate/src/Plugin/*.php files

The fixes to the files in modules/migrate/scr/Plugin/build/*.php and modules/migrate/scr/Plugin/destination/*.php have been added in the related issue #2624888: More fixes to migrate/src/Plugin/*.php files.

lars toomre’s picture

lars toomre commented
Related issues: +#2624914: Fixes to migrate/src/Tests/*.php files

Added a patch to clean up migrate/src/Tests/*.php files in #2624914: Fixes to migrate/src/Tests/*.php files.

benjy’s picture

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

Changes look good.

+++ b/core/modules/migrate/src/Plugin/MigrateDestinationInterface.php
@@ -40,17 +42,17 @@ public function getIds();
+   * @todo Review the cases where we need the Migration parameter, can we avoid

This should have an issue but I guess it's an outstanding problem.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2624660-2-migrate-module-src-plugin-d8.patch, failed testing.

lars toomre’s picture

lars toomre commented
Status: Needs work » Needs review
StatusFileSize
new 2624660-7-migrate-module-src-plugin-d8.patch35.96 KB

Here is a re-rolled after the IDs patch was committed.

quietone’s picture

quietone commented

Perhaps the @todo could reference, #2543568: Remove the md_entity destination plugin hack or the Meta Issue, #2543536: [meta] Reduce/remove tight coupling of migration classes ?

quietone’s picture

quietone commented
Status: Needs review » Needs work
lars toomre’s picture

lars toomre commented
Status: Needs work » Needs review
Related issues: +#2543568: Remove the md_entity destination plugin hack
StatusFileSize
new interdiff-2624660-10-7.txt908 bytes
new 2624660-10-migrate-module-src-plugin-d8.patch36.01 KB

Here is an interdiff and revised patch including a reference to #2543568: Remove the md_entity destination plugin hack in the @todo directive.

quietone’s picture

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

benjy reviewed this patch, #5, and his only question was if an issue should be added to the @todo. That is now done, so setting to RTBC.

alexpott’s picture

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

Committed 7b175e0 and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed 5421fde on 8.1.x 
    Issue #2624660 by Lars Toomre: Some fixes to migrate/src/Plugin/*.php...

Status: Fixed » Closed (fixed)

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