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?
Comment | File | Size | Author |
---|---|---|---|
#58 | 1068016-number-field-validation-period-58.patch | 513 bytes | Niklas Fiekas |
#53 | 1068016-number-field-validation-period.patch | 533 bytes | franz |
#50 | 1068016-number-field-validation-fixed.patch | 2.13 KB | catch |
#48 | 1068016-number-field-validation-tests.patch | 1.15 KB | franz |
#48 | 1068016-number-field-validation-fixed.patch | 2.12 KB | franz |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedFrom the SQL query, your editor entered "4-6".
Comment #2
jpincas CreditAttribution: jpincas commentedThanks Damien. I'll slap his wrists.
Comment #3
jpincas CreditAttribution: jpincas commentedPresumably though, the field validation should return an intelligible message and not crash causing loss of all edits?
Comment #4
Rendergraf CreditAttribution: Rendergraf commentedI 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).
Comment #5
yched CreditAttribution: yched commentedEntries 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
Comment #6
mortona2k CreditAttribution: mortona2k commentedI'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?
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedFrom definition of "major" priority in http://drupal.org/node/45111
Describes this issue exactly :)
See also #1026878: Float and Decimal values not fully validated
Comment #8
effulgentsia CreditAttribution: effulgentsia commentedOops. (would be nice if the misspellings of the "needs backport to D7" tag were removed from the autocomplete list)
Comment #9
Patrick R. CreditAttribution: Patrick R. commentedSubscribing.
Comment #10
sun-fire CreditAttribution: sun-fire commentedSubscribe. I have the same error.
Comment #11
karschsp CreditAttribution: karschsp commentedThis patch simply adds an is_numeric() check number fields.
Comment #12
franzI 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".
Comment #13
franzAdding tests. These should fail.
Comment #14
franzJust realized the assertion was totally wrong
Comment #16
franzMerging both, should pass
Comment #17
franzAssertion was still wrong
Comment #18
yched CreditAttribution: yched commentedHm, 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.
Comment #19
yched CreditAttribution: yched commentedEdited my previous post, I was on crack, 'thousands_separator' setting only exists for formatters, and 'decimal_separator' setting doesn't exist for integer fields.
Comment #20
Lars Toomre CreditAttribution: Lars Toomre commentedAm 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)?
Comment #21
franzyes, Lars the ones you suggested fail also in the currently implemented logic.
I think this is yet another issue.
Comment #22
yched CreditAttribution: yched commentedYes, 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.
Comment #23
Lars Toomre CreditAttribution: Lars Toomre commentedI 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).
Comment #24
chx CreditAttribution: chx commented@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]+$
Comment #25
franz@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!"...
Comment #26
Tsubo CreditAttribution: Tsubo commentedSubscribing.
Comment #27
dddbbb CreditAttribution: dddbbb commentedsub
Comment #28
BartVB CreditAttribution: BartVB commentedI 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 :)
Comment #29
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedis_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.
Comment #31
franzNiklas, it seems that patch on #17 also works for your case.
Comment #32
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedyched:
I don't see that either - I haven't tested it, though. Speaking of tests: I should have appended yours.
Comment #33
franzWhy are you ignoring patch on #17 considering it works and has tests?
Comment #34
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedWe'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.)
Comment #35
franzI've added the test and re-rolled patch. Should fail, fix coming.
Comment #36
franzActually, 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.
Comment #37
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedAh ... 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?Comment #38
franzNo, it gets an error too. Is that expected? Why would someone try to input that? Copy & Paste?
Comment #39
franzErr, why is the test on #35 postponed?
Comment #40
Niklas Fiekas CreditAttribution: Niklas Fiekas commented#38: Oh. + in your regexp is (of course) a special character. The following minus (+-) tricked me. Would it work without the +?
Comment #41
franzmight as well.
Comment #42
franzNow that tests are running, let's try with #40
Comment #43
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedWasn't this one indented correctly, before?
Comment #44
franzMaybe there shouldn't be a line break in the first place? Someone care to check?
Comment #45
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedI 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.
Comment #46
franzOk, hope this does it now.
Comment #47
xjmUsually 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.)
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!
Comment #48
franzTalked 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.
Comment #49
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedTest works, coding style good. Manually verified it works. Good arguments for fixing the regular expression like that.
Comment #50
catchFixing a missing period at the end of a sentence before I commit this.
Comment #51
catch#48: 1068016-number-field-validation-fixed.patch queued for re-testing.
Comment #53
franzSeems like you commit it before fixing it.
Comment #54
franzI wonder how did this get committed on D7 already, but hey, cheers!
Comment #55
xjmLooks 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.
Comment #56
catchAh 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.
Comment #57
xjmTechnically needs to be ported, though one could probably just cherry-pick the D7 commit, I think?
Comment #58
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYep, that would be
57019c74
and a bit faster than applying the attached patch.Comment #59
xjmYeah. :)
Comment #60
webchickCommitted and pushed to 7.x. Thanks!