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

Update DoctrineHelper #143

Merged
merged 3 commits into from
Dec 13, 2019

Conversation

p365labs
Copy link
Contributor

@p365labs p365labs commented Dec 7, 2019

remove DoctrineHelper as it's a duplicate of Doctrine Common ClassUtils,

Have a look a this discussion and this is the class which will substitute [the Common ClassUtils]
(https://github.com/Ocramius/ProxyManager/blob/addb7d39c990d3d2299315f0200e78ebe658cd7a/src/ProxyManager/Inflector/ClassNameInflector.php#L41)

@p365labs p365labs force-pushed the remove_doctrine_helper branch from 1b445ff to 8cbbf4c Compare December 7, 2019 14:17
@maxhelias
Copy link
Contributor

It's not a duplicate of ClassUtils because this one is deprecated. So we this change, we introduce a deprecated log

@maxhelias
Copy link
Contributor

But we can improve this behavior with this implementation : https://github.com/api-platform/core/blob/master/src/Util/ClassInfoTrait.php#L38

@p365labs
Copy link
Contributor Author

p365labs commented Dec 8, 2019

I said duplicate because the code inside DoctrineHelper is exactly the same the one is inside Doctrine-> ClassUtils, so if it's deprecated for Doctrine .. it's deprecated also for this project.

In my implementation, I simply remove DoctrineHelper and used the deprecated solution, which is
not forbidden, but it's a statement to let us know we need to find a different strategy.

Ocramius suggested the right replacement strategy:
doctrine/common#826 (comment) which is this class

Don't know about the API-Platform solution. But why using that solution instead of the ProxyManager one?

@maxhelias
Copy link
Contributor

That correct for the duplicate but not for the deprecated for the project, it allows us to detach ourselves from doctrine depreciation and to be able to have compatibility with several versions without deprecated.

The replacement suggested by Ocramius, it's a good alternative but adds a hard dependency. So the good way would be to read the metadata with the EntityManager like this doctrine/common#867 :

$em = $this->getDoctrineEntityManager();
$classname = $em->getClassMetadata(get_class($object))->getName();

But i seems there is a problem with doctrine odm, so we need to integration like Api-Platform to be compatible with both proxy markers and without hard dependency :

__CG__: Doctrine Common Marker for Proxy (ODM < 2.0 and ORM < 3.0)
__PM__: Ocramius Proxy Manager (ODM >= 2.0)

Copy link
Owner

@DamienHarper DamienHarper left a comment

Choose a reason for hiding this comment

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

@p365labs I prefer the ApiPlatform's approach of the problem for the same reasons as the ones discussed by @maxhelias
Could you please update your PR accordingly?

@p365labs
Copy link
Contributor Author

thnk you @DamienHarper & @maxhelias thank u for the clarification, yep for sure! I'll do it in the next couple of days.

@p365labs p365labs force-pushed the remove_doctrine_helper branch from 8cbbf4c to 00e8c55 Compare December 12, 2019 06:51
@p365labs p365labs changed the title remove DoctrineHelper [WIP] remove DoctrineHelper Dec 12, 2019
@p365labs p365labs force-pushed the remove_doctrine_helper branch 2 times, most recently from 0d3514e to a2905ec Compare December 12, 2019 07:06
@p365labs
Copy link
Contributor Author

Hi! I've updated the PR implementing the solution from ApiPlatform and referencing the authors.
Instead of using a trait I implemented a static method, and I think before merging we need more tests on this use case, I'll work on this during these days.

@p365labs p365labs force-pushed the remove_doctrine_helper branch from a2905ec to 31eef44 Compare December 12, 2019 07:07
@p365labs p365labs changed the title [WIP] remove DoctrineHelper remove DoctrineHelper Dec 12, 2019
@p365labs p365labs changed the title remove DoctrineHelper [WIP] remove DoctrineHelper Dec 12, 2019
@p365labs p365labs force-pushed the remove_doctrine_helper branch from 31eef44 to d8ef73c Compare December 12, 2019 08:58
@p365labs p365labs force-pushed the remove_doctrine_helper branch from d8ef73c to 83b2cec Compare December 12, 2019 09:35
@p365labs
Copy link
Contributor Author

p365labs commented Dec 12, 2019

@DamienHarper & @maxhelias
I'm wondering why inside DoctrineHelper we need a method generateProxyClassName
is it a mandatory requirements?
At the moment is not used inside the src and it's only tested in tests

Otherwise I need to find a way to identify which ORM or ODM version is using...which is a bit weird to do.
Any ideas?

@DamienHarper
Copy link
Owner

@p365labs I'm afraid it's a leftover from the past :) You can remove it from DoctrineHelper and from relevant tests

@p365labs p365labs changed the title [WIP] remove DoctrineHelper remove DoctrineHelper Dec 13, 2019
@p365labs
Copy link
Contributor Author

p365labs commented Dec 13, 2019

tests done! I've added some more tests and tested the Proxy use cases. I've also updated the PR title.

@p365labs p365labs changed the title remove DoctrineHelper Update DoctrineHelper Dec 13, 2019
@DamienHarper DamienHarper merged commit 322cedf into DamienHarper:master Dec 13, 2019
@p365labs p365labs deleted the remove_doctrine_helper branch December 13, 2019 16:04
@DamienHarper DamienHarper added this to the 3.3.0 milestone Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants