To recreate:

1. Create two numeric webform fields
2. For 'Thousands separator' choose comma.
3. Use the comparison (greater than/less than) form validation between the two fields.
4. Test with a numeric value greater than 999 that contains a comma.

Proposed fix is to use str_replace() to remove commas from field values before processing the comparison.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jastraat created an issue. See original summary.

jastraat’s picture

Status: Active » Needs review
FileSize
539 bytes
Liam Morland’s picture

Status: Needs review » Needs work

What if the thousands separator is a different character?

It would be great for this patch to include a test.

jastraat’s picture

I could reformat this to remove spaces (which is another separator option), but removing periods is more complicated since those could also indicate decimal points.

I would argue that while ideally the comparison validator could support all numeric separator options, supporting at least a few would be better than supporting none.

I also didn't see tests for any of the validation rules - only the act of adding validation to a webform.

Liam Morland’s picture

What about figuring out what character is being used for the decimal and removing anything else?

I know that there is not much in the way of tests. If you are able to add something, even just a unit test for the function you are changing, that would be great.

Anybody’s picture

Sadly #2 definitely needs work for configuration because

  • Comma is used as decimal separator in several (e.g. European) countries (like Germany)
  • Validation is also allowed for text fields, which makes #5 impossible sadly. I think that flexibility is needed.

So I'd suggest to add two new options to that validation:

  • Decimal separator (None (full numbers), '.', ',',default: ".")
  • Thousands separator (None, '.', ',', ' ', default: ",")
helonaut’s picture

I agree, this is the solution we implemented in our running website, by altering the module code manually.
Maybe it's just a better idea to enable multiple lines of "replacement rules" with (un)limited + sign buttons like with the Rules module?

mforbes’s picture

Is it perhaps not that thousands-delimiters are problematic and need to be scrubbed (which is difficult due to decimal separator conflicts), but rather that commas in general are problematic (regardless of whether the comma is used as a thousands-delimiter or as a decimal-delimiter) due to _webform_validation_flatten_array using a comma as an implode character? (near line 983 as of this writing)

If so, I think imploding with some less-likely-to-be-used character would be a decent path forward.

Liam Morland’s picture

Perhaps if both components are of type number, it should cast to a PHP numeric type before making the comparison.