From f180f4e3e2de597c1761ac8ac5abf16dd169d192 Mon Sep 17 00:00:00 2001 From: mhabibi Date: Sat, 19 Sep 2020 14:33:19 +0200 Subject: [PATCH 1/5] Issue673 Optimize removeElement method in doctrine entities --- src/Util/ClassSourceManipulator.php | 40 +++++++++++++++++------------ 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/src/Util/ClassSourceManipulator.php b/src/Util/ClassSourceManipulator.php index ad880eb64..dc5fac7e8 100644 --- a/src/Util/ClassSourceManipulator.php +++ b/src/Util/ClassSourceManipulator.php @@ -661,21 +661,23 @@ private function addCollectionRelation(BaseCollectionRelation $relation) $paramBuilder->setTypeHint($typeHint); $removerNodeBuilder->addParam($paramBuilder->getNode()); - // add if check to see if item actually exists - //if ($this->avatars->contains($avatar)) - $ifContainsStmt = new Node\Stmt\If_($containsMethodCallNode); - $removerNodeBuilder->addStmt($ifContainsStmt); - - // call removeElement - $ifContainsStmt->stmts[] = BuilderHelpers::normalizeStmt(new Node\Expr\MethodCall( + // $this->avatars->removeElement($avatar) + $removeElementCall = new Node\Expr\MethodCall( new Node\Expr\PropertyFetch(new Node\Expr\Variable('this'), $relation->getPropertyName()), 'removeElement', [new Node\Expr\Variable($argName)] - )); + ); // set the owning side of the relationship - if (!$relation->isOwning()) { + if ($relation->isOwning()) { + // $this->avatars->removeElement($avatar); + $removerNodeBuilder->addStmt(BuilderHelpers::normalizeStmt($removeElementCall)); + } else { if ($relation instanceof RelationOneToMany) { + //if ($this->avatars->removeElement($avatar)) + $ifRemoveElementStmt = new Node\Stmt\If_($removeElementCall); + $removerNodeBuilder->addStmt($ifRemoveElementStmt); + // OneToMany: $student->setCourse(null); /* * // set the owning side to null (unless already changed) @@ -684,7 +686,7 @@ private function addCollectionRelation(BaseCollectionRelation $relation) * } */ - $ifContainsStmt->stmts[] = $this->createSingleLineCommentNode( + $ifRemoveElementStmt->stmts[] = $this->createSingleLineCommentNode( 'set the owning side to null (unless already changed)', self::CONTEXT_CLASS_METHOD ); @@ -707,14 +709,18 @@ private function addCollectionRelation(BaseCollectionRelation $relation) )), ]; - $ifContainsStmt->stmts[] = $ifNode; + $ifRemoveElementStmt->stmts[] = $ifNode; } elseif ($relation instanceof RelationManyToMany) { - // ManyToMany: $student->removeCourse($this); - $ifContainsStmt->stmts[] = new Node\Stmt\Expression(new Node\Expr\MethodCall( - new Node\Expr\Variable($argName), - $relation->getTargetRemoverMethodName(), - [new Node\Expr\Variable('this')] - )); + // $student->removeCourse($this); + // $this->students->removeElement($student); + $removerNodeBuilder->addStmt(BuilderHelpers::normalizeStmt($removeElementCall)); + $removerNodeBuilder->addStmt( + new Node\Stmt\Expression(new Node\Expr\MethodCall( + new Node\Expr\Variable($argName), + $relation->getTargetRemoverMethodName(), + [new Node\Expr\Variable('this')] + )) + ); } else { throw new \Exception('Unknown relation type'); } From 4c41435d8c8feca7a5819e241b43f3ccf2f02196 Mon Sep 17 00:00:00 2001 From: mhabibi Date: Sat, 19 Sep 2020 18:15:46 +0200 Subject: [PATCH 2/5] Issue673 Fix Tests --- .../fixtures/expected_no_overwrite/src/Entity/Client.php | 4 +--- .../fixtures/expected_no_overwrite/src/Entity/User.php | 7 ++----- .../fixtures/expected_overwrite/src/Entity/Client.php | 4 +--- .../fixtures/expected_overwrite/src/Entity/User.php | 7 ++----- .../Doctrine/fixtures/expected_xml/src/Entity/UserXml.php | 3 +-- .../add_many_to_many_relation/User_simple_inverse.php | 6 ++---- .../add_many_to_many_relation/User_simple_no_inverse.php | 4 +--- .../add_many_to_many_relation/User_simple_owning.php | 4 +--- .../Util/fixtures/add_one_to_many_relation/User_simple.php | 3 +-- .../User_simple_orphan_removal.php | 3 +-- .../add_one_to_many_relation/User_with_use_statements.php | 3 +-- 11 files changed, 14 insertions(+), 34 deletions(-) diff --git a/tests/Doctrine/fixtures/expected_no_overwrite/src/Entity/Client.php b/tests/Doctrine/fixtures/expected_no_overwrite/src/Entity/Client.php index 308b93217..144265697 100644 --- a/tests/Doctrine/fixtures/expected_no_overwrite/src/Entity/Client.php +++ b/tests/Doctrine/fixtures/expected_no_overwrite/src/Entity/Client.php @@ -79,9 +79,7 @@ public function addTag(Tag $tag): self public function removeTag(Tag $tag): self { - if ($this->tags->contains($tag)) { - $this->tags->removeElement($tag); - } + $this->tags->removeElement($tag); return $this; } diff --git a/tests/Doctrine/fixtures/expected_no_overwrite/src/Entity/User.php b/tests/Doctrine/fixtures/expected_no_overwrite/src/Entity/User.php index 3524c472d..9fd6a1d81 100644 --- a/tests/Doctrine/fixtures/expected_no_overwrite/src/Entity/User.php +++ b/tests/Doctrine/fixtures/expected_no_overwrite/src/Entity/User.php @@ -75,8 +75,7 @@ public function addAvatar(UserAvatar $avatar): self public function removeAvatar(UserAvatar $avatar): self { - if ($this->avatars->contains($avatar)) { - $this->avatars->removeElement($avatar); + if ($this->avatars->removeElement($avatar)) { // set the owning side to null (unless already changed) if ($avatar->getUser() === $this) { $avatar->setUser(null); @@ -110,9 +109,7 @@ public function addTag(Tag $tag): self public function removeTag(Tag $tag): self { - if ($this->tags->contains($tag)) { - $this->tags->removeElement($tag); - } + $this->tags->removeElement($tag); return $this; } diff --git a/tests/Doctrine/fixtures/expected_overwrite/src/Entity/Client.php b/tests/Doctrine/fixtures/expected_overwrite/src/Entity/Client.php index 308b93217..144265697 100644 --- a/tests/Doctrine/fixtures/expected_overwrite/src/Entity/Client.php +++ b/tests/Doctrine/fixtures/expected_overwrite/src/Entity/Client.php @@ -79,9 +79,7 @@ public function addTag(Tag $tag): self public function removeTag(Tag $tag): self { - if ($this->tags->contains($tag)) { - $this->tags->removeElement($tag); - } + $this->tags->removeElement($tag); return $this; } diff --git a/tests/Doctrine/fixtures/expected_overwrite/src/Entity/User.php b/tests/Doctrine/fixtures/expected_overwrite/src/Entity/User.php index 2e42b65a3..91fdbcee3 100644 --- a/tests/Doctrine/fixtures/expected_overwrite/src/Entity/User.php +++ b/tests/Doctrine/fixtures/expected_overwrite/src/Entity/User.php @@ -82,8 +82,7 @@ public function addAvatar(UserAvatar $avatar): self public function removeAvatar(UserAvatar $avatar): self { - if ($this->avatars->contains($avatar)) { - $this->avatars->removeElement($avatar); + if ($this->avatars->removeElement($avatar)) { // set the owning side to null (unless already changed) if ($avatar->getUser() === $this) { $avatar->setUser(null); @@ -117,9 +116,7 @@ public function addTag(Tag $tag): self public function removeTag(Tag $tag): self { - if ($this->tags->contains($tag)) { - $this->tags->removeElement($tag); - } + $this->tags->removeElement($tag); return $this; } diff --git a/tests/Doctrine/fixtures/expected_xml/src/Entity/UserXml.php b/tests/Doctrine/fixtures/expected_xml/src/Entity/UserXml.php index b5df783be..9c0a3a700 100644 --- a/tests/Doctrine/fixtures/expected_xml/src/Entity/UserXml.php +++ b/tests/Doctrine/fixtures/expected_xml/src/Entity/UserXml.php @@ -56,8 +56,7 @@ public function addAvatar(UserAvatar $avatar): self public function removeAvatar(UserAvatar $avatar): self { - if ($this->avatars->contains($avatar)) { - $this->avatars->removeElement($avatar); + if ($this->avatars->removeElement($avatar)) { // set the owning side to null (unless already changed) if ($avatar->getUser() === $this) { $avatar->setUser(null); diff --git a/tests/Util/fixtures/add_many_to_many_relation/User_simple_inverse.php b/tests/Util/fixtures/add_many_to_many_relation/User_simple_inverse.php index 5662d25a9..a9d4dcc38 100644 --- a/tests/Util/fixtures/add_many_to_many_relation/User_simple_inverse.php +++ b/tests/Util/fixtures/add_many_to_many_relation/User_simple_inverse.php @@ -53,10 +53,8 @@ public function addRecipe(Recipe $recipe): self public function removeRecipe(Recipe $recipe): self { - if ($this->recipes->contains($recipe)) { - $this->recipes->removeElement($recipe); - $recipe->removeFood($this); - } + $this->recipes->removeElement($recipe); + $recipe->removeFood($this); return $this; } diff --git a/tests/Util/fixtures/add_many_to_many_relation/User_simple_no_inverse.php b/tests/Util/fixtures/add_many_to_many_relation/User_simple_no_inverse.php index 8421cd682..2d1e7288d 100644 --- a/tests/Util/fixtures/add_many_to_many_relation/User_simple_no_inverse.php +++ b/tests/Util/fixtures/add_many_to_many_relation/User_simple_no_inverse.php @@ -52,9 +52,7 @@ public function addRecipe(Recipe $recipe): self public function removeRecipe(Recipe $recipe): self { - if ($this->recipes->contains($recipe)) { - $this->recipes->removeElement($recipe); - } + $this->recipes->removeElement($recipe); return $this; } diff --git a/tests/Util/fixtures/add_many_to_many_relation/User_simple_owning.php b/tests/Util/fixtures/add_many_to_many_relation/User_simple_owning.php index d822debe5..e1b079156 100644 --- a/tests/Util/fixtures/add_many_to_many_relation/User_simple_owning.php +++ b/tests/Util/fixtures/add_many_to_many_relation/User_simple_owning.php @@ -52,9 +52,7 @@ public function addRecipe(Recipe $recipe): self public function removeRecipe(Recipe $recipe): self { - if ($this->recipes->contains($recipe)) { - $this->recipes->removeElement($recipe); - } + $this->recipes->removeElement($recipe); return $this; } diff --git a/tests/Util/fixtures/add_one_to_many_relation/User_simple.php b/tests/Util/fixtures/add_one_to_many_relation/User_simple.php index c078da61c..19b79ca62 100644 --- a/tests/Util/fixtures/add_one_to_many_relation/User_simple.php +++ b/tests/Util/fixtures/add_one_to_many_relation/User_simple.php @@ -53,8 +53,7 @@ public function addAvatarPhoto(UserAvatarPhoto $avatarPhoto): self public function removeAvatarPhoto(UserAvatarPhoto $avatarPhoto): self { - if ($this->avatarPhotos->contains($avatarPhoto)) { - $this->avatarPhotos->removeElement($avatarPhoto); + if ($this->avatarPhotos->removeElement($avatarPhoto)) { // set the owning side to null (unless already changed) if ($avatarPhoto->getUser() === $this) { $avatarPhoto->setUser(null); diff --git a/tests/Util/fixtures/add_one_to_many_relation/User_simple_orphan_removal.php b/tests/Util/fixtures/add_one_to_many_relation/User_simple_orphan_removal.php index b71daa37b..4f92ff0fd 100644 --- a/tests/Util/fixtures/add_one_to_many_relation/User_simple_orphan_removal.php +++ b/tests/Util/fixtures/add_one_to_many_relation/User_simple_orphan_removal.php @@ -53,8 +53,7 @@ public function addAvatarPhoto(UserAvatarPhoto $avatarPhoto): self public function removeAvatarPhoto(UserAvatarPhoto $avatarPhoto): self { - if ($this->avatarPhotos->contains($avatarPhoto)) { - $this->avatarPhotos->removeElement($avatarPhoto); + if ($this->avatarPhotos->removeElement($avatarPhoto)) { // set the owning side to null (unless already changed) if ($avatarPhoto->getUser() === $this) { $avatarPhoto->setUser(null); diff --git a/tests/Util/fixtures/add_one_to_many_relation/User_with_use_statements.php b/tests/Util/fixtures/add_one_to_many_relation/User_with_use_statements.php index 273e9c703..df6e9ed06 100644 --- a/tests/Util/fixtures/add_one_to_many_relation/User_with_use_statements.php +++ b/tests/Util/fixtures/add_one_to_many_relation/User_with_use_statements.php @@ -55,8 +55,7 @@ public function addAvatarPhoto(UserAvatarPhoto $avatarPhoto): self public function removeAvatarPhoto(UserAvatarPhoto $avatarPhoto): self { - if ($this->avatarPhotos->contains($avatarPhoto)) { - $this->avatarPhotos->removeElement($avatarPhoto); + if ($this->avatarPhotos->removeElement($avatarPhoto)) { // set the owning side to null (unless already changed) if ($avatarPhoto->getUser() === $this) { $avatarPhoto->setUser(null); From fc334e6803726ce9c69f19a6dbe90559d31f0b22 Mon Sep 17 00:00:00 2001 From: mhabibi Date: Sat, 19 Sep 2020 18:28:25 +0200 Subject: [PATCH 3/5] Issue673 Fix CS --- src/Util/ClassSourceManipulator.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Util/ClassSourceManipulator.php b/src/Util/ClassSourceManipulator.php index dc5fac7e8..b83b1b331 100644 --- a/src/Util/ClassSourceManipulator.php +++ b/src/Util/ClassSourceManipulator.php @@ -674,7 +674,7 @@ private function addCollectionRelation(BaseCollectionRelation $relation) $removerNodeBuilder->addStmt(BuilderHelpers::normalizeStmt($removeElementCall)); } else { if ($relation instanceof RelationOneToMany) { - //if ($this->avatars->removeElement($avatar)) + //if ($this->avatars->removeElement($avatar)) $ifRemoveElementStmt = new Node\Stmt\If_($removeElementCall); $removerNodeBuilder->addStmt($ifRemoveElementStmt); From d114edb3dff27d006214f27a16ca865ee6872e65 Mon Sep 17 00:00:00 2001 From: mhabibi Date: Sat, 10 Oct 2020 12:24:19 +0200 Subject: [PATCH 4/5] Issue673 Fix ManyToMay --- src/Util/ClassSourceManipulator.php | 14 ++++++-------- .../User_simple_inverse.php | 5 +++-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/Util/ClassSourceManipulator.php b/src/Util/ClassSourceManipulator.php index b83b1b331..01e5987cd 100644 --- a/src/Util/ClassSourceManipulator.php +++ b/src/Util/ClassSourceManipulator.php @@ -673,10 +673,10 @@ private function addCollectionRelation(BaseCollectionRelation $relation) // $this->avatars->removeElement($avatar); $removerNodeBuilder->addStmt(BuilderHelpers::normalizeStmt($removeElementCall)); } else { + //if ($this->avatars->removeElement($avatar)) + $ifRemoveElementStmt = new Node\Stmt\If_($removeElementCall); + $removerNodeBuilder->addStmt($ifRemoveElementStmt); if ($relation instanceof RelationOneToMany) { - //if ($this->avatars->removeElement($avatar)) - $ifRemoveElementStmt = new Node\Stmt\If_($removeElementCall); - $removerNodeBuilder->addStmt($ifRemoveElementStmt); // OneToMany: $student->setCourse(null); /* @@ -712,14 +712,12 @@ private function addCollectionRelation(BaseCollectionRelation $relation) $ifRemoveElementStmt->stmts[] = $ifNode; } elseif ($relation instanceof RelationManyToMany) { // $student->removeCourse($this); - // $this->students->removeElement($student); - $removerNodeBuilder->addStmt(BuilderHelpers::normalizeStmt($removeElementCall)); - $removerNodeBuilder->addStmt( - new Node\Stmt\Expression(new Node\Expr\MethodCall( + $ifRemoveElementStmt->stmts[] = new Node\Stmt\Expression( + new Node\Expr\MethodCall( new Node\Expr\Variable($argName), $relation->getTargetRemoverMethodName(), [new Node\Expr\Variable('this')] - )) + ) ); } else { throw new \Exception('Unknown relation type'); diff --git a/tests/Util/fixtures/add_many_to_many_relation/User_simple_inverse.php b/tests/Util/fixtures/add_many_to_many_relation/User_simple_inverse.php index a9d4dcc38..fa2bc65d8 100644 --- a/tests/Util/fixtures/add_many_to_many_relation/User_simple_inverse.php +++ b/tests/Util/fixtures/add_many_to_many_relation/User_simple_inverse.php @@ -53,8 +53,9 @@ public function addRecipe(Recipe $recipe): self public function removeRecipe(Recipe $recipe): self { - $this->recipes->removeElement($recipe); - $recipe->removeFood($this); + if ($this->recipes->removeElement($recipe)) { + $recipe->removeFood($this); + } return $this; } From 7f7c32d76f416b09b93ef67ca821834496a0e355 Mon Sep 17 00:00:00 2001 From: mhabibi Date: Sat, 10 Oct 2020 12:36:40 +0200 Subject: [PATCH 5/5] Issue673 Fix CS --- src/Util/ClassSourceManipulator.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Util/ClassSourceManipulator.php b/src/Util/ClassSourceManipulator.php index 01e5987cd..f4bc45a40 100644 --- a/src/Util/ClassSourceManipulator.php +++ b/src/Util/ClassSourceManipulator.php @@ -677,7 +677,6 @@ private function addCollectionRelation(BaseCollectionRelation $relation) $ifRemoveElementStmt = new Node\Stmt\If_($removeElementCall); $removerNodeBuilder->addStmt($ifRemoveElementStmt); if ($relation instanceof RelationOneToMany) { - // OneToMany: $student->setCourse(null); /* * // set the owning side to null (unless already changed)