Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Line item quantity validation fix #855

Merged
merged 1 commit into from
May 10, 2019

Conversation

VitaliiTsilnyk
Copy link
Contributor

Fixes a couple of problems with order line item quantity validation.

First of all, the $qty variable is being calculated only one time per LineItem object instance because Yii creates all validators when you request them the first time and then reuses them inside the same model object for all subsequent validations. And although the quantity validation functions will be called each time the validation runs, the actual quantities of purchasables will not be updated, they will be stored in the closure and you will always validate the first set of purchasable quantities each time you run a validation. I understand that this calculation had been put outside of the validator to optimize and minimize the code, but it will fail in case when you:

  1. Validate the order with correct quantity.
  2. Change the quantity of any line item.
  3. Re-validate the order. And no matter what quantity you have in your LineItem object right now, validation will always pass, as long as it passed for the first time.

Also, I believe the current version of the code contains a typo:

// count new line items
if ($lineItem->id === null) {

I think there should be the $item variable used instead of $lineItem. Because in case the current line item is new, only it's quantity will be used, ignoring all other line items in the order with the same purchasableId.

My fix encloses the quantity calculation into a closure, allowing to re-calculate the quantities each time properly while keeping the code minimal. And although I had to get rid of the optimization, I don't think it will have any noticeable impact on the performance.

@lukeholder
Copy link
Member

This is a great PR, thanks for the detailed explanation also. We appreciate it! Merging shortly.

@lukeholder lukeholder merged commit 1773e3e into craftcms:develop May 10, 2019
@lukeholder
Copy link
Member

Changelog item: efe51a8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants