Problem/Motivation

We have some data where amount does not get split properly so $amount is an integer.

Error: Call to a member function getNumber() on integer in commerce_migrate/modules/commerce/src/Plugin/migrate/process/commerce1/OrderItemDiscountAdjustment.php on line 235 #0 commerce_migrate/modules/commerce/src/Plugin/migrate/process/commerce1/OrderItemDiscountAdjustment.php(156): Drupal\commerce_migrate_commerce\Plugin\migrate\process\commerce1\OrderItemDiscountAdjustment->getAdjustment(Array, Object(Drupal\migrate_tools\MigrateExecutable), Object(Drupal\migrate\Row))

Attached patch throws an exception if method isn't found.

Proposed resolution

Add a dependency on commerce_price, which has the getNumber method.
Check the value of the returned value from $this->spilt before processing it.
Add tests!

CommentFileSizeAuthor
#22 3073150-12.patch19.06 KBquietone
#14 commerce_migrate-n3073150-14.patch25.69 KBdamienmckenna
#12 3073150-12.patch19.06 KBquietone
#12 diff-6-12.txt13.97 KBquietone
#6 interdiff-5-6.txt13.31 KBquietone
#6 3073150-6.patch29.81 KBquietone
#5 3073150-5.patch20.75 KBquietone
#2 3073150-2-error-on-int.patch1.2 KBgrahl

Comments

grahl created an issue. See original summary.

grahl’s picture

grahl
he/him
Location Zürich
commented
Status: Active » Needs review
StatusFileSize
new 3073150-2-error-on-int.patch1.2 KB
brightbold’s picture

brightbold
she/her
Primary language English
Location Boston, MA
commented

Applying the patch in #2 to RC1 resulted in the offending order item being ignored and the following message was logged: "Invalid amount extraction for line item '44950'." Without the patch, drush migrate-import vomited up the above error and accompanying callstack and terminated abnormally. Thanks for solving that, @grahl!

From a functional standpoint, this patch behaved as promised. But I can't mark as RTBC since it's now failing tests, so it probably needs to be rerolled.

quietone’s picture

quietone commented
Status: Needs review » Needs work

getNumber is provided by commerce_price. So this will be a dependency issue.

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 3073150-5.patch20.75 KB

There is also the problem that the returned value from $this->spilt is not checked to determine if it is empty before using it. For that just an "if ($amount)" will fix it.

Not sure why checking if the getNumber method exists solves the problem in the IS. The error shows that getNumber is indeed being called but with an integer and not a string.

This patches adds a test for the commerce1 orderitemadjustment process plugin and applies fixes to that plugin and similar to the ubercart version. And adds a dependency on commerce_price.

quietone’s picture

quietone commented
StatusFileSize
new 3073150-6.patch29.81 KB
new interdiff-5-6.txt13.31 KB

Adding more testing.

quietone’s picture

quietone commented
Issue summary: View changes
quietone’s picture

quietone commented

Self review, I don't see anything to prevent this from going in. That assumes the tests are robust and I haven't missed a case.

quietone’s picture

quietone commented

Retested

quietone’s picture

quietone commented

Retesting

quietone’s picture

quietone commented

Is anyone using the latest patch here? Having any problems with it?

quietone’s picture

quietone commented
StatusFileSize
new diff-6-12.txt13.97 KB
new 3073150-12.patch19.06 KB

Needs a reroll. Some of the changes here sneaked in in #3178923: Add OrderItemAdjustment test process plugin. Removing changes here that are for Ubercart - that should be in a separate issue.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

This conflicts with #3150733: Drupal 9 compatibility fixes for Commerce Migrate.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
StatusFileSize
new commerce_migrate-n3073150-14.patch25.69 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 14: commerce_migrate-n3073150-14.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

quietone commented
Status: Needs work » Needs review

I am not sure about that reroll. The patch in #12 still applies and I just re-ran the tests and they pass.

Back to NR for the patch in #12.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

Yeah, let's skip #14, not sure what I did there :-\

quietone’s picture

quietone commented

Is anyone using the patch in #12? If so, what were the results?

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Issue summary: View changes

I'm using #12 but am having other problems with my migration and can't get to the point where I can test it.

tonytheferg’s picture

tonytheferg commented

I have had #14 applied for sometime, and it seems to be working. Would you like me to try #12 instead to review?

14 breaks tax and discount migrations from what I have seen as it removes the adjustment array. #12 is preferred

quietone’s picture

quietone commented
Version: 8.x-2.x-dev » 3.2.x-dev
quietone’s picture

quietone commented
StatusFileSize
new 3073150-12.patch19.06 KB

Re-uploading the patch from #12 to rerun tests.

quietone’s picture

quietone commented

Any reason not to commit this?

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs review » Reviewed & tested by the community

LGTM.

  • quietone committed a1097a0 on 3.2.x 
    Issue #3073150 by quietone, DamienMcKenna, grahl: Exception on...
quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Fixed

@DamienMcKenna, thanks!

Status: Fixed » Closed (fixed)

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