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

UnitOfWork::clear() misses $eagerLoadingEntities #7869

Merged
merged 2 commits into from
Nov 15, 2019

Conversation

BenMorel
Copy link
Contributor

AFAICS, eager loading entities should be cleared as well when the UnitOfWork is cleared.

Actually, this is already the case on master!

@greg0ire
Copy link
Member

phpcs has a weird complaint I don't understand… can you try running vendor/bin/phpcbf?

@BenMorel
Copy link
Contributor Author

I'm not sure why this happens either... Locally, phpcs reports, and phpcbf fixes, thousands (!) of CS issues.

I'm not familiar with phpcs, but I suspect from the command:

php git-phpcs.phar origin/$TRAVIS_BRANCH...$TRAVIS_PULL_REQUEST_SHA

that it only reports issues on the line(s) modified by the PR. And due to the surrounding lines, there is not much I can do to fix this.

@greg0ire
Copy link
Member

Maybe you could try aligning all equal signs, including in the surrounding lines?

@BenMorel
Copy link
Contributor Author

Let's try this!

@greg0ire
Copy link
Member

Hey! That actually worked! :)

@greg0ire
Copy link
Member

Original commit: d070fc6

greg0ire
greg0ire previously approved these changes Oct 16, 2019
@greg0ire
Copy link
Member

The commit above contains a similar change for $persisters, should it be applied too?

@BenMorel
Copy link
Contributor Author

I have unfortunately no idea. For sure anything that contains a reference to an entity must be cleared, hence this PR, for the rest I'm not the right person to ask ;)

@greg0ire
Copy link
Member

Let's wait for feedback from contributors that know the UOW better than I do.

Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BenMorel thanks for the patch. The fix is 100% valid but we need a test to be able to get it merged 👍

@lcobucci lcobucci added this to the 2.6.5 milestone Oct 16, 2019
@BenMorel
Copy link
Contributor Author

@lcobucci Adding a test does not look like a trivial task, as there is no setter/getter for $eagerLoadingEntities.

Suggestions:

  • test using reflection 🤢
  • side-test using triggerEagerLoads(); this would require a complex setting, where you have to somehow set up an eager loading entity, and check that its EntityPersister is not invoked when calling triggerEagerLoads() after clear(); really not trivial, in comparison to the fix!

@SenseException
Copy link
Member

Suggestion: You could use an EntityManager in a functional test to setup eagerloading entities and test the clear-method of the UOW.

Otherwise, getClassMetadata would be triggered more times
@ostrolucky
Copy link
Member

Test case added

@BenMorel
Copy link
Contributor Author

@ostrolucky Thank you!

Copy link
Member

@guilhermeblanco guilhermeblanco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an issue I found when i was working on ephemeral unit of work support. It also helps on freeing resources as eager loaded entities hang there forever. Patch on its own is great.

@guilhermeblanco guilhermeblanco merged commit c629774 into doctrine:2.6 Nov 15, 2019
*/
class GH7869Test extends OrmTestCase
{
public function testDQLDeferredEagerLoad()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to rename test case 😞 Let me know if I Should create new PR with just this change

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

Successfully merging this pull request may close these issues.

None yet

6 participants