Problem/Motivation

Numeric item fields do not have their minimum value validated if the minimum value is zero. This allows for negative values for decimal items or numeric item fields that are not flagged as unsigned.

This is due to a not-empty check:

    if (!empty($settings['min'])) {
      $min = $settings['min'];
      $constraints[] = $constraint_manager->create('ComplexData', [
        'value' => [
          'Range' => [
            'min' => $min,
            'minMessage' => t('%name: the value may be no less than %min.', ['%name' => $label, '%min' => $min]),
          ],
        ],
      ]);
    }

Zero is technically empty.

Proposed resolution

Check if the value is not equal to ''

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#9 3090629-9.patch4.57 KBmglaman
#9 3090629-9-test-only.patch3.58 KBmglaman
#6 3090629-6.patch1.15 KBmglaman
#2 3090629-2.patch957 bytesmglaman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mglaman created an issue. See original summary.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Status: Active » Needs review
FileSize
3090629-2.patch957 bytes

This means Commerce has been relying on this base field definition and had no validation support.

    $fields['quantity'] = BaseFieldDefinition::create('decimal')
      ->setLabel(t('Quantity'))
      ->setDescription(t('The number of purchased units.'))
      ->setReadOnly(TRUE)
      ->setSetting('min', 0)
      ->setDefaultValue(1)
      ->setDisplayOptions('form', [
        'type' => 'commerce_quantity',
        'weight' => 1,
      ])
      ->setDisplayConfigurable('form', TRUE)
      ->setDisplayConfigurable('view', TRUE);
oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g at Diputació de Barcelona commented

!empty it's also checking for isset. If $settings['min'] is not set the new code will produce a warning. Maybe we can add an isset check? $settings['min'] can not be set if you add the field programatically?

Status: Needs review » Needs work

The last submitted patch, 2: 3090629-2.patch, failed testing. View results

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

I forgot getSettings doesn't merge defaults. The default is '', so maybe that should be done as well.

Or we have PHP 7 so ??

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Status: Needs work » Needs review
Issue tags: +API-First Initiative
FileSize
3090629-6.patch1.15 KB

This ensures the default settings are present when checking the definition settings. This way min/max always have a default value of ''.

Marking this for API-First Initiative since it deals with data definition level validation

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
Status: Needs review » Needs work
Issue tags: +DrupalCon Amsterdam 2019 
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/NumericItemBase.php
@@ -76,10 +76,10 @@ public function getConstraints() {
+    $settings = array_merge($this->getSettings(), self::defaultFieldSettings());

If we use the array_merge then I'd say we have to call static::defaultFieldSettings() instead of self::defaultFieldSettings(). Also, the order of arguments should be changed in order $this->getSettings() could override defaults.

Generally, I think it is better to use two isset($settings[PARAM]) than merging settings.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Assigned: Unassigned » mglaman

Updating to use isset and adding tests.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Assigned: mglaman » Unassigned
Status: Needs work » Needs review
FileSize
3090629-9-test-only.patch3.58 KB
3090629-9.patch4.57 KB

Patch + Test only.

The last submitted patch, 9: 3090629-9-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented

Could we deny specifying empty min and max? I mean if the isset($settings['min']) returns true then we assume this value needs to be validated.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

Could we deny specifying empty min and max? I mean if the isset($settings['min']) returns true then we assume this value needs to be validated.

The default value is empty because there shouldn't be a constraint. The only reason it's not set is due to Drupal does not guarantee the settings array contains the default settings as well or all values as defined in the config schema.

Which is what my other patch did.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
Status: Needs review » Reviewed & tested by the community

Makes sense.

rachel_norfolk’s picture

rachel_norfolk
Primary language English
Location UK
CreditAttribution: rachel_norfolk at Drupal Association commented
Issue tags: -DrupalCon Amsterdam 2019 +Amsterdam2019

retagging

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed eb99b6b50f to 9.0.x and 9ef14c6195 to 8.9.x. Thanks!

Fixed coding standards on commit.

----------------------------------------------------------------------
FOUND 15 ERRORS AFFECTING 12 LINES
----------------------------------------------------------------------
 147 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
 147 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
 148 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
 149 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
 150 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
 154 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
 154 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
 155 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
 156 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
 157 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
 161 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
 161 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
 162 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
 163 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
 164 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
----------------------------------------------------------------------
PHPCBF CAN FIX THE 15 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

  • alexpott committed eb99b6b on 9.0.x 
    Issue #3090629 by mglaman, BR0kEN, oriol_e9g: NumericItemBase never sets...

  • alexpott committed 9ef14c6 on 8.9.x 
    Issue #3090629 by mglaman, BR0kEN, oriol_e9g: NumericItemBase never sets...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Patch (to be ported) » Fixed

Discussed with @catch who +1'd 8.8.x backport.

  • alexpott committed a06b5bf on 8.8.x 
    Issue #3090629 by mglaman, BR0kEN, oriol_e9g: NumericItemBase never sets...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone credited amateescu.

quietone credited cilefen.

quietone credited drugan.

quietone credited pingwin4eg.

quietone credited shadcn.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

closed #2838121: Numeric item min/max settings ignored if set to 0 as a duplicate and moving credit.