Adds metadata field type validation against Entity property type#10662
Adds metadata field type validation against Entity property type#10662DavideBicego wants to merge 4 commits into
Conversation
|
Note: potentially worth going to |
| /** | ||
| * @Column(type="boolean") | ||
| */ | ||
| protected int $property; |
There was a problem hiding this comment.
Ooooops, the test fails here because Doctrine didn't yet drop PHP 7.3 support (REALLY?!)
I think the best way to handle this would be to:
- split this out to a separate file
- mark the tests with
@requires php>=7.4 - exclude the separated file from analysis by tooling like phpcs/phpstan
There was a problem hiding this comment.
How would you prevent the file from being read with php < 7.4?
The test should not be executed with a @requires php 70400 but the file contains some classes definition too...
There was a problem hiding this comment.
I don't remember how the test suite works precisely, but can check it tomorrow.
I'd say something like require_once __DIR__ . '/some-php-7.4-only-file.php and then reference the classes in that file that way
There was a problem hiding this comment.
Or use autoload, or team up with @MatTheCat who is having the same issue on https://github.com/doctrine/orm/pull/10666/files#r1186698321
We could have a subdirectories with tests that require a specific version of PHP, and then future contributors would not need to bother with @requires or with splitting test files and companion classes.
Features welcome, and indeed, this should target 2.15.x (2.15.0 should be released quite soon BTW) |
|
For some reason the CI step "PHPUnit with SQLite (7.1)" ignores the PHP version requirement... |
|
CI is also stuck due to https://www.githubstatus.com/ |
| SmallIntType::class => 'int', | ||
| StringType::class => 'string', | ||
| TextType::class => 'string', | ||
| ]; |
There was a problem hiding this comment.
What I don't like about this PR is this harcoded map, but for now there is nothing better I'm afraid. Once doctrine/dbal 4 is published and supported, we can start using reflection to get the return type.
|
Triggered the CI again |
| use Doctrine\ORM\Tools\SchemaValidator; | ||
| use Doctrine\Tests\OrmTestCase; | ||
|
|
||
| /** @requires PHP >= 7.4 */ |
There was a problem hiding this comment.
Other occurrences of this in the project are multiline. Maybe old versions of PHPUnit ignore single line stuff. The next suspect will be >=
There was a problem hiding this comment.
Ah I can't push… push from maintainers is disabled. I'll open another PR I guess.
|
Closing in favor of #10946 |
3 participants
This adds some properties type validation to
SchemaValidator.I think some strictness when setting the Entity value during Hydration would be better but
ReflectionProperty::setValueskips strictness validation.Resolve #10661