Doctrine2: `grabEntityFromRepository()` causes invalid entity to be persisted

[ThomasLandauer]

In short: When editing an existing entity with a Symfony form and making it invalid, calling $I->grabEntityFromRepository() in a functional tests causes the (invalid!) entity to get persisted to the database.

Sample repository to reproduce: https://github.com/ThomasLandauer/codeception_issue_5439

Instructions:

• The sqlite database is located at /data.sqlite. Doublecheck that user ID 1's name is NULL.
• Start PHP's internal webserver in /public with php -S localhost:8000
• Open http://localhost:8000/user in the browser. The form is editing user ID 1.
• Enter "foobar" and submit. You'll see an error message, due to the constraint at https://github.com/ThomasLandauer/codeception_issue_5439/blob/master/src/Entity/User.php#L22
• Doublecheck that user ID 1's name is still NULL in the database.
• Now run vendor/bin/codecept run functional UserCest. The test does the same as you just did in the browser. Even the error message is present, see https://github.com/ThomasLandauer/codeception_issue_5439/blob/master/tests/functional/UserCest.php#L24
• Now look into the database: User ID 1's name is "foobar"!

The bug only appears if the form is editing an existing user (not creating a new one).

This might be related to https://github.com/Codeception/Codeception/issues/4804 , but it's worse, since it doesn't just flush what is "ready" to get flushed, but performs a full persist on its own.

[alexkunin]

Doctrine2 calls persist() only in haveInRepository() and persistEntity(). Any grab*() or see*() do not cause persisting. I think Codeception/Codeception#4804 explains it mostly correctly, except the following:

• persist() does not say Doctrine ORM to save the entity now -- it says 'make it managed (tracked), be aware of it, and if you see it has been updated, persist it on next flush()'
• only new Entity() is not persisted; if you load entity using some repository method, it is already 'persistent' (i.e. managed by Doctrine ORM)
• symfony/form updates entity fields during validation, but if validation fails, it does not rollback changes

(Note: persist() is a bit vague, track() or attach() or even watch() would better describe what is going on.)

What I'm trying to say is the following: this is a complex issue spread across several systems (ORM, Symfony, Codeception), and in my opinion only ORM does decent job here. Both Symfony and Codeception use somewhat dirty tricks. Your code (not blaming you, as you seem to follow what Symfony docs say) also implicitly 'hopes' for request to die and to discard invalid entity in that way. Which might not always be true, e.g. in case of this functional test, and also maybe if you use something like php-pm where Symfony kernel and Doctrine entity manager persist across requests.

I agree that removing implicit flush() would help. I think it might be an option in Doctrine2 configuration (TBH I'd remove it altogether, but I'm not sure how bad this would be for backward compatibility).

Another (much more complex, but probably more correct) solution would be decoupling entity from form data object. Entity must be valid at all times, and symfony/form does not follow that rule. TBH default Doctrine codegeneration with lots of separate getters/setters also does help having entity in valid state (or maybe it is symfony/maker who generates the code like that?).

[alexkunin]

@DavertMik, it seems you're the one who added flush() calls in the first place 7 years ago -- maybe you remember any reasoning behind that decision?

[alexkunin]

Another solution would be detaching entity if validation fails -- $em->detach($entity);, but that requires changes in your code and still is a workaround of Doctrine2 module quirk.

[ThomasLandauer]

This issue just caused me some trouble again ;-) I even suggested a (bogus) improvement for Symfony, before realizing that it's in fact Codeception that is magically saving my entities to the database; see symfony/symfony#7828 (comment)

So what can we do? I see those options:

• @artmnv is essentially saying that Symfony is doing it wrong: https://github.com/Codeception/Codeception/issues/4804#issuecomment-368550425 But waiting for Symfony to change its behavior isn't a real option, I would say.

Before discussing other options, let me show you how I just fixed it in my project:

"Inject" the repository like this in Cest's _before() method:

$this->myRepository = $I->grabService('App\Repository\MyRepository');

...and then go with Doctrine's built-in functions (like e.g. findOneBy()) or your own repository methods.

This workaround is so easy and adds so little overhead that I'm asking a fundamental question: What's the real benefit of grabEntityFromRepository() at all? If we can't solve this issue, why can't we just deprecate grabEntityFromRepository() and tell people to do it the above way?

If the answer is to keep grabEntityFromRepository(), I see two options:

• Add a "warning" at https://codeception.com/docs/modules/Doctrine2 and let people know that it's safer to use either the above workaround. Or another: Add a helper method ( e.g. grabEntityFromRepositoryWithoutFlush()) that doesn't flush(), see https://github.com/Codeception/Codeception/issues/4804#issuecomment-465990375

• Remove the flush() from grabEntityFromRepository(): https://github.com/Codeception/module-doctrine2/blob/master/src/Codeception/Module/Doctrine2.php#L964

Bottom line: This is a serious issue, since it leads to false test results. I think it should be fixed now. @alexkunin, @Naktibalda - what do you say?

[ThomasLandauer]

I now finally added what I recommended above to this module's docs: https://codeception.com/docs/modules/Doctrine2#Grabbing-Entities-with-Symfony
So I'm closing this.