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

Issue fork drupal-3522116

Command icon 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

murz created an issue. See original summary.

murz’s picture

Status: Active » Needs review

The validation error appears in the core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/PrimitiveTypeConstraintValidator.php file when it uses the function filter_var() having the empty string ('') in the$value:

    if ($typed_data instanceof IntegerInterface && filter_var($value, FILTER_VALIDATE_INT) === FALSE) {
      $valid = FALSE;
    }

So, seems the easiest solution is to cast the value to the "int" type (and the same - for float), something like this:

    if ($typed_data instanceof IntegerInterface && filter_var((int) $value, FILTER_VALIDATE_INT) === FALSE) {
      $valid = FALSE;
    }

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.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks 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

murz’s picture

Status: Needs work » Needs review

I 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.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests +Needs Review Queue Initiative

Removing 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

quietone’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs title update

I 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.

murz’s picture

Title: ConfigFormBase produces an error when saving empty number value from the form to a numeric schema item: This value should be of the correct primitive type » Fix validating the number shema items to accept empty strings
Status: Needs work » Needs review

I updated the issue title, please check if the name suits the rules now.

murz’s picture

Attach 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.

murz’s picture

Title: Fix validating the number shema items to accept empty strings » Fix validating the number type schema items to accept empty strings
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs title update

Title has been updated

penyaskito’s picture

Status: Reviewed & tested by the community » Needs review

How is that casted back to a numeric then? Do we need to use int|string everywhere?
I'm not sure this is the way forward. I'd prefer it to be nullable than allowing a string.

smustgrave’s picture

Lets see if we can get sub-maintainer to weigh in

alexpott’s picture

my_module.my_config:
  type: config_object
  label: 'My Config'
  mapping:
    my_int:
      type: integer
      label: 'My int'
      nullable: true

Is 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..

smustgrave’s picture

Status: Needs review » Postponed (maintainer needs more info)

any objection to #16

smustgrave’s picture

Status: Postponed (maintainer needs more info) » Closed (works as designed)

Since 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!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.