number.module is enforcing a maximum value even if one is not specified

[rbargerhuff]

Description of the bug

This issue/bug concerns the number.module within Backdrop Core. When defining a Number (integer) field, if states that if a maximum value is not specified for the field, then there will not be a maximum value set for the field. Below is the help text for the Maximum setting associated with the Number field.

"The maximum value that should be allowed in this field. Leave blank for no maximum."

Here is the code that is enforcing the maximum when one is not required:

function number_field_widget_form(&$form, &$form_state, $field, $instance, $langcode, $items, $delta, $element) {
  $value = isset($items[$delta]['value']) ? $items[$delta]['value'] : '';

  $element += array(
    '#type' => 'number',
    '#default_value' => $value,
    '#max' => 1000000,
  );

  ....

  // Set minimum and maximum.
  if (is_numeric($instance['settings']['min'])) {
    $element['#min'] = $instance['settings']['min'];
  }
  if (is_numeric($instance['settings']['max'])) {
    $element['#max'] = $instance['settings']['max'];
  }

Expected Behavior

Honor not having a maximum value defined on a Number (integer) field.

Additional information

• Backdrop CMS version: 1.25

[argiepiano]

Looking at the "blame" history, it looks like this was added to control the size of the widget. According to Mozilla, number inputs don't accept the size attribute, and therefore sizing must be done via CSS, or by specifying the max attribute. I believe the person who added this overlooked the fact that the number max could be left blank in the field definition.

Solving this by removing the #max attribute without affecting the look of the widget will be tricky - adding css to Basis is one way to go, but that would leave out all contrib themes.

[rbargerhuff]

I understand your concern. I'm not sure what kind of work-around would be needed. However, enforcing a maximum value where one may not be required could become a serious issue if the developer was not aware of this.

As of right now, every number field has a maximum value 1000000 and not every developer is going to be aware of this. This in my opinion is a huge flaw because there are many scenarios where developers would not require a maximum number, for example a number associated with a student ID.

My proposal is that this flaw needs to be rectified and once it that is in place, an hook_update() needs to process all existing number fields within a site and define a max of 1000000 for any number fields that do not have a max set. That way, everything stays the same but you are no longer enforcing an abitrary value. A blog post of some sort would need to be posted to make developers aware of this.

At the very least, the field definition's description needs to be updated to indicate to the dfeveloper exactly what it is doing behind the scenes.

@smaiorana @quicksketch

[argiepiano]

@rbargerhuff I agree this needs to be fixed. I was just pointing out things to keep in mind when we are fixing - it seems you gave this already some thought. Would you be able to submit a PR?

[quicksketch]

Thanks for the background research @argiepiano. I couldn't find an original issue other than the pull request, it might have been so early in Backdrop development we didn't track everything with issues yet. The original PR is here: backdrop/backdrop#247

I agree that we shouldn't be using #max for the purpose of controlling display. If there's no max specified, it should be unlimited as described. In my testing on Chrome and Firefox, I found that max doesn't actually seem to affect display anyway. Maybe it used to, but regardless of whether the max is set to 3 or a billion, the width of the field always stays the same for me. Can that be confirmed by others?

Here's a field with a max of 3, which you'd think would display only large enough for 1 character, but it displays enough field for a much larger number:

[smaiorana]

I was looking into this issue yesterday with @rbargerhuff and reached the same conclusion as @quicksketch. I couldn't get the size of the number field to change, even without the max attribute. This was in Chrome, Firefox, and Safari.

Is it already being controlled by the default Backdrop theme(s)? I was testing with Seven and only found max-width: 100% on the element.

[rbargerhuff]

@rbargerhuff I agree this needs to be fixed. I was just pointing out things to keep in mind when we are fixing - it seems you gave this already some thought. Would you be able to submit a PR?

Sorry I can't. I recently finished a Backdrop Core PR involving Layouts and new features... I just do not have the time to submit a new PR due to time constraints with getting Backdrop deployed to our production websites and sticking to our rollout schedule.

[indigoxela]

I guess, this has partly been discussed before in #4751

There will always be a limit, just because of the database - the highest value, that can be saved.
The number "1000000" seems arbitrary and can get discussed. That there is a limit - even if it's just the largest value, this database column type can hold - is a technical necessity.

[avpaderno]

Yes, there will be always a limit, even if that is the database limit. The description of that form element should be changed.

The maximum value that should be allowed in this field. Leave blank for 1000000.

[smaiorana]

Both @indigoxela and @kiamlaluno are correct that there will always be a limit enforced by the database. Here's the thing I just realized - while Backdrop is enforcing a default maximum limit on number fields, it doesn't enforce a default minimum limit on those fields. So if you create a number field, leave the minimum limit blank, then try to populate that field with a number lower than what the database will accept, the site will return a database error.

My suggestion is that we let these fields accept the lowest/highest number the database will allow when a minimum/maximum is not specified.

Here's how we can handle it for the different number types:

• Integer Fields: We know from the MariaDB Doc Page on INT Data Types that the signed range is -2147483648 to 2147483647. I did notice that limits of -2147483647 and 2147483647 are already referenced elsewhere in the Number module, so it's not foreign to existing Backdrop code.
• Decimal Fields: These can be easily calculated based on their precision and scale settings.
• Float Fields: These are the only question mark. The MariaDB Doc Page on Float Data Types mention a theoretical range of -3.402823466E+38 to 3.402823466E+38. I tested this range on a Backdrop instance of my own and it accepted them just fine. But the documentation also notes the range might be lower based on hardware or OS. So I don't know if we should just accept the theoretical range or cap it at something smaller - maybe the same as the range for integers.

I submitted a PR containing the default ranges above, including the theoretical range on float values. Let me know what you all think.

[indigoxela]

@smaiorana many thanks for providing a PR. 🙏

So, if I get it right, your suggestion is to always display min/max in the field widget? The previous PR just prevented admins to set an unusable limit.

Not sure, if that's ideal if it comes to user experience... Needs some testing with common use-cases.

[klonos]

Thanks for contributing @smaiorana 🙏🏼

I will need to have a closer look and do some research, but for now I would like to leave these 2 comments here:

• Actual feedback on the PR and the code: Why are we using '#type' => 'textfield' instead of '#type' => 'number' in the form?
• Theoretical comments: Ideally people should be using some sort of container-based local development approach, in order to be matching the versions of software being used on their live/production site, but that is not always the case. That means that the versions of db/web server and sometimes even php don't match exactly those on the live site. Attempting to set min/max values is then only best effort the way I see it. If we "hard-code" these min/max values in the db or configuration to the absolute max/min that is possible in their current setup, what happens when the site moves to another platform/setup and the min/max limits are different? (by "different" I mean that what we have previously specified exceeds the supported min/max) ...and what happens if the configuration that was exported on a local setup has "conflicts" with what is supported on that actual site (and vice versa) ...not much we can do about this, and I understand that we are trying to set some sensible defaults, so just mentioning all that in order to help make better-informed decisions here. On a related note: is there any native PHP constant that we can use that "dynamically" adapts to what is supported by the system? I know for instance that there's PHP_INT_MAX/PHP_INT_MIN and PHP_FLOAT_DIG/PHP_FLOAT_MIN/PHP_FLOAT_MAX etc., but these seem to be tied to what is supported by the PHP version - not the db.

[klonos]

...the trick mentioned here might be of help: https://stackoverflow.com/questions/2679064/in-sql-how-do-i-get-the-maximum-value-for-an-integer

So we might be able to use that to set some global constants with respect to what is supported by MySQL + then take into account what the current version of PHP supports handling.

Having said the above, we need to consider that reaching these limits is still theoretical and would apply to edge cases (sites that use maths?). As long as our help text mentions what the min/max supported values can be, I think that we're good.

[smaiorana]

@klonos I think the theoretical limits are for floating point numbers only. Correct me if I'm wrong, but I believe MySQL/MariaDB always has a standard range of -2147483648 to 2147483647 on the signed INT data type (which is what Backdrop's integer fields use). I think it's generally safe to hardcode those. It's already hardcoded into number.module when you save the settings on a number field:

  if ($field_type == 'number_integer') {
    // @see https://mariadb.com/kb/en/int/
    $min_db_integer = -2147483647;
    $max_db_integer = 2147483647;
    $settings = $form_state['values']['instance']['settings'];

    if (!empty($settings['min']) && $settings['min'] < $min_db_integer) {
      form_error($min, t('%min is smaller than the smallest value the database can save. Choose a value equal to or higher than %db_min.', array(
        '%min' => $settings['min'],
        '%db_min' => $min_db_integer,
      )));
    }
    if (!empty($settings['max']) && $settings['max'] > $max_db_integer) {
      form_error($max, t('%max is greater than the largest value the database can save. Choose a value equal to or lower than %db_max.', array(
        '%max' => $settings['max'],
        '%db_max' => $max_db_integer,
      )));
    }
  }

