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.

CommentFileSizeAuthor
#2 numeric_with_commas-2872526-2.patch539 bytesjastraat

Comments

jastraat created an issue. See original summary.

jastraat’s picture

jastraat commented
Status: Active » Needs review
StatusFileSize
new numeric_with_commas-2872526-2.patch539 bytes
liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
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

jastraat commented

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

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

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

anybody
Primary language German
Location Porta Westfalica
commented

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

helonaut commented

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

mforbes commented

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

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

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

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Needs work » Closed (outdated)

Drupal 7 is no longer supported. If this applies to a supported version, please re-open.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.