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.

Sign up for GitHub

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

Jump to bottom

fixed EntityRegenerator when using MappedSuperclass or Traits #181

Merged
merged 2 commits into from
May 17, 2018
Merged

fixed EntityRegenerator when using MappedSuperclass or Traits #181

merged 2 commits into from
May 17, 2018

Conversation

andrewtch
Copy link
Contributor

@andrewtch andrewtch commented May 11, 2018 •
edited
Loading

Should fix #167

What has been fixed

  • generation for mapped inherited fields
  • generation for mapped fields that come from direct traits
  • skipping parent::__construct() if Entity has parent class and it has constructor

How it is done

  • fields (fields, and associations, but not embedded) are checked before generation, generation happens only for class' fields
  • ClassSourceManipulator is updated to include parent call (if parent class exists and the class has already been generated)
  • EntityRegeneratorTest skips copying of Traits as it causes autoloader to fail (again, strangely, on Travis only, works fine on Mac)

Verified on live project, seems to generate OK with a lot of different entities (MappedSuperclasses, DiscriminatorMaps etc).

  • fix errors generating embedded fields
  • fix strange Kernel class errors

@andrewtch
Copy link
Contributor Author

andrewtch commented May 12, 2018 •
edited
Loading

@dunglas , can you kindly remind how did you debug these kind of errors? I am again having 18 failed tests on Mac, zero in AppVeyour and 2 on Travis.

Upd: Sorry, nevermind - got out of composer github calls white testing (as the bundle installs flex internally)

weaverryan
weaverryan reviewed May 13, 2018
View reviewed changes
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Wow! This is awesome! And your test is SWEET! I have one nit-pick comment and then one question. But, thanks to the test and the clean code, I have a lot of confidence in this. :)

src/Doctrine/EntityRegenerator.php Outdated
return $classReflection->hasProperty($field) &&
$classReflection->getProperty($field)->getDeclaringClass()->getName() == $classReflection->getName();
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's refactor this into a private method, just for clarity - like getMappedFieldsInEntity().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved, variable renamed to "$mappedFields"

tests/Doctrine/EntityRegeneratorTest.php Outdated

$directoryIterator = new \RecursiveDirectoryIterator($sourceDir, \FilesystemIterator::SKIP_DOTS);
$filter = new AllButTraitsIterator($directoryIterator);
$iterator = new \RecursiveIteratorIterator($filter, \RecursiveIteratorIterator::SELF_FIRST);
Copy link
Member

Choose a reason for hiding this comment

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

Can you walk me through this? You don't want the traits copied into the source directory? This would mean that when we execute the regenerator, the entities are referencing non-existent traits? I'm sure I'm missing the point :)

Or, were you doing this simply to avoid us needing to duplicate them in the expected_overwrite and expected_no_overwrite directories? If so... hmm... ok, that makes sense. Let's add some comments about that

Copy link
Contributor Author

@andrewtch andrewtch May 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Sure! If you don't use the iterator, the ProxyCacheWarmer will include the Traits multiple times and this will lead to autoloader error:

./vendor/bin/simple-phpunit tests/Doctrine/EntityRegeneratorTest.php 
PHPUnit 6.5.8 by Sebastian Bergmann and contributors.

