Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specifying Cake\Validation\Validator::decimal() $places parameter causes validation to fail #17459

Closed
maurobrandoni1 opened this issue Nov 30, 2023 · 12 comments

Comments

@maurobrandoni1
Copy link
Contributor

Description

How to reproduce the problem:
FooTable.php:

public function validationDefault(Validator $validator): Validator
{
    $validator->decimal('myValue', 2);
    return $validator;
}

FooController.php:

$foo = $this->Foo->newEmptyEntity();
$foo = $this->Foo->patchEntity($foo, ['bar' => '3.14']);
dd($document->getError('bar'));

According to @LordSimal

It has to do with what you set as intl.default_locale
I have de_AT and the same problem.
If I change it to ini_set('intl.default_locale', 'en_US'); it works again

CakePHP Version

4.4.17

PHP Version

No response

@dereuromark dereuromark added this to the 4.5.2 milestone Nov 30, 2023
@LordSimal
Copy link
Contributor

LordSimal commented Nov 30, 2023

As can be seen in the following screenshot the Validation::decimal method expects the given value to be in the current defined default_locale number format.

image

So for me (de_DE) it expects the decimal-point to be a , and the groupingSep (aka "thousands point") to be a . because thats how the number format is defined for my locale (as you can see here)

Therefore this returns false since it changes 3.14 to 314 as it removes all thousands points from the string.

My main question now for my fellow core devs is: shouldn't that validation method be called localeDecimal so its more clear that it expects a locale formatted number string?

Maybe I misunderstand something here but no matter which locale I have set in my browser/OS a input number with a step="0.1" always returns the number formatted with a . as a decimal separator so we don't need to take care of that when parsing $_POST data from a number input.

@LordSimal
Copy link
Contributor

@maurobrandoni1 which locale is set in your app? Meaning, what do you have in your config/.env as a APP_DEFAULT_LOCALE or in your App.defaultLocale config?

@dereuromark
Copy link
Member

dereuromark commented Nov 30, 2023

Yeah, the locale part comes from the time where normal text fields are being used.

That said:
If someone is using normal text fields that whole localized validation could also fall apart on the localized group separator it seems.

E.g. English vs German: 1,234.56 vs 1.234,56 (exact opposite)

Wouldnt it make more sense to normalize this to the new _ group separator and go from there?

1_234.56

when dealing with the input from user?

And back to localized string on output anywhere it is used, of course.
Non tec people shouldnt see that kind of separator.

Note: Also PHP 7.4+ _ seems not allowed yet as separator and fails the regex. But this is usually supposed to be only used within PHP directly, so probably not something to directly support as user input.

@LordSimal
Copy link
Contributor

The support for input type number has been great for quite some time. We also default to it in the FormHelper so we should adapt this as well imho.

Input numbers are submitted as string without a thousand's separator anyway as far as I have tested it. So there is no thousands place to replace here.

I'd still keep the regex part here so it still supports larger numbers (in both directions) than float can support as well.

In the end I think we just have to rip out the NumberFormatter parts and we should be fine.

@dereuromark
Copy link
Member

@maurobrandoni1
Did you put the

TypeFactory::build('decimal')->useLocaleParser();

in your code? As that seems to make it work for me.

@LordSimal
Copy link
Contributor

I just tried it with the FloatType (since my db columns are of type float) via doing

\Cake\Database\TypeFactory::build('float')->useLocaleParser();

inside my bootstrap.php but this problem still exists for me

@dereuromark
Copy link
Member

It could be that float is not a valid type for a decimal check (since float doesnt have the precision/scale by nature).
I added a live demo for decimal:
https://sandbox.dereuromark.de/sandbox/decimal-examples/validation/de_AT?value=1234%2C56
The transformation string => value object shouldnt be relevant as this happens after validation afaik.

@maurobrandoni1
Copy link
Contributor Author

My locale is 'es'

Adding this piece of code to my bootstrap.php doesn't make any difference:

$decimalType = \Cake\Database\TypeFactory::build('decimal');
/** @var \Cake\Database\Type\DecimalType $decimalType */
$decimalType->useLocaleParser();

Foo->getSchema():

'typeMap' => [
    'bar' => 'decimal',
]

@dereuromark
Copy link
Member

dereuromark commented Nov 30, 2023

I am not too familiar with ES, but it seems to want a comma as decimal separator:
https://sandbox.dereuromark.de/sandbox/decimal-examples/validation/es?value=1234%2C56
Not sure but it sounds more like the intl locale parser might be at fault here then?

es_MX however again: https://sandbox.dereuromark.de/sandbox/decimal-examples/validation/es_MX?value=1234.56
Maybe your PHP env doesnt allow to use non-location locales?

@ADmad
Copy link
Member

ADmad commented Nov 30, 2023

shouldn't that validation method be called localeDecimal so its more clear that it expects a locale formatted number string?

I agree, similar to localizedTime().

I too would except decimal() to only support "normal" numeric values with dot as decimal separator as that's the format always returned by HTML input of type numeric.

Sadly due to backwards compatibility issues we can't change it even in 5.x :(

@LordSimal
Copy link
Contributor

Sadly due to backwards compatibility issues we can't change it even in 5.x :(

yes, but we can at least document it and make it clear how it currently behaves

Copy link

This issue is stale because it has been open for 120 days with no activity. Remove the stale label or comment or this will be closed in 15 days

@github-actions github-actions bot added the stale label Apr 30, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants