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.

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

Since v2.19.5 $em->remove($child) doesn't work if called prior to creating another child and $em->persist($parent) #11448

Open
mislavjakopovic opened this issue May 10, 2024 · 1 comment

Comments

@mislavjakopovic
Copy link

mislavjakopovic commented May 10, 2024

BC Break Report

Q A
BC Break yes
Version 2.19.5

Summary

Following scenario produces a breaking change after upgrading to doctrine/orm: "2.19.5":

  1. Create a Parent and Child entity which are linked via OneToMany relation. Parent relation is configured to cascade: ['persist', 'remove']
  2. Delete a Child entity via $entityManager->remove($child)
  3. Retrieve a Parent entity which is linked to a deleted Child via f.e. $parentRepository->find()
  4. Create a new Child entity and link it to retrieved Parent entity
  5. Persist that Parent entity $entityManager->persist($parent)

Persisting a Parent entity directly instead of Child is not the best approach, but afaik there is nowhere written that
it is forbidden. I believe there are some use cases where this can actually be quite practical, instead of having persist child on 10 places in code, a simple persist of parent works here more elegant. It is not the best pattern, but I believe it is a valid behavior, otherwise we wouldn't be even allowed to call persist on already loaded entity manually.

The issue comes from a change where we would remove entity from identityMap only upon executing deletion queries (UnitOfWork::executeDeletions) rather than straight away (UnitOfWork::scheduleForDelete): #11428

Example

// Remove all entries with odd IDs
foreach ($this->pageRepository->findAll() as $page) {
    if ($page->getId() % 2) {
        $this->entityManager->remove($page);
    }
}

foreach ($this->bookRepository->findAll() as $book) {
    $book->setName('New Book Name');

    // The loop here is not required since the same bug would occur with just ONE new entity,
    // here we're just creating more of them to showcase a possible real world example
    for ($i = 0; $i < 5; $i++) {
        $newPage = new Page();
        $newPage->setTitle('New Page Name');
        $book->addPage($newPage);
    }
    $this->entityManager->persist($book);
}

$this->entityManager->flush();

Previous Behavior (v2.19.4)

After calling $entityManager->flush() Child entity was deleted and a new Child was created and assigned to Parent.

Current Behavior (v2.19.5)

After calling $entityManager->flush() Child entity is not deleted and a new Child is created and assigned to parent (remove had no effect)

How to recreate

Made a test case repository https://github.com/mislavjakopovic/doctrine-orm-issue-11448/ with example code which can be ran quickly via docker-compose.

Workarounds

In doctrine/orm: "2.19.5" for above remove() to have effect we need to either:

  • place a flush() call right after remove()
  • or persist($newPage), not persist(book)
  • or remove call to persist(book)
@xificurk
Copy link
Contributor

xificurk commented May 12, 2024

Thanks for the example repo. The main issue is that the Page entity is removed without updating its existing association to Book entity, i.e. the removed Page is still contained in Book::$pages collection.

Why the Page rows are deleted from database on <=2.19.4 :

When Page entity is removed, Book::$pages collection is not updated, but it is also not initialized. The collection gets initialized only after Book::addPage() call. Because of the original bug (fixed in #11428) the collection will contain a references to new instances of removed entities, and because they are in managed state, they will be ignored by cascading persist($book) call. The flush results in inconsistent state, where odd-id Page entities are deleted from database, but their instances are kept in identity map and Book::$pages collections. See PR with expanded output that iterates over the contents of Book::$pages: mislavjakopovic/doctrine-orm-issue-11448#3

Furthermore, the entities are deleted from database only thanks to the fact that Book::$pages collections are not initialized at the moment of remove() call. If those collections are initialized before that, they will not get deleted: mislavjakopovic/doctrine-orm-issue-11448#2

The proper fix is to take care of any existing associations before removing the entity: mislavjakopovic/doctrine-orm-issue-11448#1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants