[DoctrineBridge] Fix required guess of boolean fields #16302
Conversation
enumag
commented
Oct 20, 2015
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | |
| License | MIT |
| Doc PR |
|
Currently boolean fields of Doctrine entities are correctly guessed as checkboxes but incorrectly guessed as required. |
| @@ -95,7 +95,7 @@ public function guessRequired($class, $property) | |||
|
|
|||
| // Check whether the field exists and is nullable or not | |||
| if ($classMetadata->hasField($property)) { | |||
| if (!$classMetadata->isNullable($property)) { | |||
| if (!$classMetadata->isNullable($property) && $classMetadata->getTypeOfField($property) !== 'boolean') { | |||
There was a problem hiding this comment.
please use 'boolean' !== $classMetadata->getTypeOfField($property)
There was a problem hiding this comment.
actually also use the constant please: https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Types/Type.php#L40
There was a problem hiding this comment.
Right... Forgot about the coding style.
|
Please also refactor guessType to use DBAL constants. |
| @@ -51,28 +52,28 @@ public function guessType($class, $property) | |||
| } | |||
|
|
|||
| switch ($metadata->getTypeOfField($property)) { | |||
| case 'array': | |||
| case Type::TARRAY: | |||
There was a problem hiding this comment.
Should I add SIMPLE_ARRAY and JSON_ARRAY?
There was a problem hiding this comment.
Not sure if they work as collection form type as well. So lets do that in another PR if necessary.
| return new TypeGuess('checkbox', array(), Guess::HIGH_CONFIDENCE); | ||
| case 'datetime': | ||
| case Type::DATETIME: | ||
| case Type::DATETIMETZ: | ||
| case 'vardatetime': |
There was a problem hiding this comment.
Not sure why this is here. There doesn't seem to be a corresponding type in Doctrine.
There was a problem hiding this comment.
|
👍 Should be merged in 2.3 by merger! Status: Reviewed |
|
@Tobion Thanks! I refactored guessPattern and guessMaxLength to use the constants as well. |
|
Thank you @enumag. |
…mag) This PR was submitted for the 2.8 branch but it was merged into the 2.3 branch instead (closes #16302). Discussion ---------- [DoctrineBridge] Fix required guess of boolean fields | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Commits ------- b21d498 [DoctrineBridge] Fix required guess of boolean fields