-
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
Allow using objects as discriminators #6141
Conversation
85da025
to
f5c899d
Compare
{ | ||
parent::setUp(); | ||
|
||
Type::addType(GH6141PeopleType::NAME, __NAMESPACE__ . '\\GH6141PeopleType'); |
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.
Can you please use ::class
here?
Type::addType(GH6141PeopleType::NAME, __NAMESPACE__ . '\\GH6141PeopleType'); | ||
|
||
$this->_schemaTool->createSchema(array( | ||
$this->_em->getClassMetadata(__NAMESPACE__ . '\\GH6141Person'), |
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.
same
|
||
$this->_schemaTool->createSchema(array( | ||
$this->_em->getClassMetadata(__NAMESPACE__ . '\\GH6141Person'), | ||
$this->_em->getClassMetadata(__NAMESPACE__ . '\\GH6141Boss'), |
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.
same
$this->_schemaTool->createSchema(array( | ||
$this->_em->getClassMetadata(__NAMESPACE__ . '\\GH6141Person'), | ||
$this->_em->getClassMetadata(__NAMESPACE__ . '\\GH6141Boss'), | ||
$this->_em->getClassMetadata(__NAMESPACE__ . '\\GH6141Employee'), |
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.
same
)); | ||
} | ||
|
||
public function testEnumDiscriminatorsShouldBeConvertedToString() |
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.
One last thing to add to this test: asserting that the discriminator is an object.
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.
How would I do that? The retrieved object doesn't have the discriminator and the entity metadata contains it as a string
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.
Probably via metadata and $metadata->getDiscriminatorValue()
, or by hydrating a row as array.
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.
done 😉
$this->_em->flush(); | ||
$this->_em->clear(); | ||
|
||
$query = $this->_em->createQueryBuilder() |
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.
findOneBy()
instead - simpler here, as we don't go through the entire DQL scenarios
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.
With that findOneBy()
would use the SimpleObjectHydrator
instead of the ObjectHydrator
.
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.
Can you add a comment about that then?
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.
Will do
return new self($value); | ||
} | ||
|
||
public static function isValid($valid) |
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.
Remove or make private, since it's not used in the examples
|
||
private $value; | ||
|
||
public static function get($value) |
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.
Docblocks for types here (methods/properties)
* @InheritanceType("JOINED") | ||
* @DiscriminatorColumn(name="discr", type="gh6141people") | ||
* @DiscriminatorMap({ | ||
* "boss" = "Doctrine\Tests\ORM\Functional\Ticket\GH6141Boss", |
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.
Can use GH6141Boss::class
here, instead of a string
* @DiscriminatorColumn(name="discr", type="gh6141people") | ||
* @DiscriminatorMap({ | ||
* "boss" = "Doctrine\Tests\ORM\Functional\Ticket\GH6141Boss", | ||
* "employee" = "Doctrine\Tests\ORM\Functional\Ticket\GH6141Employee" |
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.
Can use GH6141Employee::class
here, instead of a string
f5c899d
to
bc3d209
Compare
|
||
Type::addType(GH6141PeopleType::NAME, GH6141PeopleType::class); | ||
|
||
$this->_schemaTool->createSchema(array( |
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.
I'll just convert this to short-array notation
bc3d209
to
1b39cd8
Compare
Aaaaaaand merged! Thanks @lcobucci! 👍 |
@Ocramius was too eager to merge stuff Related to: doctrine#6141
@Ocramius this is the small change to make it work on
master
.The test passes without any change on
2.5-x
so it looks like we have a small break onmaster
which is fixed with this changeset.