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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LoMo’s picture

Status: Active » Needs review
FileSize
2.84 KB

I 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).

Status: Needs review » Needs work

The last submitted patch, number-1026878.patch, failed testing.

LoMo’s picture

FileSize
2.92 KB

Oops. 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.

LoMo’s picture

Status: Needs work » Needs review
LoMo’s picture

FileSize
2.91 KB

Minor changes... removal of stray "white space" at ends of lines and other small coding standards compliance edits.

yched’s picture

Status: Needs review » Needs work

Makes sense, I guess.

Patch looks fine, thanks ! A couple nitpicks :

+        // Check to make sure the decimal separator isn't in the field more than once

Missing period at the end of the sentence. + comments need to wrap at 80 chars.

+          $message = t('There should only be one decimal separator (@separator) in %field.',

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 ?

LoMo’s picture

FileSize
2.89 KB

1) 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. ;-)

LoMo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, number-1026878-4.patch, failed testing.

yched’s picture

Thanks - a couple more nitpicks and we're there :-)

+++ modules/field/modules/number/number.module	30 Jan 2011 08:27:46 -0000
@@ -380,9 +380,21 @@ function number_field_widget_validate($e
+        // Check that the decimal separator isn't in the field more than once.

Core tries to avoid "doesn't", "hasn't" forms in comments.
Maybe "Check that the separator does not appear more than once" ?

+++ modules/field/modules/number/number.test	30 Jan 2011 08:27:46 -0000
@@ -71,5 +71,28 @@ class NumberFieldTestCase extends Drupal
+    // Try to create entries with more than one decimal separator; assert fail.
+      $wrong_entries = array(
+      '3.14.159',

Indentation error

Powered by Dreditor.

yched’s picture

Status: Needs work » Needs review

+ retest for possible test bot hiccup

#7: number-1026878-4.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, number-1026878-4.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

#7: number-1026878-4.patch queued for re-testing.

montesq’s picture

Status: Needs review » Needs work

The patch is validated by the test bot.
You just need to take into account #10 before setting as RTBC

effulgentsia’s picture

effulgentsia’s picture

Version: 7.x-dev » 8.x-dev
Priority: Normal » Major
Issue tags: +Needs backport to D7

From definition of "major" priority in http://drupal.org/node/45111

An example would be a PHP error which is only triggered under rare circumstances or which affects only a small percentage of all users.

wedge’s picture

Subscribing

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
3.04 KB

@#10:

a couple more nitpicks

Nits picked.

pillarsdotnet’s picture

Corrected issue number in the patch name and commit message; sorry.

pillarsdotnet’s picture

yched’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks !

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yay, test-driven development! :) Thanks LoMo, and pillarsdotnet!

Committed to 8.x and 7.x.

webchick’s picture

Issue tags: +String freeze

Also, since this introduced a new string, tagging 'string freeze'.

Status: Fixed » Closed (fixed)
Issue tags: -String freeze, -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.