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.

Sign up for GitHub

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

Jump to bottom

Correctly save Product Custom Option values #13569

Merged
merged 3 commits into from
Jul 14, 2018
Merged

Correctly save Product Custom Option values #13569

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d7f6baa
Correctly save Product Custom Option values
Feb 8, 2018
cb304db
Merge branch '2.2-develop' into patch-5
Apr 15, 2018
0b3fd22
Correctly save Product Custom Option values
May 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions app/code/Magento/Catalog/Model/Product/Option/Value.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,27 +190,29 @@ public function getProduct()
public function saveValues()
{
foreach ($this->getValues() as $value) {
$this->isDeleted(false);
$this->setData(
$optionValue = clone $this;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know exactly what values need to be set on this object to save correctly? I am thinking rather than cloning here we add a factory that can create the object as needed. We should also consider the cleanup of these objects once we are done with them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That depends on what type of custom option you added I think. So no, don't think you have a predefined list of values that need to be set unless we have factories for each option type?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JeroenVanLeusden do you think you could update the code to use a factory rather than a clone and see if this covers all the cases?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure do, hopefully have some time by the end of this week.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmanners Code underneath makes saving stop altogether. Am I missing something?

See that a Magento\Catalog\Model\Product\Option\Value\Repository is not present, might be an idea to add that or include the saving of custom options in Magento\Catalog\Model\Product\Option\Repository?

foreach ($this->getValues() as $value) {
    $option = $this->getOption();
    $data = array_merge($value, [
        'option_id' => $option->getId(),
        'store_id' => $option->getStoreId()
    ]);
    $optionValue = $this->customOptionValueFactory->create(['data' => $data]);

    if ((bool) $optionValue->getData('is_delete') === true) {
        if ($optionValue->getId()) {
            $optionValue->deleteValues($optionValue->getId());
            $optionValue->delete();
        }
    } else {
        $optionValue->save();
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @JeroenVanLeusden I would be tempted to add the repository for just working with the option values. Though https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Catalog/Model/Product/Option/Repository.php#L146 does deal with saving of option values.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmanners What are you proposing? Don't know what the preferred way is to get this done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @JeroenVanLeusden sorry for the delay. I would say your suggestion of adding the saving of custom options in Magento\Catalog\Model\Product\Option\Repository would be a good option for this.

$optionValue->isDeleted(false);

$optionValue->setData(
$value
)->setData(
'option_id',
$this->getOption()->getId()
$optionValue->getOption()->getId()
)->setData(
'store_id',
$this->getOption()->getStoreId()
$optionValue->getOption()->getStoreId()
);

if ($this->getData('is_delete') == '1') {
if ($this->getId()) {
$this->deleteValues($this->getId());
$this->delete();
if ($optionValue->getData('is_delete') == '1') {
if ($optionValue->getId()) {
$optionValue->deleteValues($optionValue->getId());
$optionValue->delete();
}
} else {
$this->save();
$optionValue->save();
}
}
//eof foreach()

return $this;
}

Expand Down