I've noticed an anomaly in the validation of input for float and decimal field types.
If you set up a Float or Decimal field with default values (our dev system, where I discovered this, had a minimum value of 0, but that's the only non-default change I made), you will see that you if you enter a value with anything but numeric digits or decimal points, validation correctly rejects the input.
However, if you "accidentally" enter something like 1..5698
, in a "Decimal" field, the value is accepted, but stored/displayed as:
1.00
Entry of 1.5.698
results in
1.50<code> being saved... so entry is simply truncated at the second decimal. Where this could pose a problem is with European users on a site configured to use the decimal point (.) as the decimal separator instead of comma (,). Since they enter a point instead of a "thousands" separator, an accidental entry including a point twice would result in completely wrong values being stored (i.e. if the user remembers to use the point separator for the decimal, but still forgets and copies a value with a thousands (.) separator.)
Here: <code>1.569.63
(intended, perhaps, as 1569.63
is stored as 1.57
.
If you enter the same in a "Float" field, nothing is stored and the content is not saved; you will get an error something like this:
PDOException: SQLSTATE[01000]: Warning: 1265 Data truncated for column 'field_float_test_value' at row 1: INSERT INTO {field_data_field_float_test} (entity_type, entity_id, revision_id, bundle, delta, language, field_float_test_value) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6); Array ( [:db_insert_placeholder_0] => node [:db_insert_placeholder_1] => 6 [:db_insert_placeholder_2] => 6 [:db_insert_placeholder_3] => article [:db_insert_placeholder_4] => 0 [:db_insert_placeholder_5] => und [:db_insert_placeholder_6] => 1..5698 ) in field_sql_storage_field_storage_write() (line 425 of /home/lmontgomery/workspace/drupal7/modules/field/modules/field_sql_storage/field_sql_storage.module).
Steps to reproduce (tried in both 7.0 and dev)
1. Install Drupal with minimal settings and turn on the Field UI and Number modules.
2. Create a simple content type: Float Test
3. Add new field:
Label: Float Test
field name: field_float_test
Type: Float
Widget: Text Field
Field Settings: Decimal Point (decimal marker)
Minimum: 0
4. Add new field:
Label: Decimal Test
field name: field_decimal_test
Type: Decimal
Widget: Text Field
Field Settings: Decimal Point (decimal marker)
Minimum: 0
Attempt to create content (using one field or the other) and test... If you try only Decimal fields, you should see many variations of this result in data being stored, albeit probably not in the way the user meant. For most such content, you should see a similar SQL error to the one pasted above.
I'm attaching a patch which extends the number.test (found in modules/field/modules/number
) to more-or-less replicate the issue described (at least with "Decimal" values). This "patch" does not "fix" anything. It just provides other testers and more experienced core developers with a way to simply replicate at least some of this issue. (Perhaps I'll see if I can write a proper patch to fix the issue, but this is a start).
I've also attached the number.test file (already patched), in case that's more appropriate.
Comment | File | Size | Author |
---|---|---|---|
#19 | field_number-float_validation-1026878-19.patch | 3.04 KB | pillarsdotnet |
#18 | field_number-float_validation-1008402-18.patch | 3.04 KB | pillarsdotnet |
#7 | number-1026878-4.patch | 2.89 KB | LoMo |
#5 | number-1026878-3.patch | 2.91 KB | LoMo |
#3 | number-1026878-2.patch | 2.92 KB | LoMo |
Comments
Comment #1
LoMo CreditAttribution: LoMo commentedI think the attached patch should solve this issue... I'm a bit new to all this "patching core" stuff, so please look twice before you commit this issue. ;-) But I think things are working now.
Note that this patch includes both the changes to the numbers.test (which has been edited a bit more) and the number.module (where the validation takes place).
Comment #3
LoMo CreditAttribution: LoMo commentedOops. I made changes in one copy of the number.test and forgot to edit the CVS checkout version before patching.
This one should pass the tests (minor, but necessary changes that reflected some changes I made in the number.module)
Locally, this passes in Simpletest.
Comment #4
LoMo CreditAttribution: LoMo commentedComment #5
LoMo CreditAttribution: LoMo commentedMinor changes... removal of stray "white space" at ends of lines and other small coding standards compliance edits.
Comment #6
yched CreditAttribution: yched commentedMakes sense, I guess.
Patch looks fine, thanks ! A couple nitpicks :
Missing period at the end of the sentence. + comments need to wrap at 80 chars.
Error messages tend to standardize on a pattern like "%field_name: blah blah error"
Thanks for the tests ! But please inline the test code at the end of the existing testNumberDecimalField() method rather than in a helper function with a cryptic name :-)
Also, I'd say $wrong_entries probably sounds like a better variable name than $bad_entries ?
Comment #7
LoMo CreditAttribution: LoMo commented1) Edited comment to < 80 char wrap and ended with period. Thanks for the reminder. :-)
2) I've changed the error message, moved the test into your function with a short comment, and modified the variable names. The message with the field name at the end was similar to the one already used above. ;-)
Comment #8
LoMo CreditAttribution: LoMo commentedComment #10
yched CreditAttribution: yched commentedThanks - a couple more nitpicks and we're there :-)
Core tries to avoid "doesn't", "hasn't" forms in comments.
Maybe "Check that the separator does not appear more than once" ?
Indentation error
Powered by Dreditor.
Comment #11
yched CreditAttribution: yched commented+ retest for possible test bot hiccup
#7: number-1026878-4.patch queued for re-testing.
Comment #13
yched CreditAttribution: yched commented#7: number-1026878-4.patch queued for re-testing.
Comment #14
montesq CreditAttribution: montesq commentedThe patch is validated by the test bot.
You just need to take into account #10 before setting as RTBC
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedSee also #1068016: number field validation fails to block some invalid input causing later SQL fatal error.
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedFrom definition of "major" priority in http://drupal.org/node/45111
Comment #17
wedge CreditAttribution: wedge commentedSubscribing
Comment #18
pillarsdotnet CreditAttribution: pillarsdotnet commented@#10:
Nits picked.
Comment #19
pillarsdotnet CreditAttribution: pillarsdotnet commentedCorrected issue number in the patch name and commit message; sorry.
Comment #20
pillarsdotnet CreditAttribution: pillarsdotnet commented#19: field_number-float_validation-1026878-19.patch queued for re-testing.
Comment #21
yched CreditAttribution: yched commentedLooks good, thanks !
Comment #22
webchickYay, test-driven development! :) Thanks LoMo, and pillarsdotnet!
Committed to 8.x and 7.x.
Comment #23
webchickAlso, since this introduced a new string, tagging 'string freeze'.