Fix duplicate relations on event listeners save #17408
Conversation
Review Checklist
|
|
|
@dpfaffenbauer any thoughts on this? |
|
looks like a follow up of #16596 , just linking it for code documentation purposes |
|
Hello @cuca24 , there is also a getter for the property you moved. What do you think about moving it to the abstract class too? |
|
Agree, I moved it to abstract class. |
|
@cuca24 Thank you! One last thing, could you please take a look at the merge conflict and tag me after it was resolved? Will merge it then. |
211ded6 to
ebf162b
Compare
…relations-on-event-liseners-save # Conflicts: # lib/Tool.php
|
Thanks @mcop1 done. |
|
Hey @mattamon Thanks So yes $this->getClassId() doesn't exist in AbstractObject it will be populated later in child Concrete class. @mattamon or anybody else i need your advice how to pull out classId. I could just make a protected variable classId in AbstractObject and use it, other methods as i can see is to store variable in Dao object and read it from there. What would be preferred pimcore's way of doing it. |
|
Is there any known workaround for now? 🤔 |
Hey @cuca24 sorry I was on vacation. For the static error to go away, we need to define the class in the abstract. WDYT? |
|
Hey @mattamon
You mean to add |
I just checked and yes you are right sorry. This is a conundrum that the DataObject is another layer inbetween the concret and the abstract. Either we would need to make it abstract or implement the getClassId in the DataObject. The first option is not possible and the second is not ideal. Let's go for the protected variable then and implement the function in the abstract please. |
|
@fashxp sorry, I did not see that you pinged me here. What confuses me, that the issue itself also happens on Pimcore 10, so pre delta calculations for relations. |
models/DataObject/AbstractObject.php
Outdated
|
|
||
| public function getClassId(): ?string | ||
| { | ||
| if (isset($this->classId)) { |
There was a problem hiding this comment.
@cuca24 thank you for moving the method and the property. I am thinking about also improving the method itself.
Since the setClassId has now a string parameter, we could remove the string cast from the getter. Also we do not need the isset anymore right? Since classId always exists and it returns null anyways if the setter is not called.
WDYT?
There was a problem hiding this comment.
@mattamon You are totally right. PR updated :)
…s://github.com/cuca24/pimcore into fix-duplicate-relations-on-event-liseners-save
Co-authored-by: Sebastian Blank <[email protected]>
Co-authored-by: Sebastian Blank <[email protected]>
|
Changes in this pull request
Resolves #
Duplicate relations on event listeners save
Additional info
Prerequisite
What happens is that if we have event listener pimcore.dataobject.postAdd or pimcore.dataobject.postUpdate and inside that function we have to call additional save in that case all relations inside of that object would get duplicate.
This happens because of the calculation of the delta determines that there is no records for relation id db, introduced here: 5a91072
Although there is noting wrong with the delta calculation itself, the problem is that state of relations has been temporary cached inside __rawRelationData. Now Concrete class itself has
$this->__rawRelationData = nullbut it happens too late, as event for post add/post update is dispatched earlier and therefore any additional calls to save inside listeners will duplicate relations.To correct this late call i moved
$this->__rawRelationData = nullinto parent class AbstractObject as well as full variable there. With this change there is no duplication of relations data.