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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone at Acro Commerce 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 CreditAttribution: quietone at Acro Commerce commented
Status: Active » Needs review

heddn, Is this still relevant?

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented

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

quietone’s picture

quietone CreditAttribution: quietone at Acro Commerce 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 CreditAttribution: quietone at Acro Commerce commented
FileSize
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 CreditAttribution: quietone at Acro Commerce commented
Status: Needs work » Needs review
FileSize
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 NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs work » Needs review
FileSize
migrate_plus-n2918496-11.interdiff.txt0 bytes
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 NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
FileSize
migrate_plus-n2918496-12.patch4 KB
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 CreditAttribution: quietone at Acro Commerce commented
FileSize
2918496-14.patch3.98 KB
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 Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce 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 NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

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

quietone’s picture

quietone CreditAttribution: quietone at Acro Commerce commented
Status: Needs work » Needs review
FileSize
2918496-17.patch3.63 KB
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 CreditAttribution: quietone at Acro Commerce commented
Status: Needs review » Fixed

Thanks all!

Status: Fixed » Closed (fixed)

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