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

Product->save() shouldn't be called directly #8498

Merged
merged 1 commit into from
Feb 18, 2017

Conversation

stansm
Copy link
Contributor

@stansm stansm commented Feb 9, 2017

Here is a problem with calling Product->save() directly in ProductWebsiteLinkRepository:

When product has custom options it should have 'has_options' and possibly 'required_options' set to true but calling Product->save() without $product->setCanSaveCustomOptions(true); will result in both 'has_options' and 'required_options' to be set to false.

Also calling $product->save() won't invalidate internal ProductRepository's cache (instances , instancesById) and this would produces other unexpected issues.

Here is a problem with calling Product->save() directly:

When product has custom options it should have 'has_options' and possibly 'required_options' set to true but calling Product->save() without $product->setCanSaveCustomOptions(true); will result in both 'has_options' and 'required_options' to be set to false.

Also calling $product->save() won't invalidate internal ProductRepository's cache (instances , instancesById) and this would produces other unexpected issues.
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Feb 9, 2017

CLA assistant check
All committers have signed the CLA.

@vrann vrann self-requested a review February 9, 2017 22:50
@vrann vrann self-assigned this Feb 9, 2017
@okorshenko okorshenko added this to the February 2017 milestone Feb 11, 2017
@vrann
Copy link
Contributor

vrann commented Feb 15, 2017

@stansm Thanks you, change looks good.
Can you please write integration test which exposes the issue?

@vrann vrann added the bugfix label Feb 15, 2017
@stansm
Copy link
Contributor Author

stansm commented Feb 15, 2017

@vrann ok, I'll do it this weekend

@vrann
Copy link
Contributor

vrann commented Feb 15, 2017

@stansm please verify this bug report. Is that what you had in mind?

Steps to Reproduce

  1. Create visible product in default store and assign it to category. SKU: "test".
  2. Add custom option to the product, i.e. type text field, SKU test-test, proce $10
  3. Navigate to the frontpage of default store to the category, click on the product. Observe field to enter value of the custom option for the proce $10
  4. Create website#2 with the store and store view
  5. Invoke Web API POST /V1/products/:sku/websites per this schema http://devdocs.magento.com/swagger/
{
  "productWebsiteLink": {
    "sku": "test",
    "website_id": 2
  }
}
  1. Navigate to the product page in either website

Expected result:
product is displayed with the custom option

Actual result:
custom option is missing on the product page

@vrann
Copy link
Contributor

vrann commented Feb 15, 2017

@stansm please verify steps and I'll merge pull request upon manual verification. You are welcome to add tests as a separate pull request though!

@vrann vrann added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Progress: accept labels Feb 15, 2017
@stansm
Copy link
Contributor Author

stansm commented Feb 15, 2017

@vrann Instead of 4 I've just had a code like:

$siteLink = $productWebsiteLink->create();
$siteLink->setSku($product->getSku());
$siteLink->setWebsiteId(2);
$productWebsiteLinkRepository->save($siteLink);`

and after it executed 'has_options' and 'required_options' were set to false on this product (I've traced it and that's how it should really be according to the logic in the Product::beforeSave). And even that 'has_options' and 'required_options' were gone, custom options were still showing on a product page but were missing when added to the cart. This is what I know for now..

@vrann
Copy link
Contributor

vrann commented Feb 15, 2017

@stansm thank you, that is probably same the flow but from the code perspective. I reproduced the issue using the steps to reproduce.

@mmansoor-magento mmansoor-magento merged commit 8227c72 into magento:develop Feb 18, 2017
magento-devops-reposync-svc pushed a commit that referenced this pull request Sep 1, 2023
…_25082023

Hammer platform health scope 25082023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Award: advanced bugfix Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Progress: accept
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants