Numeric item min/max settings ignored if set to 0

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/NumericItemBase.php
   @@ -79,7 +79,7 @@ public function getConstraints() {
   -    if (!empty($settings['min'])) {
   +    if (!empty($settings['min']) || is_numeric($settings['min'])) {
   
   @@ -91,7 +91,7 @@ public function getConstraints() {
   -    if (!empty($settings['max'])) {
   +    if (!empty($settings['max']) || is_numeric($settings['max'])) {
   

   Since is_numeric() returns FALSE (https://3v4l.org/523Qg) for NULL and '' (an empty string), let's drop the empty() check and just keep the is_numeric() one.

2. +++ b/core/tests/Drupal/KernelTests/Core/TypedData/TypedDataTest.php
   @@ -629,6 +629,14 @@ public function testTypedDataValidation() {
   +      ->setSettings(array('min' => 0, 'max' => 0));
   

   Let's use the short array syntax here.

Updated as per #9

Great!

I am definitely for this fix in principle, but I noticed that one cannot POST out-of-range values even with the max or min set to 0, because the Drupal\Core\Render\Element\Number::validateNumber prevents it with its own validation code. So we have two range validations in core: one for form posting and a lower-level one for fields themselves when we are not dealing with forms. I find this odd. That's not a reason not to fix this, but I'd like to hear if anybody knows the history and would like to sort this out, maybe in a follow-up.

My question is: Why isn't Drupal\Core\Render\Element\Number::validateNumber using the same constraint code as NumericItemBase and is that something we want?

edited

This issue is fixed here:

#2816859: Allow the 'step' to be configured as a NumberWidget setting

Additionally per form display mode number widget settings are introduced. Also, all the number field types now have unsigned storage setting on the UI. Also, the size storage setting is exposed on the UI.

I'll reroll it!

Hum, looks strange, but the patch applied perfectly for me. I reupload it with a minor difference (array short syntax and commit ref id).

@pingwin4eg

Sorry for getting you confused. Yes, you're right there: if $min = '0' then the constraint will not work. I am going to make this change in my patch.

BTW, I am also subscribing for RTBC on the #10 patch.

+++ b/core/tests/Drupal/KernelTests/Core/TypedData/TypedDataTest.php
@@ -629,6 +629,14 @@ public function testTypedDataValidation() {
+
+    // Test validating when settings min/max = 0
+    $definition = BaseFieldDefinition::create('integer')
+      ->setSettings(['min' => 0, 'max' => 0]);
+    $violations = $this->typedDataManager->create($definition, 1)->validate();
+    $this->assertEqual($violations->count(), 1);
+    $violations = $this->typedDataManager->create($definition, -1)->validate();
+    $this->assertEqual($violations->count(), 1);

I think we should add further tests of what happens when one is not set. I.e something like:

    $definition = BaseFieldDefinition::create('integer')
      ->setSettings(['min' => 5]);
    $violations = $this->typedDataManager->create($definition, 6)->validate();
    $this->assertEqual($violations->count(), 0);
    $violations = $this->typedDataManager->create($definition, -1)->validate();
    $this->assertEqual($violations->count(), 1);
    $violations = $this->typedDataManager->create($definition, 0)->validate();
    $this->assertEqual($violations->count(), 1);

For both min and max case.

Also when both min and max are 0 I think it is worth adding something like:

    $violations = $this->typedDataManager->create($definition, 0)->validate();
    $this->assertEqual($violations->count(), 0);

I spent a long time thinking about whether this is the right place for this test and finally concluded it is after seeing the tests in \Drupal\KernelTests\Core\TypedData\TypedDataTest::testGetAndSet().

@alexpott

Did you mean this?

@drugan yep the new tests are looking good.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/NumericItemBase.php
   @@ -78,26 +78,26 @@ public function getConstraints() {
   -            'minMessage' => t('%name: the value may be no less than %min.', ['%name' => $label, '%min' => $min]),
   +            'minMessage' => $this->t('%name: the value may be no less than %min.', ['%name' => $label, '%min' => $min]),
   ...
   -            'maxMessage' => t('%name: the value may be no greater than %max.', ['%name' => $label, '%max' => $max]),
   +            'maxMessage' => $this->t('%name: the value may be no greater than %max.', ['%name' => $label, '%max' => $max]),
   

   This change whilst technically correct is not in scope for this patch.

2. +++ b/core/tests/Drupal/KernelTests/Core/TypedData/TypedDataTest.php
   @@ -114,6 +114,33 @@ public function testGetAndSet() {
   +    $violations = $this->typedDataManager->create($definition, 0)->validate();
   +    $this->assertEqual($violations->count(), 1);
   ...
   +    $violations = $this->typedDataManager->create($definition, 0)->validate();
   +    $this->assertEqual($violations->count(), 0);
   

   Thinks about the code we are test - rather than validating a 0 value here we should use a value of 5.

@alexpott

Yes, I agree on that. We should check the value which is equal to a setting, then the value which is greater by one and then the value which is less by one than the setting.

The test changes look good good.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/NumericItemBase.php
@@ -78,9 +78,10 @@ public function getConstraints() {
+    $min = is_numeric($settings['min']) ? $settings['min'] : NULL;
...
-    if (!empty($settings['min'])) {

I pondered whether this might introduce php errors if min is not set. It would, but these keys are added in \Drupal\Core\Field\Plugin\Field\FieldType\NumericItemBase::defaultFieldSettings() so they have to be set. So all good here as far as I can see.

@alexpott

What I've found on number field settings is the following:

The FieldItemBase::defaultSettings() return is empty array. The NumericitemBase class merges its default min and max settings which are empty strings with FieldItemBase::defaultSettings(). The IntegerItem class merges its default min and max which are empty strings with NumericitemBase::defaultSettings(). Therefore min and max double ensured they exist and have an empty string value by default.

The NumericItemBase->getSettings() return is the result of the following merge:

FieldItemBase::defaultSettings() + NumericitemBase::defaultSettings() + IntegerItem::defaultSettings() + IntegerItem::defaultStorageSettings() + actual field settings and field storage settings saved on the UI form.

So, if the field is created with all default values it has min and max values as NULL.

Why is not empty string?

This is because Number::validateNumber() will not even try to validate the empty string which further goes to FormElement::valueCallback() to be returned as NULL, disregarding of whatever the $input value is. That's how default min and max string values become NULL values after the field being saved empty on a form.

The conclusion of the above is:

The min and max settings are always exist in the NumericItemBase->getSettings() return either as empty strings (in code) or NULL values or actual numeric values saved on those fields.

The #26 fix is merged into the latest patch of:

#2816859: Allow the 'step' to be configured as a NumberWidget setting

So, if you want to get more than just properly working constraints on the number filed, please take the patch in the link for testing.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Could someone confirm #3090629: NumericItemBase never sets minimum value constraint if minimum value is zero. solves this issue?

The issue summary at #3090629: NumericItemBase never sets minimum value constraint if minimum value is zero. does look like it is solving the same problem and so does the code changes. I doubled checked and tested as well. I am closing this as a duplicate and moving credit over there.

Cheers