-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
entity generator - ignore trait properties and methods #632
Conversation
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: |
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. |
if (class_exists($metadata->name)) { | ||
$reflClass = new \ReflectionClass($metadata->name); | ||
|
||
foreach ($reflClass->getTraits() as $trait) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't bump the requirement to 5.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, forgot about that. I will add an if (method_exists($reflClass, 'getTraits'))
This should be tested |
@pamil You're welcome :) Sorry guys, i don't have the time right now, but I will create some unit test this weekend. |
Can you remove the code duplication, extract the code into their own methods and try to add a test? |
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. |
? new \ReflectionClass($metadata->name) | ||
: $metadata->reflClass; | ||
|
||
if (method_exists($reflClass, 'getTraits')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'd prefer this check:
if (PHP_VERSION_ID >= 50400) {
Why did you put code into |
@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.) |
@Padam87 the testsuite already contains lots of models: https://github.com/doctrine/doctrine2/tree/master/tests/Doctrine/Tests/Models |
*/ | ||
public function testTraitPropertiesAndMethodsAreNotDuplicated() | ||
{ | ||
if (PHP_VERSION_ID >= 50400) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO better mark test skipped in other case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed
up |
Hello all. I was just wondering if anyone had any updates on this. I just ran into this issue tonight. Thanks! |
? new \ReflectionClass($metadata->name) | ||
: $metadata->reflClass; | ||
|
||
if (PHP_VERSION_ID >= 50400) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move this check at the beginning of the method and return early
entity generator - ignore trait properties and methods
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? |
@AlexanderParker You are right, in that case, the problem still exists. On it. |
Fixes:
DDC-1825
DDC-2154