But for floating point numbers, I haven't been able to find a way to gauge limits on those, though the likelihood that someone would reach a number like 3.402823466E+38 I think puts in in the category of edge cases like you said.

Why are we using '#type' => 'textfield' instead of '#type' => 'number' in the form?

That looks like a holdover from Drupal 7. I can change the field type in my PR if you'd like.

Having said the above, we need to consider that reaching these limits is still theoretical and would apply to edge cases (sites that use maths?). As long as our help text mentions what the min/max supported values can be, I think that we're good.

Are you suggesting something like this: "The maximum value that should be allowed in this field. Leave blank for 2,147,483,647 (the maximum value that can be saved in the database)."

[yorkshire-pudding]

I think that suggestion for wording makes sense.

[robertgarrigos]

I just run into this problem. I tested the PR, and it works fine for me.

[quicksketch]

Thanks folks! I wasn't sure about the extremely high values set for float, but in testing this didn't seem to have any negative impact and it seems like a good safety measure. With the approval of multiple people, I went ahead and merged backdrop/backdrop#4483 into 1.x and 1.25.x. Thanks @smaiorana for your work and @rbargerhuff for filing the issue!

backdrop/backdrop@a8cb694 by @smaiorana, @rbargerhuff, @argiepiano, @indigoxela, @kiamlaluno, @klonos, @yorkshire-pudding, @robertgarrigos, and @quicksketch.