PrestaShop icon indicating copy to clipboard operation
PrestaShop copied to clipboard

(Potential) Issue with setFieldsToUpdate() method

Open PrestaEdit opened this issue 1 year ago • 4 comments

Prerequisites

Describe the bug and add screenshots

Regarding the setFieldsToUpdate(), there is something that I don't test yet and that we need to verify cause I couldn't see it.

It's seems that once you have call this method, it's never emptied after an update or a save. So, if you want to re-use the same ObjectModel later with a full save, it will not save all fields.

Expected behavior

No response

Steps to reproduce

It's a technical issue.

PrestaShop version(s) where the bug happened

8.0.0

PHP version(s) where the bug happened

No response

If your bug is related to a module, specify its name and its version

No response

PrestaEdit avatar Sep 22 '22 11:09 PrestaEdit

Ping @kpodemski

PrestaEdit avatar Sep 22 '22 11:09 PrestaEdit

Hi @PrestaEdit

Thank for your report, This issue is too technical for me, ping @PrestaShop/prestashop-core-developers, can someone please reproduce this issue ?

Thanks

AureRita avatar Sep 22 '22 12:09 AureRita

@PrestaEdit We can add

$this->setFieldsToUpdate(null);

somewhere around here:

https://github.com/PrestaShop/PrestaShop/blob/434137b764ab0faa9de420a0a8ec54a039165dd0/classes/ObjectModel.php#L871

and it would be solved.

Hlavtox avatar Sep 22 '22 12:09 Hlavtox

@PrestaEdit We can add

$this->setFieldsToUpdate(null);

somewhere around here:

https://github.com/PrestaShop/PrestaShop/blob/434137b764ab0faa9de420a0a8ec54a039165dd0/classes/ObjectModel.php#L871

I agree, should be good :+1:

zuk3975 avatar Sep 22 '22 12:09 zuk3975

Thank you for the ping. We need to create a module so we can have an easy scenario to reproduce (if needed).

If there is an issue, I think we could agree on having a fix in the next patch.

kpodemski avatar Oct 14 '22 17:10 kpodemski

On the other hand, should we really reset the fields before the hook? Hook represents the action that was done, so in case the update() action was done with the "fieldsToUpdate" set, maybe its alright to leave them intact and let the module decide what to do with them (or decide what to do depending on them)? e.g. new product page has lots of "update" actions firing during single product update. Maybe module needs to know which was the action that updated the name or the reference etc. ?

Also, just a note - if you try to update() the same object in the hookActionObjectModelUpdateAfter, you will get into infinite loop (the update hook will fire over and over again), so maybe the issue is irrelevant in this case and we should indeed just reset the fields after the hook?

zuk3975 avatar Oct 16 '22 13:10 zuk3975