entity generator - ignore trait properties and methods #632

Merged
merged 11 commits into from Jul 7, 2013

Conversation

Projects
None yet
10 participants
Contributor

Padam87 commented Mar 26, 2013

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2372

Contributor

Padam87 commented Mar 26, 2013

I'm not sure if this is the right solution, but modifying the ClassMetadataInfo because of this felt like a bad idea. That would change trait handling entirely.

@stof stof and 1 other commented on an outdated diff Mar 27, 2013

lib/Doctrine/ORM/Tools/EntityGenerator.php
@@ -709,6 +709,17 @@ protected function hasProperty($property, ClassMetadataInfo $metadata)
}
}
+ // check traits for existing property
+ if (class_exists($metadata->name)) {
+ $reflClass = new \ReflectionClass($metadata->name);
+
+ foreach ($reflClass->getTraits() as $trait) {
@stof

stof Mar 27, 2013

Member

Please don't bump the requirement to 5.4

@Padam87

Padam87 Mar 27, 2013

Contributor

Thanks, forgot about that. I will add an if (method_exists($reflClass, 'getTraits'))

Member

stof commented Mar 27, 2013

This should be tested

Contributor

Padam87 commented Mar 28, 2013

@pamil You're welcome :)

Sorry guys, i don't have the time right now, but I will create some unit test this weekend.

Owner

beberlei commented Mar 30, 2013

Can you remove the code duplication, extract the code into their own methods and try to add a test?

Contributor

Padam87 commented Mar 30, 2013

Fixed the code duplication, and created a unit test.

Ran into some trouble with the test, I had to use a real entity, couldn't create a traited entity from ClassMetadata.

@Majkl578 Majkl578 commented on an outdated diff Mar 31, 2013

lib/Doctrine/ORM/Tools/EntityGenerator.php
@@ -741,6 +755,26 @@ protected function hasMethod($method, ClassMetadataInfo $metadata)
/**
* @param ClassMetadataInfo $metadata
*
+ * @return array
+ */
+ protected function getTraits(ClassMetadataInfo $metadata)
+ {
+ if ($metadata->reflClass != null || class_exists($metadata->name)) {
+ $reflClass = $metadata->reflClass == null
+ ? new \ReflectionClass($metadata->name)
+ : $metadata->reflClass;
+
+ if (method_exists($reflClass, 'getTraits')) {
@Majkl578

Majkl578 Mar 31, 2013

Contributor

Personally, I'd prefer this check:

if (PHP_VERSION_ID >= 50400) {

@stof stof commented on an outdated diff Apr 4, 2013

lib/Doctrine/ORM/Tools/EntityGenerator.php
@@ -741,6 +755,26 @@ protected function hasMethod($method, ClassMetadataInfo $metadata)
/**
* @param ClassMetadataInfo $metadata
*
+ * @return array
+ */
+ protected function getTraits(ClassMetadataInfo $metadata)
+ {
+ if ($metadata->reflClass != null || class_exists($metadata->name)) {
@stof

stof Apr 4, 2013

Member

this should be a strict comparison.

@stof stof commented on an outdated diff Apr 4, 2013

lib/Doctrine/ORM/Tools/EntityGenerator.php
@@ -741,6 +755,26 @@ protected function hasMethod($method, ClassMetadataInfo $metadata)
/**
* @param ClassMetadataInfo $metadata
*
+ * @return array
+ */
+ protected function getTraits(ClassMetadataInfo $metadata)
+ {
+ if ($metadata->reflClass != null || class_exists($metadata->name)) {
+ $reflClass = $metadata->reflClass == null
@stof

stof Apr 4, 2013

Member

same here

Owner

beberlei commented Apr 6, 2013

Why did you put code into sandbox?

Contributor

Padam87 commented Apr 6, 2013

@beberlei Wasn't sure where to put the entities for the test, and that seemed to be the place you guys have used. Where should I put them?

(Could not create an entity from class metadata, like the rest of the tests do, because of the trait.)

Member

stof commented Apr 6, 2013

stof referenced this pull request in Atlantic18/DoctrineExtensions Apr 13, 2013

Closed

traits issue with doctrine:generate:entities #668

@Koc Koc and 1 other commented on an outdated diff Apr 13, 2013

tests/Doctrine/Tests/ORM/Tools/EntityGeneratorTest.php
@@ -452,6 +454,34 @@ public function testEntityTypeAlias(array $field)
}
/**
+ * @group DDC-2372
+ */
+ public function testTraitPropertiesAndMethodsAreNotDuplicated()
+ {
+ if (PHP_VERSION_ID >= 50400) {
@Koc

Koc Apr 13, 2013

Contributor

IMHO better mark test skipped in other case

@stof

stof Apr 13, 2013

Member

indeed

Contributor

Padam87 commented May 31, 2013

up

jmonday commented Jun 13, 2013

Hello all. I was just wondering if anyone had any updates on this. I just ran into this issue tonight. Thanks!

@Ocramius Ocramius commented on an outdated diff Jun 13, 2013

lib/Doctrine/ORM/Tools/EntityGenerator.php
@@ -741,6 +755,26 @@ protected function hasMethod($method, ClassMetadataInfo $metadata)
/**
* @param ClassMetadataInfo $metadata
*
+ * @return array
+ */
+ protected function getTraits(ClassMetadataInfo $metadata)
+ {
+ if ($metadata->reflClass !== null || class_exists($metadata->name)) {
+ $reflClass = $metadata->reflClass === null
+ ? new \ReflectionClass($metadata->name)
+ : $metadata->reflClass;
+
+ if (PHP_VERSION_ID >= 50400) {
@Ocramius

Ocramius Jun 13, 2013

Owner

You can move this check at the beginning of the method and return early

Contributor

Padam87 commented Jul 7, 2013

@Ocramius @beberlei Can you merge this? Really annoying bug... :) Do I need to modify something?

@guilhermeblanco guilhermeblanco added a commit that referenced this pull request Jul 7, 2013

@guilhermeblanco guilhermeblanco Merge pull request #632 from Padam87/entgentrait
entity generator - ignore trait properties and methods
78fc129

@guilhermeblanco guilhermeblanco merged commit 78fc129 into doctrine:master Jul 7, 2013

1 check passed

default The Travis CI build passed
Details

I just tested this and it definitely helped fix my issue of classes with traits having the trait properties and methods copied into their body.

However, any subclasses of that class still get the properties and methods copied into them. This causes null value errors when saving child entities unless you manually remove the properties and methods from the child class.

Can anyone else confirm this?

Contributor

Padam87 commented Aug 20, 2013

@AlexanderParker You are right, in that case, the problem still exists. On it.

@Padam87, your fix in #763 seems to have worked. Thanks for looking at this so quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment