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

Introduce proxy class name resolvers #145

Merged
merged 1 commit into from
Feb 17, 2021

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Nov 27, 2020

The metadata factory tries to resolve a proxy class name to get the real class from it. However, this only works when using proxies from doctrine/common. MongoDB ODM 2 and ORM 3 will use ProxyManager to create proxy objects, which requires workarounds in ODM whenever metadata is fetched.

This PR introduces a small interface that can be used to implement and inject such class name resolvers. If no resolver is defined, it falls back to using the current logic. Note that the resolver is implemented in this package as doctrine/common is a dev dependency and thus not available when doctrine/persistence is used in userland code.

@alcaeus alcaeus added the Enhancement New feature or request label Nov 27, 2020
@alcaeus alcaeus added this to the 2.2.0 milestone Nov 27, 2020
@alcaeus alcaeus requested a review from greg0ire November 27, 2020 14:39
@alcaeus alcaeus self-assigned this Nov 27, 2020
@greg0ire
Copy link
Member

I pushed a fix for static analysis, let's trigger a build on a different merge commit.

@greg0ire greg0ire closed this Nov 27, 2020
@greg0ire greg0ire reopened this Nov 27, 2020

private function createDefaultProxyClassNameResolver(): void
{
$this->proxyClassNameResolver = new class implements ProxyClassNameResolver {
Copy link
Member

@malarzm malarzm Dec 1, 2020

Choose a reason for hiding this comment

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

I remember needing a real class name in quite few projects I was working on in the past, I think it'd be helpful to have a real DoctrineProxyClassNameResolver one can depend on instead of an anonymous class

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this is very much internal, as ocramius/proxy-manager already provides a public interface to fetch the real class name from a proxy object. This is only glue code to be able to inject those resolvers into persistence to ensure metadata fetches are done on the real class.

Since ORM still uses common proxies (with Doctrine\Common\ClassUtils to provide the real implementation name), I believe it makes sense to not expose a fully fledged class here.

@beberlei
Copy link
Member

beberlei commented Dec 8, 2020

You are reimplementing the common function ClassUtils::getRealClass here, i am wondering if we should just hardcore proxy manager support in there too. This would be much better than deprecating this code, there is so much depedent code out there using these APIs.

@beberlei
Copy link
Member

beberlei commented Dec 8, 2020

See doctrine/common#867 and the referenced code from a user DamienHarper/auditor-bundle#143 doing just that.

@alcaeus
Copy link
Member Author

alcaeus commented Dec 9, 2020

You are reimplementing the common function ClassUtils::getRealClass here, i am wondering if we should just hardcore proxy manager support in there too.

Not going to happen. I duplicated our own logic to avoid a dependency on doctrine/common, but I can refactor this to try and use ClassUtils::getRealClass if it exists and do nothing if no resolver was set. Duplicating internal logic from a third-party package when that package provides a public API to do this work is not feasible here.

Note that we won't be able to get around doing this kind of logic here either: since Doctrine offers to return proxy objects when fetching from the database, it should be very much capable with receiving proxy objects or their class names in its logic. Each implementation of the Doctrine persistence API (e.g. ORM or ODM) can inject their resolvers depending on the solution they use for proxies.

@alcaeus alcaeus force-pushed the introduce-proxy-classname-resolvers branch from 6cab3f4 to a4befd7 Compare February 15, 2021 15:15
@alcaeus alcaeus requested a review from greg0ire February 15, 2021 15:15
@alcaeus alcaeus merged commit 12a05f8 into doctrine:2.2.x Feb 17, 2021
@alcaeus alcaeus deleted the introduce-proxy-classname-resolvers branch February 17, 2021 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants