[DoctrineBridge] Add decimal form type #24793
Conversation
|
I have found a new case that was not handled correctly by my first implementation and that was not detected in the tests because it depends on the database platform. Fields mapped as "decimal" will always be returned as string. But on some platform the values are rounded when the number of decimals is inferior to the "scale" option. Eg with inserted float value 8.000 (scale 3) :
My solution is to add an option to the new form type to correctly format the number in the model transformer.
|
|
@xabbuh @stof @webmozart maybe you can help moving forward here? |
| private $scale; | ||
|
|
||
| /** | ||
| * @param bool $forceFullScale |
| } | ||
|
|
||
| if (!is_string($value)) { | ||
| throw new TransformationFailedException('Expected a string.'); |
There was a problem hiding this comment.
Should probably be reflected in the @param
| try { | ||
| return (string) $value; | ||
| } catch (\Exception $e) { | ||
| throw new TransformationFailedException(); |
There was a problem hiding this comment.
Previous exception should be passed on here
|
|
||
| try { | ||
| return (string) $value; | ||
| } catch (\Exception $e) { |
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function buildForm(FormBuilderInterface $builder, array $options) |
There was a problem hiding this comment.
force_full_scale option to be documented above
|
About #24793 (comment), I just discussed it with @webmozart and it is not fixable in There is a solution, which is making the If the whole issue drills down to avoiding superfluous updates at DB level, I don't think that problem should be tackled here, and instead a change in |
|
Working on finishing this as part of EUFOSSA. |
|
I agree that Doctrine should return coherent values between platforms for this case. I think we can just use my initial commit with the new Form type, it will still be better than now and will be better once Doctrine is ready. @webmozart Do you mean you are gonna work on a fix on Doctrine side? |
|
Closing for #30893. |
|
@webmozart No, I provided some thoughts on how to implement this in #30893 though. :) |
…chussek) This PR was merged into the 4.3-dev branch. Discussion ---------- Add "input" option to NumberType | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24793 | License | MIT | Doc PR | TODO This PR replaces #24793 in (partially) fixing how Doctrine's DECIMAL type is handled by the Form component. Previously, DECIMAL was mapped to the regular NumberType. That confuses Doctrine's change detection, depending on the DB platform. Examples: | DB | DB value | Doctrine entity before submit | Form input | Doctrine entity after submit | --- | --- | --- | --- | --- | SQLite | 8.000 | '8' | 8 | 8 | SQLite | 8.123 | '8.123' | 8.123 | 8.123 | PostgreSQL | 8.000 | '8.000' | 8 | 8 | PostgreSQL | 8.123 | '8.123' | 8.123 | 8.123 The value in the Doctrine entity changes before and after submit. Hence Doctrine believes an update is necessary. This PR introduces an `input` option to NumberType (similar to DateType), that can be set to `'number'` (default) or `'string'`. If set to `'string'`, the conversion is as follows: | DB | DB value | Doctrine entity before submit | Form input | Doctrine entity after submit | --- | --- | --- | --- | --- | SQLite | 8.000 | **'8'** | 8 | **'8.000'** | SQLite | 8.123 | '8.123' | 8.123 | '8.123' | PostgreSQL | 8.000 | '8.000' | 8 | '8.000' | PostgreSQL | 8.123 | '8.123' | 8.123 | '8.123' You see that this does not completely solve this issue for SQLite. However, @Ocramius and I agree that this is something to be fixed by Doctrine, since Doctrine is providing the database abstraction. That fix should be done in the SqlitePlatform object in Doctrine as part of another PR and should be backwards compatible with the current Doctrine version (i.e. opt in via configuration). Compared to #24793, this PR does not introduce a new type but instead makes the NumberType more flexible. Also, this PR does not introduce the `force_full_scale` option since that should be solved by Doctrine as described above. Commits ------- 3f25734 Added new option "input" to NumberType fb2b37a add force_full_scale option to handle all cases 40f2512 [DoctrineBridge] Add decimal form type
|
Was an issue opened on Doctrine side to track this problem? |
|
@fancyweb Not yet, I'd appreciate if you could do so! |
Currently, when an entity field is mapped as a
decimal, the guessed form type for this field is theNumberType. I guess it's also the one that comes naturally to everyone's mind.The
NumberTypeuses the view transformerNumberToLocalizedStringTransformerwhich always transform the submitted string value to an int or a float.As stated in the Doctrine documentation for the
decimaltype :So when you use the
NumberType, the string value of yourdecimalfield is replaced by its float value. The big issue is that this change of type if detected when Doctrine computes the change sets (strict comparison is done ofc). That mean a lot of unnecessary updates are made.The new form type I introduce simply extends the
NumberTypebut it transform the value back to a string (the model value).