Problem/Motivation
I have a number type in my config schema, that is not required, so can be empty. And created a config form to manage the value. It works well if I fill the number value, but doesn't work if I keep the value empty, producing an error:
This value should be of the correct primitive type.
Steps to reproduce
1. Create a schema: my_module.schema.yml:
my_module.my_config:
type: config_object
label: 'My Config'
mapping:
my_int:
type: integer
label: 'My int'
2. Create a config form to manage the value - Form/MyConfigForm.php:
namespace Drupal\my_module\Form;
use Drupal\Core\Form\ConfigFormBase;
use Drupal\Core\Form\FormStateInterface;
class MyConfigForm extends ConfigFormBase {
public function getFormId() {
return 'my_config_form';
}
public function getEditableConfigNames() {
return ['my_module.my_config'];
}
public function buildForm(array $form, FormStateInterface $form_state) {
$form['my_int'] = [
'#type' => 'number',
'#title' => $this->t('My Int'),
'#config_target' => 'my_module.my_config:my_int',
];
return parent::buildForm($form, $form_state);
}
}
3. Open the form page, type any number into the field value, submit - the config will be updated successfully.
4. Make the field value empty, submit - the form will produce the validation error.
Proposed resolution
Seems this issue is related to the validation of the individual configuration values, and casting '' (empty string) to NULL.
I found a pretty old post in the issue https://www.drupal.org/project/drupal/issues/2130811#comment-8280187 related to this, and in this issue the problem with casting '' to NULL was resolved even for 8.0.x-dev, but seems not fully, or there is a regression.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | drupal_3522116-fix-validating-number-shema-item-as-empty-sting_1.patch | 3 KB | murz |
Issue fork drupal-3522116
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #3
murzThe validation error appears in the
core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/PrimitiveTypeConstraintValidator.phpfile when it uses the functionfilter_var()having the empty string ('') in the$value:So, seems the easiest solution is to cast the value to the "int" type (and the same - for float), something like this:
I prepared a MR https://git.drupalcode.org/project/drupal/-/merge_requests/11998 with this fix, which resolves the issue. But not sure that this is the correct fix, and seems we should cover this with tests, so marking the issue as "Needs Review" to get the feedback.
Comment #4
smustgrave commentedThanks for reporting and getting an MR already. Appears to have a pipeline failure.
Also can we get a test case showing the problem as a next step.
Thanks
Comment #6
murzI created a separate MR https://git.drupalcode.org/project/drupal/-/merge_requests/11998 representing the issue and the failing test.
And updated the MR to fix the issue and make the same test pass.
Please review.
Comment #8
smustgrave commentedRemoving tests tag after running test-only feature https://git.drupalcode.org/issue/drupal-3522116/-/jobs/5129880
Change seems straight forward and didn't break existing tests so don't think have to worry about breaking other stuff.
LGTM
Comment #9
quietone commentedI have only read the title here and tagging for a title update.
The title should be a description of what is being fixed or improved. The title is used as the git commit message so it should be meaningful and concise. See List of issue fields.
Comment #10
murzI updated the issue title, please check if the name suits the rules now.
Comment #11
murzAttach a static patch file with those changes, to make it possible to use in composer until #3204538: GitLab Merge Requests Unable to Generate Incremental Patch Files is resolved.
Comment #12
murzComment #13
smustgrave commentedTitle has been updated
Comment #14
penyaskitoHow is that casted back to a numeric then? Do we need to use
int|stringeverywhere?I'm not sure this is the way forward. I'd prefer it to be nullable than allowing a string.
Comment #15
smustgrave commentedLets see if we can get sub-maintainer to weigh in
Comment #16
alexpottIs what you want here. We already have test coverage... you can see this in action by enabling the config_test module...
\Drupal::configFactory()->getEditable('config_test.types')->set('nullable_int','')->save();.I think this issue should be closed works as designed..
Comment #17
smustgrave commentedany objection to #16
Comment #19
smustgrave commentedSince there's been no follow up going to close this one out. If still a bug or #16 doesn't address it please re-open
Thanks all!