Skip to content
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

Add "input" option to NumberType #30893

Merged
merged 3 commits into from Apr 6, 2019

Conversation

@webmozart
Copy link
Contributor

commented Apr 6, 2019

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.

@chalasr

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Can you add a note in the Bridge CHANGELOG?

@webmozart webmozart force-pushed the webmozart:number-type-input branch from c289645 to 85baf46 Apr 6, 2019

@webmozart webmozart requested a review from xabbuh as a code owner Apr 6, 2019

@webmozart

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

@chalasr Thanks for the heads up, done.

@webmozart webmozart force-pushed the webmozart:number-type-input branch from fc12044 to 735252b Apr 6, 2019

@chalasr chalasr added this to the next milestone Apr 6, 2019

@xabbuh

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

documentation PR added in symfony/symfony-docs#11314

@yceruto

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

Well, I guess that duplicated reviews will happen all the time, I'm sorry :)

@webmozart webmozart force-pushed the webmozart:number-type-input branch from 735252b to a99ad37 Apr 6, 2019

@webmozart webmozart force-pushed the webmozart:number-type-input branch from a99ad37 to 3f25734 Apr 6, 2019

@chalasr

chalasr approved these changes Apr 6, 2019

@fabpot

fabpot approved these changes Apr 6, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Thank you @webmozart.

@fabpot fabpot merged commit 3f25734 into symfony:master Apr 6, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 6, 2019

feature #30893 Add "input" option to NumberType (fancyweb, Bernhard S…
…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

wouterj added a commit to symfony/symfony-docs that referenced this pull request Apr 7, 2019

feature #11314 document the input option of the NumberType (xabbuh)
This PR was merged into the master branch.

Discussion
----------

document the input option of the NumberType

see symfony/symfony#30893

#FOSSHackathons #EUFOSSA

Commits
-------

c004dc5 document the input option of the NumberType

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.