Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
For now all types of a number field added in code by BaseFieldDefinition::create('integer' | 'decimal' | 'float') method as a part of Content Entity don't have access to the field edit form and can't change or set any settings such as default_value, min, max, prefix and suffix. The only setting that we have on any mode of the form display number widget is placeholder setting.
Proposed resolution
Add all available settings on the number field widget settings form, the step setting including. What we actually need is to duplicate NumericItemBase->fieldSettingsForm() form content on the widget adding step form element and slightly improve the form values handling.
Remaining tasks
Consider to duplicate field settings form on a widget form for other base fields found in core/lib/Drupal/Core/Field/Plugin/Field/FieldType folder.
User interface changes
User can set all number field settings for each of the form display modes individually.
API changes
Add min, max, default_value, step, prefix and suffix elements to NumberWidget->settingsForm() and core/config/schema/core.entity.schema.yml field.widget.settings.number and Drupal\Tests\field\Kernel\Migrate\d6\MigrateFieldWidgetSettingsTest->testWidgetSettings().
Add NumberWidget->getFormDisplayModeSettings() method.
Rewrite core/modules/field/src/Tests/Number/NumberFieldTest class to comply with new number widget settings.
Data model changes
Unknown.
| Comment | File | Size | Author |
|---|---|---|---|
| #46 | fix-step-error.patch | 2.49 KB | mishac |
| #37 | allow_the_step_to_be-2816859-37.patch | 2.65 KB | joekers |
| #28 | Screen Shot 2017-06-02 at 14.59.14.png | 53.75 KB | finne |
| #27 | Screen Shot 2017-06-02 at 13.30.37.png | 13.69 KB | finne |
| #26 | interdiff-25-26.txt | 4.22 KB | drugan |
Comments
Comment #2
drugan CreditAttribution: drugan as a volunteer commentedThe form display mode number widget settings abide following rules:
max setting on a form display mode may not be greater than the relative base field setting.
min setting on a form display mode may not be less than the relative base field setting.
default_value and min may not be greater than max.
max and default_value may not be less than min.
step may not be less than zero and less than min and greater than (max - min) / 2.
As an example is taken Account Entity settings form which has 2 display modes: "Default" and "Register".
For testing purposes added integer, decimal and float number field types to the entity. The scale setting for decimal field is set to 4 when saving the field.
"Default" form display mode.
"Register" form display mode. Note that this mode have different settings.
User account edit form ("Default" mode).
User register form ("Register" mode). Note different settings.
All these settings are very useful for a number but the main magic is done by the step setting. It basically creates field subtypes on the fly. For example, if you set step 0.5 for a decimal field you restrict this field to have only one digit after decimal despite the default decimal field scale allows two digits. The same way if you omit decimal sign on this field you restrict it to integer values despite the field is decimal.
The rule of thumb is that: a number field could shrink its functionality but not extend.
Comment #3
drugan CreditAttribution: drugan as a volunteer commentedComment #5
drugan CreditAttribution: drugan as a volunteer commentedNew settings were added for field.widget.settings.number in the core/config/schema/core.entity.schema.yml
to prevent exceptions thrown when running simpletest.
Also, Drupal\field\Tests\Number\NumberFieldTest class was rewritten as to be in compliance with the feature added and a bit of refactoring were done too on the class code.
Comment #6
drugan CreditAttribution: drugan as a volunteer commentedComment #7
drugan CreditAttribution: drugan as a volunteer commentedTesting module on simplytest.me always crashes. As I think the reason the PHP version on the server is incompatible with code of the module.
By the way, while working on the patch I also got a lot of crashes with this module. The Drupal 8 is still in unstable state and a lot of work need to be done.
Comment #9
drugan CreditAttribution: drugan as a volunteer commentedUPD!
The following failed tests need to be resolved:
Drupal\file\Tests\FilePrivateTest
Drupal\Tests\field\Kernel\Migrate\d6\MigrateFieldWidgetSettingsTest
Drupal\system\Tests\Update\UpdatePathRC1TestBaseFilledTest
Drupal\system\Tests\Update\UpdatePathTestBaseFilledTest
Comment #10
drugan CreditAttribution: drugan as a volunteer commentedResolved failed tests:
1. Drupal\file\Tests\FilePrivateTest.
2. Added new config settings to Drupal\Tests\field\Kernel\Migrate\d6\MigrateFieldWidgetSettingsTest.
All the rest failed tests are derivative from the above, as I think.
Comment #11
drugan CreditAttribution: drugan as a volunteer commentedComment #12
drugan CreditAttribution: drugan as a volunteer commentedImproved displaying initial default step value on a field form display summary.
Comment #14
steveoliver CreditAttribution: steveoliver at Circatree commentedThis works as expected, has passing tests, and seems like an obvious helpful addition to core.
Before a detailed nit-pick review and RTBC, I wonder if any Field subsystem maintainers (@amateescu, @yched) might be able to review this?
Comment #15
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI can't see what is the point of this patch/issue. A widget only works (and is configured) for a specific field (field instance in D7 terms), so any of these settings would only affect how the form element is displayed for that particular field, in which case a user can configure the field directly...
Comment #16
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedDiscussed with @steveoliver on IRC and it seems that only 'step' is not exposed anywhere, and I have a feeling that's the only needed for #2794909: The quantity "step" must be configurable on the widget level, so let's focus this issue only on that :)
Comment #17
steveoliver CreditAttribution: steveoliver at Circatree commentedFrom my continued discussion with @drugan on irc this morning: I think the crux of the issue with articulating the issue(s) and reviewing this patch is this:
Is that also your understanding @drugan and @amateescu, or do you have other/different concerns?
Comment #18
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI don't think we need 1., because like 3. mentions, only the widget setting is actually needed (2.).
Comment #19
drugan CreditAttribution: drugan as a volunteer commented@amateescu
I think that not having access to the step setting from the UI we loose a lot of a power of the number field. Also, the default value is not visible on the form display mode despite being set on the field setting page.
I bit later I'll prepare the patch reroll to 8.3.x version and after that we can discuss the changes.
Comment #20
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI said in #18 that I agree with exposing the step as a widget setting :)
That's completely by design. The default value of the field has nothing to do with how its widget is displayed, it's not even a field 'setting' (i.e. as part of the 'settings' config key), separation of concerns and all that :)
Since this is a feature request, it can only get into 8.4.x.
Comment #21
drugan CreditAttribution: drugan as a volunteer commentedSeems that some tests extending core/modules/system/src/Tests/Update/UpdatePathTestBase.php are failed. The reason have to be found, though I think it does not prevent users from testing the patch.
The patch has been a lot rewritten.
All the number types now have unsigned storage setting. The integer size now could be configured from the UI. The decimal number is also restricted as for its size. If you leave alone default precision = 10 and scale = 2, then the size of the decimal will be in the range -99999999.99 - 99999999.99. As for the float number it can't be restricted by the size properly for now. As it said in the specs it can be **any** of a step, min, max, precision and scale. But actually that is not true. The system silently truncates it if the precision exceeds 14 digits. The max, min and step settings should not be left empty if you want to see more or less expected behaviour on the field.
The step and placeholder settings are added to the base field settings. Basically it means that the settings for all possible form display modes now may be set up in one place. Though can be overridden on a form display mode's widget settings. The default value is displayed on a form display mode if no value on the field is saved in the database. And of course if the default value is set up as the base or a form display mode setting. Otherwise the database saved field's value is displayed and default value is not. The prefix and suffix now also have a plural variant on a form number field if that is set up with a pipe '|' separator.
Note that the patch also applies cleanly to 8.3.x version for now.
Comment #23
drugan CreditAttribution: drugan as a volunteer commentedfield_update_8004() is added.
Some refactoring work are done.
Comment #25
drugan CreditAttribution: drugan as a volunteer commentedThe reason for the latest fail was in core/tests/Drupal/Tests/Component/Utility/NumberTest.php.
It checked and validated a negative step. Actually it is impossible case and current patch invalidates such steps.
Also, it invalidated a step which is equal to value but that is wrong because when those numbers are the same there is no way to move up or down. Therefore the step is valid disregarding of whatever the min is. The only condition is the min must be less or equal to the value otherwise the whole integrity breaks and even actually valid step has no any sense. Look into the failed test output:
Despite both value and step can be represented in such a huge length only internally nevertheless they are both equal numbers and the patch fairly asserts this step as a valid one. In a **real life** they are truncated to 14 digits if it is a float number and to the precision length if it is a decimal number.
Also, the test passes to the Number::validStep() the values which are result of calculation, like:
value = 2 / 7 + 5 / 9, step = 1 / 7, min = 5 / 9
value = 1 / 5, step = 1 / 7, min = 1 / 11
value = 70 + 9e-7, step = 10 + 9e-7
As for me it should be avoided because the end result may look differently on different systems. Instead numbers should be passed for testing implicitly and preferably in a string representation. But for now I've leaved this code alone just to prove the patch works with those calculations too.
What I've found PHP considers the number **AS IS** if it is a string number but may give a surprisingly strange result if it is the actual number of whatever type. By the way PHP library BC MATH functions' return is always a string number.
Comment #26
drugan CreditAttribution: drugan as a volunteer commentedMerged #26 patch from:
#2838121: Numeric item min/max settings ignored if set to 0
Also, some coding standard errors are fixed.
Comment #27
finneReview:
the placeholder does not fit in a number field, and is confusing. Move the explanation to the description. See screenshot
The patch seems work fine in my manual test.
Comment #28
finneAlso, maybe you should hide any default values in the widget settings. Showing all defaults creates a very large row for any number widget, while most settings will not be altered. Maybe display only non empty/default values.
Comment #29
drugan CreditAttribution: drugan as a volunteer commented@finne
The propose to move min and max allowed values from placeholder to description seems to me sensible. I am going to do this on the next commit.
As for the summary the unalterable values also might be moved to the widget settings form's markup but I am not sure with that. The initial reason for displaying those values was to prevent questions like "why I can't set the value less/greater while I did not restrict it on the form modes' widget?".
The complexity with the number widget is that the numeric fields on the widget settings form such as step, min, max also have their own step, min, max settings which should not be overflowed. Any attempt to save widget settings with a wrong value will result in the standard Drupal error message but as we have floor, ceil and base_* values this will result in just a small pop-up with the allowed values explained.
For me it seems better to have a full summary on the widget than having bad UX. Usually site's administrators are not much particular about aesthetic on the admin forms. Showing the full summary on the number widget allows to check all the values without opening the form by clicking at the gear icon.
Comment #30
finne@drugan Sounds good to me.
The patch is almost 100k, a lot to review. Maybe we should cut it into pieces that offer 1 config addition each. But I will see if I can find time to look at the code.
Comment #31
finneHmm installing the patch gave me status errors complaining I needed db updates and entity updates. Running these (drush updb) fails. Existing entity number fields could not be updated.
This might be specific because it concerns a commerce entity which might have its fields definition locked...?
Comment #32
drugan CreditAttribution: drugan as a volunteer commented@finne
If you use the current patch together with the Commerce 2.x number patch and get a drush warning you can easily ignore it. I didn't explored deeply why it complains but think that drush makes so called dry run before the actual run and obviously it "sees" that the fields have data saved on them so there is a warning (mysite.com/update.php has no this warning). The db update hook on both the patches truncates the data first and restore it after configurations on the field are changed. The Drupal 8 does not allow changes on the not empty fields. Read more here and here.
I understand that the patch is very big to be reviewed but all the changes introduced are so interdependent that the whole amount of work of breaking the patch into chunks may become a huge one if at all feasible. Remember every chunk will require the Drupal automated test to be green before anyone can decide to review it. Even if the functionality introduced by the chunk will work as expected.
Let's take for example the "step" property introduced by the patch on the base field settings page. For now it has a default value 1 for the integer and float number fields and 0.01 for the decimal and can't be changed through UI. OK, we'll add an additional "step" field ("#type" => "number") in the NumericItemBase.php file but the actual default value for this field may be only assigned in the respective IntegerItem.php or DecimalItem.php or FloatItem.php file. So, we need to make changes in each of the specific number type files. Going further, what the "min" ("floor") and "max" ("ceil") values should be allowed by default for the step? It only could be decided from the respective field storage settings, so we need to add the code for checking database driver used by the site because different db drivers may have different range of allowed values for one and the same field. It's not an easy task which requires a lot of code added and should be repeated for the actual "min" and "max" property fields on a number field base settings page ("floor" and "ceil" by default).
Back to the "step", let's say someone changes the default value for this field (i.e.- the minimum positive step allowed on number type) by typing an arbitrary value manually (without using HTML number field widget controls (usually small arrows)). Is that the right value? It is checked in the core/lib/Drupal/Component/Utility/Number.php::validStep() but for now this method invalidates actually valid values (introduced by our new code for the storage allowed ranges) and should be rewritten to give an answer on the step validity. So, we need to change this method and introduce a bunch of helper methods (repeat for the "min" and "max" chunks of the current patch).
Also, think of the "default_value" field on a field base settings page. It is not the same field as the "step", "min" and "max" fields there
and actually a regular number field of the default form display mode. Like on any other webform which we use to get user's data. So, in order to fetch the correct settings and default value (!) for this field we need to introduce these settings for a form display mode in the NumberWidget.php file. Here we are! The snake has eaten its tail.
Anyway, if somebody will see the ways to break the current patch into chunks, please fell free to take the snippet(s) from the patch and open a child issue with current issue referenced as the parent one.
Comment #33
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@drugan, how do you expect this issue to move forward when you simply ignore feedback from the core subsystem maintainers?
Comment #34
drugan CreditAttribution: drugan as a volunteer commented@amateescu
Strictly speaking there was not a lot of feedback. @finne suggested to move min and max from placeholder to description and I'll do it a bit later. As for the too big summary we've agreed that this have sense and should be leaved alone. The error reported by @finne when running the field update with drush is only relevant to the Commerce 2.x version of the patch. Obviously the reason should be found even if the fields are updated as expected despite the error. I'll do it as soon as the patch will get some interest from the Commerce maintainers. If that will not happen then I'll create a module for this taking all the functionality from the current patch and some specific parts from the Commerce version. For now it just a temporary solution which I created for my site and shared with others who have issue with the drupalcommerce quantity field.
Again, I understand that the patch is inappropriate for a subsystem maintainer review because it is too big. Unfortunately I can't see how it could broken into sensible chunks. Hope it will be useful for some users at least in the form of patch.
Comment #35
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@drugan, I was talking about #20, where I suggested a very simple resolution for the Commerce problem that started all this, which was: 'step' not being configurable in the field widget.
Comment #36
drugan CreditAttribution: drugan as a volunteer commented@amateescu, in the #32 I've explained why it seems difficult for me to make it as a simple resolution. Also, I can't understand your "completely by design" argument against changes on the default value field. For now we have this field exposed to user on a number field settings page and it does not work. I don't believe that bad UX was intentionally designed by creators of the Drupal 8. Moreover some of them are agree that such "the design is flawed".
Comment #37
joekersHere's my attempt to strip out all the unnecessary stuff and just allow users to configure the step on the widget, as suggested by @amateescu in #18.
There should be some constraints and tests but I can add these once I know we're back on the right track.
Comment #40
drugan CreditAttribution: drugan as a volunteer commentedThis functionality is now moved to the Extended Number Field module.
Comment #44
scottsawyerHey Everyone, thank you for working on this. I definitely think that adding a "step" setting in core is the way to go. I don't understand how we get a fail with
Drupal\Component\Utility\Number::validStep
exception: [Warning] Line 47 of core/lib/Drupal/Component/Utility/Number.php:
Division by zero
Comment #46
mishac CreditAttribution: mishac commentedI had the same error that scottsawyer did, as well as an error when adding menu links that the "weight" field was an invalid number, because the step was set to "".
Here's a new version of the patch that fixed it for me.
Comment #47
drugan CreditAttribution: drugan as a volunteer commented@mishac
This functionality already exists long ago on the xnumber module which is fully supported as I use it on multiple sites and as a dependency for other my modules.
https://git.drupalcode.org/project/xnumber/blob/8.x-1.x/src/Plugin/Field...
Note that you can use the Xnumber field widget not only on the xinteger, xdecimal and xfloat types of the field but also on the core's integer, decimal and float types.