persistence
persistence copied to clipboard
doctrine
Remove dependency to `doctrine/common`
Two good reasons to do this move:
- This was a circular dependency.
- doctrine/common is being sunset.
The code does not properly handle cases where code implements the Proxy interface without the future methods
The code does not properly handle cases where code implements the Proxy interface without the future methods
I see several paths forward:
- deprecate the
Proxyinterface in favor of a more complete one, check it in addition to the one from common. Drawbacks: it will have a worse name, the best name being already taken (unless you can find something ?). Also, it won't allow us to drop the dependency todoctrine/common - add a complementary interface with only the missing methods, extend it in
doctrine/common, releasedoctrine/common, add a conflict on persistence with versions older than the latestdoctrine/common, and throw unless both interfaces are implemented. Drawback: it's quite complicated - add
method_existschecks and throw in the appropriate case. Drawback: I'm not sure how performance-sensitive the code where the check happens is, or if having those checks could become an issue. Maybe check just for one method and assuming the rest is implemented would be sufficient. Anyway, even if all methods exist, that's not a guarantee that they have the required signature.
What would you recommend? I'm leaning towards 3.
I went ahead and implemented a check, but just for the methods that are called.
It's been a while since I had to touch proxied-public-properties-voodoo magic but here goes nothing: can't we outsource the optimization to doctrine/common and/or employ consumers (well, ORM) to do some check beforehand and remove code altogether? Given ORM is the only consumer, and it already relies on doctrine/common, it could even provide its own implementation for needed reflection.
@malarzm no idea yet, but it sounds better indeed, I'll try to look into that 👍
@malarzm I've taken a look at ORM, and getValue() seems to be called in a lot of places. I'm not quite sure the check is useful in 100% of the call sites. Maybe somebody will know which one actually require the check.
I think one way to do this in a simple way would be to override AbstractClassMetadataFactory::getReflectionService() so that it returns a service implemented in the ORM with an overridden getAccessibleProperty method which would return 3 types inheriting the 3 ones from persistence, with an overridden method… this looks quite hairy.
@derrabus @beberlei what do you think about this?
It should be noted that if we decide to go that way, we will have to deprecate passing a proxy object to getValue(), and postpone the removal of the check to 4.0. Because of how this will slow things down, I think we should merge this PR regardless of whether we do what @malarzm suggests, that way we get rid of the (circular!) dependency now.
GetValue is Performance critical, i would like to avoid to increase the number of method calls there.
Here goes nothing #2 to keep the changes minimal: instanceof works just fine with possibly-non-existing classes, how about keeping things as is reflection-wise and just moving doctrine/common to require-dev for the sake of tests? It's hacky a bit but will work :)
@malarzm it's already in require-dev :wink: This PR is a step towards sunsetting doctrine/common.
it's already in require-dev
Oh, missed that. What's the point then? Once we get rid of old Proxies in ORM (and enough time passes) we'll be able to just get rid of it. What's the benefit now?
- I'm trying to migrate ORM to PHP 8 syntax: https://github.com/doctrine/orm/projects/6
- It's better to add type declarations to doctrine/common beforehand, that way I can add them as well on inheriting types, but @derrabus suggested we kill
doctrine/commoninstead - I'm trying to kill it
Shouldn't ORM switch to ProxyManager first and continue PHP 8 syntax afterwards? Then doctrine/common will be dropped from its requirements. We can't really kill doctrine/common before releasing ORM 3.0 and even after that we'll have to provide support for 2.x for quite some time
Shouldn't ORM switch to ProxyManager first and continue PHP 8 syntax afterwards?
This comment says we shouldn't: https://github.com/doctrine/orm/issues/8518#issuecomment-1124805479
We can't really kill doctrine/common before releasing ORM 3.0
Can't we? If we copy proxy classes to the ORM, maybe we can?
If we copy proxy classes to the ORM, maybe we can?
All right, had no idea it was the plan :)
@beberlei after #299, the extra method calls are moved to setValue, is that better performance-wise? This method is called less than getValue I suppose.
Converting to draft as determining how to replace doctrine/common proxy functionality might have some influence on this PR.
@greg0ire Generally setValue is used in hydration, getValue in changeset computation. So setValue is probably called much more often as its for every read.