Problem/Motivation

It looks like we have all the right composer.json dependencies setup, more or less. We could probably make the version constraint less strict and move migrate_plus into the required section. Why? Because it looks like we now have some event subscribers in this base module that depend on migrate_plus. And we should add migrate_plus to .info.yml file as well.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#17 interdiff.txt352 bytesquietone
#17 2918496-17.patch3.63 KBquietone
#14 interdiff-12-14.txt482 bytesquietone
#14 2918496-14.patch3.98 KBquietone
#12 migrate_plus-n2918496-12.interdiff.txt777 bytesdamienmckenna
#12 migrate_plus-n2918496-12.patch4 KBdamienmckenna
#11 migrate_plus-n2918496-11.patch0 bytesdamienmckenna
#11 migrate_plus-n2918496-11.interdiff.txt0 bytesdamienmckenna
#9 2918496-8.patch3.64 KBquietone
#6 2918496-6.patch3.58 KBquietone

Comments

heddn created an issue. See original summary.

quietone’s picture

quietone commented
Related issues: +#2917891: Remove postRowSave event subscribers

Needs to be considered along with the idea of removing the event subscribers.

quietone’s picture

quietone commented
Status: Active » Needs review

heddn, Is this still relevant?

heddn’s picture

heddn
Primary language English
Location Illinois
commented

I'd have to check. Do we still have event subscribers?

quietone’s picture

quietone commented

There are two, MigrateOrder and MigrateProduct, both using onPreRowDelete for handling rollback.

http://cgit.drupalcode.org/commerce_migrate/tree/src/EventSubscriber

quietone’s picture

quietone commented
StatusFileSize
new 2918496-6.patch3.58 KB

How about a patch that addresses the dependencies of migrate_plus in all modules.

Gathered some information;
Migrate Plus process plugins are used in sub modules Commerce, Magento and Ubercart.
The PrepareRow event is used in sub modules Commerce and Ubercart.

And then made a patch.
I got a little carried away and also modified the composer.json for drupal/core.

This definitely needs a review by someone other than me!

Status: Needs review » Needs work

The last submitted patch, 6: 2918496-6.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 6: 2918496-6.patch, failed testing. View results

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2918496-8.patch3.64 KB

'Twas a stray comma

Status: Needs review » Needs work

The last submitted patch, 9: 2918496-8.patch, failed testing. View results

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs work » Needs review
StatusFileSize
new migrate_plus-n2918496-11.interdiff.txt0 bytes
new migrate_plus-n2918496-11.patch0 bytes

It might be useful to list migrate_plus in the composer.json file too.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
StatusFileSize
new migrate_plus-n2918496-12.patch4 KB
new migrate_plus-n2918496-12.interdiff.txt777 bytes

Duh Damien. Sorry.

The last submitted patch, 11: migrate_plus-n2918496-11.patch, failed testing. View results

quietone’s picture

quietone commented
StatusFileSize
new 2918496-14.patch3.98 KB
new interdiff-12-14.txt482 bytes

@DamienMcKenna, thanks! 

+++ b/modules/magento/commerce_migrate_magento.info.yml
@@ -7,3 +7,4 @@ dependencies:
\ No newline at end of file

Need a new line.

Patch adding the new line.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs review » Needs work 
+++ b/composer.json
@@ -6,7 +6,7 @@
+        "drupal/migrate_plus": "^4.0"

+++ b/modules/magento/composer.json
@@ -5,7 +5,7 @@
+        "drupal/migrate_plus": "^4",

+++ b/modules/ubercart/composer.json
@@ -5,7 +5,6 @@
         "drupal/migrate_plus": "^4"

Should we standardize on ^4 or ^4.0? One position or 2? I think the difference is non-existent between the two when it comes to a point release of zero and the carrot. See https://getcomposer.org/doc/articles/versions.md.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

Because of the possibility for API changes, I'd suggest sticking to ^x.y rather than ^x.

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2918496-17.patch3.63 KB
new interdiff.txt352 bytes

After reading the composer docs linked to in #15 I think there is no practical difference between ^4 and ^4.0. So then why add it? We can always add x.y later if it is needed. Or have I got that wrong? I did make this patch using just "^4" but happy to change to "^4.0" if it makes a difference.

  • quietone committed 458bb1e on 8.x-2.x 
    Issue #2918496 by quietone, DamienMcKenna, heddn: commerce_migrate.info....
quietone’s picture

quietone commented
Status: Needs review » Fixed

Thanks all!

Status: Fixed » Closed (fixed)

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