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

Fix #6460 - \Doctrine\ORM\Mapping\ClassMetadataInfo::hasField should return true for embedded fields #6462

Merged
merged 2 commits into from Jun 1, 2017

Conversation

@mkosiedowski
Copy link
Contributor

mkosiedowski commented May 22, 2017

This fixes #6460

…return true for embedded fields
$this->_em->getClassMetadata(DDC6460ParentEntity::class),
]
);
} catch (\Exception $e) {

This comment has been minimized.

Copy link
@Ocramius

Ocramius May 22, 2017

Member

Please only catch schemaexception

->getClassMetadata(DDC6460Entity::class)
->hasField('embedded');
$this->assertTrue($isFieldMapped);

This comment has been minimized.

Copy link
@Ocramius

Ocramius May 22, 2017

Member

This test should probably be on the class metadata tests

This comment has been minimized.

Copy link
@mkosiedowski

mkosiedowski May 22, 2017

Author Contributor

I've added similar test there

$proxy = $this->_em->getRepository(DDC6460ParentEntity::class)->findOneById(1);
$this->assertNotNull($proxy->lazyLoaded->embedded);

This comment has been minimized.

Copy link
@Ocramius

Ocramius May 22, 2017

Member

Please use self::assertInstanceOf()

This comment has been minimized.

Copy link
@Ocramius

Ocramius May 22, 2017

Member

I would also check the value of field after this call

$second->id = 1;
$second->lazyLoaded = $entity;
$this->_em->persist($second);
$this->_em->flush();

This comment has been minimized.

Copy link
@Ocramius

Ocramius May 22, 2017

Member

Why two flush operations?

This comment has been minimized.

Copy link
@mkosiedowski

mkosiedowski May 22, 2017

Author Contributor

I've added cascade persist in order to remove one flush

This comment has been minimized.

Copy link
@Ocramius

Ocramius Jun 1, 2017

Member

Thanks!

$this->_em->clear();
$proxy = $this->_em->getRepository(DDC6460ParentEntity::class)->findOneById(1);

This comment has been minimized.

Copy link
@Ocramius

Ocramius May 22, 2017

Member

This is not a proxy here: please add assertions to verify if it is a proxy, and if it is loaded

$second->id = 1;
$second->lazyLoaded = $entity;
$this->_em->persist($second);
$this->_em->flush();

This comment has been minimized.

Copy link
@Ocramius

Ocramius Jun 1, 2017

Member

Thanks!

@Ocramius Ocramius added the Bug label Jun 1, 2017
@Ocramius Ocramius added this to the 2.6.0 milestone Jun 1, 2017
@Ocramius

This comment has been minimized.

Copy link
Member

Ocramius commented Jun 1, 2017

LGTM! 👍

Note that I'm not changing this for 2.5.x, because there's a subtle behavioral change when this API is used when looping over fields. It will land in 2.6.0

@Ocramius Ocramius self-assigned this Jun 1, 2017
@Ocramius Ocramius merged commit 971c400 into doctrine:master Jun 1, 2017
2 checks passed
2 checks passed
Scrutinizer 8 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@localheinz localheinz mentioned this pull request Mar 7, 2018
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.