Adds metadata field type validation against Entity property type #10946
Conversation
greg0ire
force-pushed
the
improved-validation
branch
from
8657bc6
to
860b1ee
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This avoid situations where you might map a decimal to a float, when it should really be mapped to a string. This is a big pitfall because in that situation, the ORM will consider the field changed every time.
greg0ire
force-pushed
the
improved-validation
branch
from
860b1ee
to
0f67ba2
Compare
| $ce = $this->validator->validateClass($class); | ||
|
|
||
| self::assertEquals( | ||
| ["The field 'Doctrine\Tests\ORM\Functional\Ticket\GH10661\InvalidEntity#property1' has the property type 'float' that differs from the metadata field type 'string' returned by the 'decimal' DBAL type."], |
There was a problem hiding this comment.
I added the "returned by the 'decimal' DBAL type" part, so that people understand where we get the second type from.
| /** | ||
| * @Column(type="decimal") | ||
| */ | ||
| protected float $property1; |
There was a problem hiding this comment.
I switched to decimal vs float here because I think it is a more common mistake, and it was the one I had in mind so I wanted to see how it looked for that case.
| @@ -304,4 +349,66 @@ public function getUpdateSchemaList(): array | |||
|
|
|||
| return $schemaTool->getUpdateSchemaSql($allMetadata, true); | |||
| } | |||
|
|
|||
| /** @return list<string> containing the found issues */ | |||
| private function validatePropertiesTypes(ClassMetadataInfo $class): array | |||
There was a problem hiding this comment.
We have no choice but to use ClassMetadataInfo here for now because of the calling code, and that should be fine, although I considered using if (PHP_VERSION_ID >= 70400 && $class instanceof ClassMetadata) {… that would be a bit weird.
There was a problem hiding this comment.
I'm not sure I understand this comment. Is this because of a possible different Persistence version and validateClass(ClassMetadataInfo $class)?
There was a problem hiding this comment.
I'm saying that in the unique call site of this method, we have no guarantee that we have a CM and not just a CMI (so yes, it does have to do with validateClass(ClassMetadataInfo $class). This explains why we use this type declaration here.
| SmallIntType::class => 'int', | ||
| StringType::class => 'string', | ||
| TextType::class => 'string', | ||
| ]; |
There was a problem hiding this comment.
IMO we should get rid of this harcoded map when DBAL v4 is out and switch to reflection on the DBAL type instead. The signatures will be typed.
There was a problem hiding this comment.
This list is intentional though: instanceof is NOT sufficient, because subclassing allows for types to be customized in userland.
If doctrine/dbal:^4 has final on all types, then we can use instanceof
There was a problem hiding this comment.
Ah, I understand what you mean now: convertToPhpType(): T, so we look at T?
There was a problem hiding this comment.
Yes. It could go in a property that is wider than T, or even narrower but that's kind of dangerous I'd say. At the very least, the intersection of both types must not be empty.
|
ping @derrabus @SenseException 🙂 |
|
Thanks @DavideBicego ! |
|
any recommendation for using type mixed ? For resource we used the type mixed, but unfortunatly php does not have type hint for resource therefore we used mixed. But now the validation gives the following error: |
|
@cuberinooo I was asked in #11076 to check for the mixed type and ignore it always, so it will be resolved when it get's merged (if get's). |
Copied from #10662
This avoid situations where you might map a decimal to a float, when it
should really be mapped to a string. This is a big pitfall because in
that situation, the ORM will consider the field changed every time.
Closes #10662 , closes #10661