Testing Symfony\Bundle\MakerBundle\Tests\Doctrine\EntityRegeneratorTest
PHP Fatal error:  Cannot declare trait Symfony\Bundle\MakerBundle\Tests\Doctrine\fixtures\source_project\src\Entity\TeamTrait, because the name is already in use in /Users/andrewtch/Documents/Work/maker-bundle/tests/tmp/current_project/src/Entity/TeamTrait.php on line 7
PHP Stack trace:
PHP   1. {main}() /Users/andrewtch/Documents/Work/maker-bundle/vendor/symfony/phpunit-bridge/bin/simple-phpunit:0
PHP   2. include() /Users/andrewtch/Documents/Work/maker-bundle/vendor/symfony/phpunit-bridge/bin/simple-phpunit:251
PHP   3. PHPUnit\TextUI\Command::main() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/phpunit:17
PHP   4. Symfony\Bridge\PhpUnit\Legacy\CommandForV6->run() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/TextUI/Command.php:148
PHP   5. Symfony\Bridge\PhpUnit\Legacy\TestRunnerForV6->doRun() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/TextUI/Command.php:195
PHP   6. PHPUnit\Framework\TestSuite->run() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/TextUI/TestRunner.php:546
PHP   7. PHPUnit\Framework\DataProviderTestSuite->run() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/Framework/TestSuite.php:755
PHP   8. Symfony\Bundle\MakerBundle\Tests\Doctrine\EntityRegeneratorTest->run() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/Framework/TestSuite.php:755
PHP   9. PHPUnit\Framework\TestResult->run() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/Framework/TestCase.php:895
PHP  10. Symfony\Bundle\MakerBundle\Tests\Doctrine\EntityRegeneratorTest->runBare() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/Framework/TestResult.php:698
PHP  11. Symfony\Bundle\MakerBundle\Tests\Doctrine\EntityRegeneratorTest->runTest() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/Framework/TestCase.php:940
PHP  12. ReflectionMethod->invokeArgs() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/Framework/TestCase.php:1072
PHP  13. Symfony\Bundle\MakerBundle\Tests\Doctrine\EntityRegeneratorTest->testRegenerateEntities() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/Framework/TestCase.php:1072
PHP  14. Symfony\Bundle\MakerBundle\Tests\Doctrine\EntityRegeneratorTest->doTestRegeneration() /Users/andrewtch/Documents/Work/maker-bundle/tests/Doctrine/EntityRegeneratorTest.php:38
PHP  15. Symfony\Bundle\MakerBundle\Tests\Doctrine\TestEntityRegeneratorKernel->boot() /Users/andrewtch/Documents/Work/maker-bundle/tests/Doctrine/EntityRegeneratorTest.php:79
PHP  16. Symfony\Bundle\MakerBundle\Tests\Doctrine\TestEntityRegeneratorKernel->initializeContainer() /Users/andrewtch/Documents/Work/maker-bundle/vendor/symfony/http-kernel/Kernel.php:125
PHP  17. Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerAggregate->warmUp() /Users/andrewtch/Documents/Work/maker-bundle/vendor/symfony/http-kernel/Kernel.php:556
PHP  18. Symfony\Bridge\Doctrine\CacheWarmer\ProxyCacheWarmer->warmUp() /Users/andrewtch/Documents/Work/maker-bundle/vendor/symfony/http-kernel/CacheWarmer/CacheWarmerAggregate.php:48
PHP  19. Doctrine\ORM\Mapping\ClassMetadataFactory->getAllMetadata() /Users/andrewtch/Documents/Work/maker-bundle/vendor/symfony/doctrine-bridge/CacheWarmer/ProxyCacheWarmer.php:64
PHP  20. Doctrine\Common\Persistence\Mapping\Driver\MappingDriverChain->getAllClassNames() /Users/andrewtch/Documents/Work/maker-bundle/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:114
PHP  21. Doctrine\ORM\Mapping\Driver\AnnotationDriver->getAllClassNames() /Users/andrewtch/Documents/Work/maker-bundle/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/Driver/MappingDriverChain.php:128
PHP  22. require_once() /Users/andrewtch/Documents/Work/maker-bundle/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/Driver/AnnotationDriver.php:236

Fatal error: Cannot declare trait Symfony\Bundle\MakerBundle\Tests\Doctrine\fixtures\source_project\src\Entity\TeamTrait, because the name is already in use in /Users/andrewtch/Documents/Work/maker-bundle/tests/tmp/current_project/src/Entity/TeamTrait.php on line 7

Call Stack:
    0.0012     445552   1. {main}() /Users/andrewtch/Documents/Work/maker-bundle/vendor/symfony/phpunit-bridge/bin/simple-phpunit:0
    0.0189     453760   2. include('/Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/phpunit') /Users/andrewtch/Documents/Work/maker-bundle/vendor/symfony/phpunit-bridge/bin/simple-phpunit:251
    0.0298     958744   3. PHPUnit\TextUI\Command::main() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/phpunit:17
    0.0298     958856   4. Symfony\Bridge\PhpUnit\Legacy\CommandForV6->run() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/TextUI/Command.php:148
    0.0781    2994616   5. Symfony\Bridge\PhpUnit\Legacy\TestRunnerForV6->doRun() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/TextUI/Command.php:195
    0.0904    3204544   6. PHPUnit\Framework\TestSuite->run() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/TextUI/TestRunner.php:546
    0.0970    3225752   7. PHPUnit\Framework\DataProviderTestSuite->run() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/Framework/TestSuite.php:755
    0.0971    3225944   8. Symfony\Bundle\MakerBundle\Tests\Doctrine\EntityRegeneratorTest->run() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/Framework/TestSuite.php:755
    0.0971    3225944   9. PHPUnit\Framework\TestResult->run() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/Framework/TestCase.php:895
    0.0990    3247728  10. Symfony\Bundle\MakerBundle\Tests\Doctrine\EntityRegeneratorTest->runBare() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/Framework/TestResult.php:698
    0.0993    3264448  11. Symfony\Bundle\MakerBundle\Tests\Doctrine\EntityRegeneratorTest->runTest() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/Framework/TestCase.php:940
    0.0993    3265064  12. ReflectionMethod->invokeArgs() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/Framework/TestCase.php:1072
    0.0993    3265096  13. Symfony\Bundle\MakerBundle\Tests\Doctrine\EntityRegeneratorTest->testRegenerateEntities() /Users/andrewtch/Documents/Work/maker-bundle/vendor/bin/.phpunit/phpunit-6.5/src/Framework/TestCase.php:1072
    0.0994    3265392  14. Symfony\Bundle\MakerBundle\Tests\Doctrine\EntityRegeneratorTest->doTestRegeneration() /Users/andrewtch/Documents/Work/maker-bundle/tests/Doctrine/EntityRegeneratorTest.php:38
    0.1355    3410136  15. Symfony\Bundle\MakerBundle\Tests\Doctrine\TestEntityRegeneratorKernel->boot() /Users/andrewtch/Documents/Work/maker-bundle/tests/Doctrine/EntityRegeneratorTest.php:79
    0.1401    3712120  16. Symfony\Bundle\MakerBundle\Tests\Doctrine\TestEntityRegeneratorKernel->initializeContainer() /Users/andrewtch/Documents/Work/maker-bundle/vendor/symfony/http-kernel/Kernel.php:125
    0.8983   11306832  17. Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerAggregate->warmUp() /Users/andrewtch/Documents/Work/maker-bundle/vendor/symfony/http-kernel/Kernel.php:556
    0.9353   12007920  18. Symfony\Bridge\Doctrine\CacheWarmer\ProxyCacheWarmer->warmUp() /Users/andrewtch/Documents/Work/maker-bundle/vendor/symfony/http-kernel/CacheWarmer/CacheWarmerAggregate.php:48
    0.9929   13763840  19. Doctrine\ORM\Mapping\ClassMetadataFactory->getAllMetadata() /Users/andrewtch/Documents/Work/maker-bundle/vendor/symfony/doctrine-bridge/CacheWarmer/ProxyCacheWarmer.php:64
    0.9929   13763840  20. Doctrine\Common\Persistence\Mapping\Driver\MappingDriverChain->getAllClassNames() /Users/andrewtch/Documents/Work/maker-bundle/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:114
    0.9929   13764096  21. Doctrine\ORM\Mapping\Driver\AnnotationDriver->getAllClassNames() /Users/andrewtch/Documents/Work/maker-bundle/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/Driver/MappingDriverChain.php:128
    0.9938   13776928  22. require_once('/Users/andrewtch/Documents/Work/maker-bundle/tests/tmp/current_project/src/Entity/TeamTrait.php') /Users/andrewtch/Documents/Work/maker-bundle/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/Driver/AnnotationDriver.php:236

The issue is that Entities are resolved in current testing namespace (e.g. tmp/current_project), but the Traits are still referenced by original namespace fixtures\source_prohect.... There are two ways to fix this - either rewriting namespaces after copying the traits (not sure it would be a simple task) or just not copying them at all (as they will be autoloaded from source_project namespace/directory), with the second being most straight-forward approach.

I will add a comment explaining the reasons behind this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commented the behaviour, moved to separate function for readability.

@andrewtch
Copy link
Contributor Author

andrewtch commented May 16, 2018 •
edited
Loading

@weaverryan anything else to change before you can merge this (and before it gets some conflicts with base master :))?

@weaverryan
Copy link
Member

Nope! This is ready. Thank you so much Andrew! I'm merging a few other PR's, but hope to get a release very shortly.

@weaverryan weaverryan merged commit b869a5c into symfony:master May 17, 2018
weaverryan added a commit that referenced this pull request May 17, 2018
@weaverryan
bug #181 fixed EntityRegenerator when using MappedSuperclass or Trait…
5927f01 
…s (andrewtch, weaverryan)

This PR was squashed before being merged into the 1.0-dev branch (closes #181).

Discussion
----------

fixed EntityRegenerator when using MappedSuperclass or Traits

Should fix #167

**What has been fixed**

- generation for mapped inherited fields
- generation for mapped fields that come from direct traits
- skipping ```parent::__construct()``` if Entity has parent class and it has constructor

**How it is done**

- fields (fields, and associations, but not embedded) are checked before generation, generation happens only for class' fields
- ClassSourceManipulator is updated to include parent call (if parent class exists and the class has already been generated)
- EntityRegeneratorTest skips copying of Traits as it causes autoloader to fail (again, strangely, on Travis only, works fine on Mac)

Verified on live project, seems to generate OK with a lot of different entities (MappedSuperclasses, DiscriminatorMaps etc).

- [x] fix errors generating embedded fields
- [x] fix strange Kernel class errors

Commits
-------

b869a5c Merge branch 'master' into fix-entity-regeneration-traits-ms
e6c640b fixed EntityRegenerator when using MappedSuperclass or Traits
@andrewtch
Copy link
Contributor Author

Welcome! Any other bug I can help with while my fork is still up to date?

@weaverryan
Copy link
Member

Not that I know of, fortunately :). But, I do have some ideas for our next command: make:user. That will be a decent amount of work

@andrewtch andrewtch mentioned this pull request Feb 9, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weaverryan weaverryan weaverryan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

make:entity --regenerate and classes using traits
2 participants
@andrewtch @weaverryan