Conversation
b27fb19 to
e399996
Compare
e399996 to
214596b
Compare
src/Model.php
Outdated
| public function duplicate($newId = null) | ||
| { | ||
| $this->setId(null); | ||
| $this->entityId = null; |
There was a problem hiding this comment.
Entity approach must satisty several axioms. One of them is to never allow to assing different ID to the same instance.
So I think we should remove duplicate completely or clone before at the beginning of this method and return the cloned model then.
There was a problem hiding this comment.
Removal of method is not good option as functionality is useful.
Cloning model and returning it is a good option but $entityId is private. how to clear it best?
There was a problem hiding this comment.
I came up with following code but it does not work as even if model unloaded $entityId value remains saved. I think this is not the correct behavior
public function duplicate($newId = null): self
{
$duplicate = clone $this;
$duplicate->unload();
$duplicate->setMulti($this->data);
if ($duplicate->id_field) {
$duplicate->setId($newId);
}
return $duplicate;
}
There was a problem hiding this comment.
simply set entityId to null after clone, + then set whateer needed (unload, reset dirty, do your research)
There was a problem hiding this comment.
I can make it work but code is not consistent.
The problem I see is that clearing entityId should be done with unload. What is unload doing otherwise?
There was a problem hiding this comment.
clone should not normally do these things
as a design rule, otherwise there is not a real issues
but to satisfy standards as well, we should do that in special method
now in duplciate() :)
There was a problem hiding this comment.
I am still of the opinion that if we support unload then entityId should be cleared. All other values are cleared.
There was a problem hiding this comment.
You got me for 1, 2 seconds. The answer is clear no.
Entity approach must satisty several axioms. One of them is to never allow to assing different ID to the same instance.
So I think we should remove duplicate completely or clone before at the beginning of this method and return the cloned model then.
because if we allow to unset entityId, it can be easily changed, and this axiom will be violated. This any code that clears entityId is dangerous (except you know what you are dooing or do that on a fresh clone)
There was a problem hiding this comment.
Then we should remove unload method as it is not working properly at the moment or just use it to return a fresh object without entity loaded.
There was a problem hiding this comment.
unload unloads data, but keep entity bound to a specific ID
I think there are usecases for it.
214596b to
cb764b3
Compare
DarkSide666
left a comment
There was a problem hiding this comment.
If it works like this (also clears entityId) then it's fine with me.
BUT I think that probably we have to finish refactoring this Model / Entity situation and then quite many internal things will change anyway. Currently this Model/Entity approach looks wrong and hard to use and understand.
04f7d9a to
bcdbdfa
Compare
bcdbdfa to
72d5309
Compare
mvorisek
left a comment
There was a problem hiding this comment.
main issues (cloning, no $newId) resolved
No description provided.