Hi there. We are developing a Drupal 7 site and one of my content editors is repeatedly running up against the following error when editing nodes. He's not very technical so it's hard to get him to record enough details to reproduce. I have played around with editing nodes and can't get the error to reproduce. Here's the error in full:

PDOException: SQLSTATE[01000]: Warning: 1265 Data truncated for column 'field_serves_value' at row 1: INSERT INTO {field_data_field_serves} (entity_type, entity_id, revision_id, bundle, delta, language, field_serves_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] => 81 [:db_insert_placeholder_2] => 81 [:db_insert_placeholder_3] => spanish_recipes [:db_insert_placeholder_4] => 0 [:db_insert_placeholder_5] => und [:db_insert_placeholder_6] => 4-6 ) infield_sql_storage_field_storage_write() (line 425 of /home/jpincas/spanishfoodworld.co.uk/modules/field/modules/field_sql_storage/field_sql_storage.module).

The field 'Serves', which appears to be where the error is originating, is a mandatory integer field. I have tried leaving the field blank, entering zero or entering non-integer content (letters) and none of these actions cause this error.

Any ideas what could be causing this?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

From the SQL query, your editor entered "4-6".

jpincas’s picture

Thanks Damien. I'll slap his wrists.

jpincas’s picture

Status: Active » Needs work

Presumably though, the field validation should return an intelligible message and not crash causing loss of all edits?

Rendergraf’s picture

Title: PDOException: SQLSTATE[01000]: Warning: 1265 Data truncated for column... » field_sql_storage_field_storage_write() ------- field_sql_storage.module

I have this error when the user adds a content review, content expected to be published by the Administrator.

PDOException: SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'field_pin_value' at row 1: INSERT INTO {field_data_field_pin} (entity_type, entity_id, revision_id, bundle, delta, language, field_pin_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] => 15 [:db_insert_placeholder_2] => 97 [:db_insert_placeholder_3] => marcadores [:db_insert_placeholder_4] => 0 [:db_insert_placeholder_5] => es [:db_insert_placeholder_6] => 4567456784 ) en field_sql_storage_field_storage_write() (línea 425 de /home/user/public_html/scd/modules/field/modules/field_sql_storage/field_sql_storage.module).

yched’s picture

Title: field_sql_storage_field_storage_write() ------- field_sql_storage.module » number field validation fails to block some invalid input causing later SQL fatal error
Category: support » bug
Status: Needs work » Active

Entries like '4-6' should be detected at validation time and raise a form validation error, but the current validation uses a dumb [^-0-9] pattern (the '-' char is valid as the unary sign char, but that's only true if it's in the 1st position).

Also, it's not clear to me anymore right now why this check currently happens in number_field_widget_validate() (an #element_validate callback, only applicable for widget inputs in forms) rather than in number_field_validate() (generic data validation)

@Rendergraf : your case rather seems an occurrence of #1003692: PDOException: SQLSTATE[22003] after entering large value in integer and decimal field

mortona2k’s picture

I'm having the same problem, glad I found this post, I couldn't even figure out what caused the error until I tried to put in "4-6". I also tried "4*6" and found that this is caught by the validation.

So it sounds like just the validation regex just needs to be improved. Why can't we just call php's is_numeric()?

* And since this is an issue with core, how should I write my own hook to compensate in the mean time?

effulgentsia’s picture

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.

Describes this issue exactly :)

See also #1026878: Float and Decimal values not fully validated

effulgentsia’s picture

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

Oops. (would be nice if the misspellings of the "needs backport to D7" tag were removed from the autocomplete list)

Patrick R.’s picture

Subscribing.

sun-fire’s picture

Subscribe. I have the same error.

karschsp’s picture

Assigned: Unassigned » karschsp
Status: Active » Needs review
FileSize
970 bytes

This patch simply adds an is_numeric() check number fields.

franz’s picture

I don't like the approach in #11, it introduces validation in another function and what's the point in having a check like is_numeric() if we are already checking for almost all invalid characters?

My suggestion is to just improve the regexp. Currently it says "If you find anything character that's not a number, not a decimal point and not a minus sign, we have an error". I added a "or if have a minus sign following any character, we also have an error".

franz’s picture

Adding tests. These should fail.

franz’s picture

Just realized the assertion was totally wrong

Status: Needs review » Needs work

The last submitted patch, 1068016-number-field-validation-tests.patch, failed testing.

franz’s picture

Status: Needs work » Needs review
FileSize
2.06 KB

Merging both, should pass

franz’s picture

Assertion was still wrong

yched’s picture

Hm, I don't get why the new regexps in patch #17 would work ? - i.e accept unary +- operators in the 1st position but not in other positions.

Actually, I'd say using is_numeric probably makes sense, but the logic is not completely straightforward :

- if field type is 'integer', check is_numeric($value) && strpos($value, '.')
if false : error message "Only numbers are allowed in %field"

- if field type is "decimal" or "float",
first, normalize the string :
$value = strtr($value, $field['settings']['decimal_separator'], '.');
then check is_numeric($value)
if false : error message "Only numbers and the decimal separator (@separator) allowed in %field."

- if no error message, set the normalized value as the new value for later processing (data being saved)
form_set_value($element, $value, $form_state);

The error message are not 100% accurate (the +- unary ops are allowed too), but that's already the case currently, and I think we prefer to avoid changing translatable strings when not strictly necessary.

yched’s picture

Edited my previous post, I was on crack, 'thousands_separator' setting only exists for formatters, and 'decimal_separator' setting doesn't exist for integer fields.

Lars Toomre’s picture

Am I understanding from #18 logic that the following are not valid text entries for number fields?

1,000.00 (USA)
1.000,00 (France)

Shouldn't the validation logic pass what is produced by the formatter(s)?

franz’s picture

yes, Lars the ones you suggested fail also in the currently implemented logic.
I think this is yet another issue.

yched’s picture

Yes, thousands separators are currently not accepted on input. I agree this sucks, for instance for copy / paste from output. But I also agree that this belongs in a separate issue.

Lars Toomre’s picture

I created the issue #1259220: Number form element validation is broken for floats to address issue of decoding text representation of numbers as produced by formatter(s) (eg 1,000.00).

chx’s picture

Status: Needs review » Needs work

@yched, the function uses preg_replace($regexp, '', $value) != $value as validation. Now, (.+-) will match at least one character followed by a minus sign. What this needs to be OTOH is (.[+-]) because that matches one character followed by plus or minus. Good trick using a dot to match not-beginning-of-the-string.

However, that's nowhere near enough. I do not think it's a good idea to negative-validate like this (what will you do with two decimal separators, hm?). Instead we simply need a regex to match numbers properly, for integers I think you want simply ^[+-]?[0-9]+$

franz’s picture

@chx, that can't be right:

1) Your string would not catch '12-12' (which is 2 characters followed by a minus). 'at least one character followed by a minus sign' is the correct pattern to be detected, as described in the bug report.

2) There's no need to look for a 'plus' sign, the existing regexp will catch it, The only non-numerical values it didn't were the 'minus' and the decimal separator.

3) The double or more decimal separator is handled properly in the following code in the same function. It may be considered ugly because it could all be a single regexp like you suggested, but...

4) ... The errors messages are different. For the sake of not introducing another one here, I've worked on the existing regexp.

So, it might be the case to reduce to a single message, but that would reduce usability. The negative pattern matching is more adequate if you want to detect several errors on input. "You can't use 2 decimal separators", "Watch out for that minus sign.", "I have some weird chars here!"...

Tsubo’s picture

Subscribing.

dddbbb’s picture

sub

BartVB’s picture

I also bumped into this issue when users reported horrible PDO exceptions when they entered:

150,-

in a price field.

I ended up with a fix almost exactly like chx proposes. The negative preg_replace was really counter intuitive, changed that into a simple preg_match and I'm checking for the position of the allowed minus sign. Added + for completeness.

Patch is against D7 but as far as I could see nothing much has changed in this area between 7 and 8. Currently 2 days before the go-live date so I don't really have time to get D8 up and running now :)

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
2.12 KB

is_numeric doesn't account for decimal seperators unless the input is normalized before checking. So I like #28. Maybe, if we're using positive regular expressions anyway (which is better than negative checking), we can include the "decimal seperator only once"-thing there.

Status: Needs review » Needs work

The last submitted patch, 1068016-number-field-validation.patch, failed testing.

franz’s picture

Niklas, it seems that patch on #17 also works for your case.

Niklas Fiekas’s picture

yched:

Hm, I don't get why the new regexps in patch #17 would work ? - i.e accept unary +- operators in the 1st position but not in other positions.

I don't see that either - I haven't tested it, though. Speaking of tests: I should have appended yours.

franz’s picture

Status: Needs work » Needs review

Why are you ignoring patch on #17 considering it works and has tests?

Niklas Fiekas’s picture

We've argued that the patch doesn't account for inputs like "-10-1". Positive checking is probably easier here, than filtering invalid characters like that.

