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

workaround for issue 1310 #1378

Closed
wants to merge 1 commit into from
Closed

Conversation

dmaicher
Copy link
Contributor

This is a first hacky idea to start a discussion on how to fix #1310 properly.

So to make sure we properly warm up complete metadata we need to ensure the metadataFactory does not have anything loaded yet when we replace the cache and load all available metadata. Hence the LogicException that is causing issues now.

Since the AbstractClassMetadataFactory does not provide a clearLoadedMetadata (or similar) method I don't see many options.

  • hacky solution I implemented here

    • but it relies on isset checks inside AbstractClassMetadataFactory to work and ignore null values
    • it violates the contract I think as setMetadataFor is not meant to be called with null
  • use reflection to set AbstractClassMetadataFactory::$loadedMetadata to an empty array?

  • add AbstractClassMetadataFactory::clearLoadedMetadata on doctrine/persistence?

  • other ideas?

@ostrolucky
Copy link
Member

We ship own implementation of AbstractClassMetadataFactory so I think we are already pretty tightly coupled to it. You said your workaround relies on isset call, so I assume it actually relies on output from hasMetadataFor method? If that's so, we could also override the function and implement custom logic in there, correct?

Another idea would be to somehow replace the service instance so that its state is cleared, similarly like doctrine-bridge does with EntityManager in ManagerRegistry::resetService. But at least in case of EntityManager it brings its own can of worms, so not sure it would be cleaner here rather than using null or using reflection.

But in the end I don't quite understand the bug, so I can't be much of a help here I'm afraid.

@dmaicher
Copy link
Contributor Author

dmaicher commented Jul 2, 2021

I will try to find some time in the next week to work on this

@stof
Copy link
Member

stof commented Nov 23, 2021

Replacing the service instance is hard to do, as it requires that no instantiated service in the container contain a reference to the old instance. This quickly becomes insane into what it implies. That's why I opened an issue a long time ago in doctrine/orm about implementing a native resetting feature (allowing to re-open a closed entity manager into a cleared state) to remove that need for the resetting proxy.

@ostrolucky
Copy link
Member

This PR is opened for a long time with no activity. Let's close until there is more activity, or feel free to open new PR later.

@ostrolucky ostrolucky closed this Dec 25, 2021
@dmaicher dmaicher deleted the fix_issue_1310 branch June 5, 2023 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants