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

Fix: Do not persist embeddables #44

Merged
merged 2 commits into from
Mar 7, 2020
Merged

Fix: Do not persist embeddables #44

merged 2 commits into from
Mar 7, 2020

Conversation

localheinz
Copy link
Contributor

@localheinz localheinz commented Mar 5, 2018 •
edited
Loading

This PR

  • asserts that embeddables are not persisted when automated persisting is turned on
  • skips persisting embeddables

💁‍♂️ Not sure, but I think it would be nice if we could write definitions for embeddables as well. When turning automatic persisting on, the fixture factory attempts to persist them, though, and this will fail because they do not have an identity.

@localheinz localheinz force-pushed the fix/embeddable branch 3 times, most recently from 151a9b6 to f853cb5 Compare March 5, 2018 17:29
localheinz
localheinz commented Mar 5, 2018
View reviewed changes
src/Provider/Doctrine/FixtureFactory.php Outdated

if ($this->persist) {

if ($this->persist && (!property_exists($entityMetadata, 'isEmbeddedClass') || false === $entityMetadata->isEmbeddedClass)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is too verbose, but the field may not yet exist, so I better check for its existence.

For reference, see

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure, that's ok

@localheinz
Copy link
Contributor Author

No idea why most of the tests are failing, to be honest.

@glaubinix
Copy link
Collaborator

I never really used embeddables so I might be not of much help here. Build failures is definitely related to the changes in this branch. I just rerun the build on latest master and everything looks ok.

You might want to temporarily disable --testdox in the travis yml to get some more info.

@localheinz localheinz force-pushed the fix/embeddable branch 5 times, most recently from b38fed0 to d344f04 Compare March 7, 2018 08:58
@localheinz
Copy link
Contributor Author

@glaubinix

Ha, build failure was caused by

FactoryGirl\Tests\Provider\Doctrine\Fixtures\BasicUsageTest::acceptsConstantValuesInEntityDefinitions
constant(): Couldn't find constant Doctrine\ORM\Mapping\ClassMetadata::GENERATOR_TYPE_UUID
/home/travis/build/breerly/factory-girl-php/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php:323
/home/travis/build/breerly/factory-girl-php/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:293
/home/travis/build/breerly/factory-girl-php/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:178
/home/travis/build/breerly/factory-girl-php/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:131
/home/travis/build/breerly/factory-girl-php/tests/Provider/Doctrine/TestDb.php:74
/home/travis/build/breerly/factory-girl-php/tests/Provider/Doctrine/TestDb.php:63
/home/travis/build/breerly/factory-girl-php/tests/Provider/Doctrine/Fixtures/TestCase.php:43

@localheinz localheinz force-pushed the fix/embeddable branch 2 times, most recently from 201e827 to c86b6fa Compare March 7, 2018 09:37
@localheinz
Copy link
Contributor Author

@glaubinix

Build failure is related to doctrine/orm#6462. Taking a look later.

@localheinz
Copy link
Contributor Author

Just realized that this is also a problem when persistOnGet(false) has been invoked.

@localheinz localheinz force-pushed the fix/embeddable branch 4 times, most recently from 12e9f8a to de6d113 Compare March 5, 2020 21:51
@localheinz localheinz force-pushed the fix/embeddable branch 4 times, most recently from 52f3d5c to f85f34c Compare March 5, 2020 21:56
glaubinix
glaubinix reviewed Mar 6, 2020
View reviewed changes
src/Provider/Doctrine/FixtureFactory.php Outdated
$this->em->persist($ent);
}

return $ent;
}

private static function canPersist(Mapping\ClassMetadata $classMetadata): bool
{
if (!property_exists($classMetadata, 'isEmbeddedClass')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This library requires doctrine/orm 2.6.3. The property should be available in all orm versions since then: https://github.com/doctrine/orm/blob/v2.6.3/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php#L270

@glaubinix glaubinix merged commit 0e6f1b6 into HelloGrayson:master Mar 7, 2020
@glaubinix
Copy link
Collaborator

Thank you!

@localheinz localheinz deleted the fix/embeddable branch March 7, 2020 13:54
@localheinz
Copy link
Contributor Author

Thank you, @glaubinix!

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

@glaubinix glaubinix glaubinix approved these changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@localheinz @glaubinix