(But the test looks good. Except a trailing space, and that we could check more stuff.)

franz’s picture

I've added the test and re-rolled patch. Should fail, fix coming.

franz’s picture

Actually, my regexp was catching that case, if you look closely, the '|(.+-)' elegantly catches any *part* of the string that contains a minus sign after something else.

Niklas Fiekas’s picture

Ah ... I didn't notice the .. That is probably the most elegant fix here, keeping everything else as it is and thus keeping a detailed error message of what's wrong. One thing: Would +1 pass?

franz’s picture

No, it gets an error too. Is that expected? Why would someone try to input that? Copy & Paste?

franz’s picture

Err, why is the test on #35 postponed?

Niklas Fiekas’s picture

#38: Oh. + in your regexp is (of course) a special character. The following minus (+-) tricked me. Would it work without the +?

franz’s picture

might as well.

franz’s picture

Now that tests are running, let's try with #40

Niklas Fiekas’s picture

Status: Needs review » Needs work
+++ b/core/modules/field/modules/number/number.testundefined
@@ -88,10 +88,32 @@ class NumberFieldTestCase extends DrupalWebTestCase {
       $this->drupalPost(NULL, $edit, t('Save'));
       $this->assertText(
         t('There should only be one decimal separator (@separator)',
-          array('@separator' => $this->field['settings']['decimal_separator'])),
+        array('@separator' => $this->field['settings']['decimal_separator'])),
         t('Correctly failed to save decimal value with more than one decimal point.')
       );

Wasn't this one indented correctly, before?

franz’s picture

Maybe there shouldn't be a line break in the first place? Someone care to check?

Niklas Fiekas’s picture

I asked in IRC, where BrockBoland pointed me to http://drupal.org/coding-standards#linelength and stated that this was a good case for line breaks. So better not touch it.

But other than that, we're pretty much ready here.

franz’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

Ok, hope this does it now.

xjm’s picture

Status: Needs review » Needs work

Usually assertions are all one line in core. (Personally I actually prefer the formatting in #46, but it deviates from most tests. If you can find other instances in core that put the parameters on separate lines, then I think it'd be fine.)

+++ b/core/modules/field/modules/number/number.testundefined
@@ -92,6 +92,28 @@ class NumberFieldTestCase extends DrupalWebTestCase {
+      $this->assertText(
+        t('Only numbers and the decimal separator (@separator) allowed in ',
+          array('@separator' => $this->field['settings']['decimal_separator'])),
+        t('Correctly failed to save decimal value with minus sign in the wrong position.')

Note that the final parameter here should not use t(). See #500866: [META] remove t() from assert message.

Also, it'd be helpful to upload in a single comment both the test by itself (expected to fail) and the combined version. Thanks!

franz’s picture

Status: Needs work » Needs review
FileSize
2.12 KB
1.15 KB

Talked to xjm about the line-splitting, since it is being used in the test that's immediatly before the new one - which is why we started to discuss that in the first place - so we'll leave it as such.

Fixed the t() and separated the patches.

Niklas Fiekas’s picture

Status: Needs review » Reviewed & tested by the community

Test works, coding style good. Manually verified it works. Good arguments for fixing the regular expression like that.

catch’s picture

Fixing a missing period at the end of a sentence before I commit this.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, 1068016-number-field-validation-fixed.patch, failed testing.

franz’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
533 bytes

Seems like you commit it before fixing it.

franz’s picture

Issue tags: -Needs backport to D7

I wonder how did this get committed on D7 already, but hey, cheers!

xjm’s picture

Issue tags: +Needs backport to D7

Looks like Dries committed it to D7. (I think we leave the backport tag on, thoguh.)
http://drupal.org/commitlog/commit/2/c9d44bf4e0e3e6009334c79fc04f5165c1f...

Edit: The fix in #53 will need to be applied to D7 as well.

catch’s picture

Version: 8.x-dev » 7.x-dev
Priority: Major » Minor
Status: Reviewed & tested by the community » Needs review

Ah I see, Dries committed to both branches, that explains why it didn't apply.

I've committed the follow-up to 8.x, moving this back to 7.x for the code style fix there too.

xjm’s picture

Status: Needs review » Patch (to be ported)

Technically needs to be ported, though one could probably just cherry-pick the D7 commit, I think?

Niklas Fiekas’s picture

Status: Patch (to be ported) » Needs review
FileSize
513 bytes

Yep, that would be 57019c74 and a bit faster than applying the attached patch.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Yeah. :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

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

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