Add validation constraints to dblog.settings

I took this one from DrupalCon Lille Contribution day

This change looks great, thank @hdnag!

Great to see a new validation! However I think the proper validation should be a number 0 or greater. We don't know if someone is altering this value with custom code and the only thing we need to ensure to have dblog working properly is row_limit an unsigned integer.

@dagmar, I have just pushed a commit with the requested change at #6.
It is an iteration of the change, but using the Range constraint plugin, so it is at least 0.
See https://git.drupalcode.org/issue/drupal-3395631/-/commit/f27c87b9a276613....

The changes were on the 11.x branch on the issue fork.
I cherry-picked the commit there to the 3395631-add-validation-constraints topic branch
I also took the chance to rebase on current 11.x.

Thanks @marvil07

Looks great! 🚢

We should also mark dblog.settings as FullyValidatable while we're at it!

@phenaproxima++

This now indeed needs the FullyValidatable constraint to be added to dblog.settings. See https://www.drupal.org/node/3404425 → .

Ship it!

Committed/pushed to 11.x, thanks!

I think this might have broken HEAD https://git.drupalcode.org/project/drupal/-/jobs/781239

Reverted for now - maybe it was just unlucky timing but can't think what else got committed to dblog recently.

    1) Drupal\Tests\dblog\Functional\DbLogViewsTest::testDbLog
    [Cache] Row limit variable of 1000 equals row limit of 100
    Failed asserting that 100 matches expected 1000.

🤯 I don't see how this change could've caused that test failure.

I've got the same failures in DbLogViewsTest::testDbLog and DbLogTest::testDbLog as @catch around the same time (https://git.drupalcode.org/issue/drupal-3420375/-/jobs/780745) and they were gone after a retest that was done after the revert (https://git.drupalcode.org/issue/drupal-3420375/-/jobs/782021).

A rebase later and the testresults now show this is causing the test-failure.

So this MR wasn't synced for about 1 month and was about 250 commits behind.

On drupalCI we tested merge-able patches every 2 days, that isn't done on GitLabCI apparently?

This is breaking because, when \Drupal\Tests\dblog\Functional\DbLogTest::verifyRowLimit() submits the form at /admin/config/development/logging, it actually runs into a validation error raised by the form, which it fails to catch because the status code is still 200.

That error is "'langcode' is a required key".

Makes sense - if you look at config_object in core.data_types.yml, you'll see that all config objects have a langcode key, and it is required (all keys of mappings are required by default unless they explicitly opt out): https://git.drupalcode.org/project/drupal/-/blob/11.x/core/config/schema...

So why didn't this break before? Because that required-ness is only checked for config objects that have opted into the FullyValidatable constraint, which until this issue did not include dblog.settings. But now that we're opting in, we're strictly checking it. And, like most config shipped with core, dblog.settings.yml doesn't include a langcode key. Some core config does, other core config doesn't.

So I'm thinking the solution here is to add langcode: en to dblog.settings.yml, which will make it consistent with some of the other config core ships.

We might also need an update path? Not sure...

Crediting myself for the debuggin'. :)

Oh, and about that update path...if we do it, we might want to do it in a separate issue, and have it ensure the langcode key exists in all simple config.

Otherwise we're almost guaranteed to run into this again as we make more and more of core's config schema-compliant, and then you gotta wonder if it's better to have lots of little update paths which all do the same thing (dear god, no)...or just have one big one that fixes this known schema compliance issue.

I see now. The last CI run was on January 5 (https://git.drupalcode.org/issue/drupal-3395631/-/pipelines/72532) for the last commit (https://git.drupalcode.org/project/drupal/-/merge_requests/5130/diffs?co...).

Somehow GitLab CI didn't re-test this in the month that passed since then? 😱

Surfacing that in the #gitlab Drupal Slack channel…

Oh, and about that update path...if we do it, we might want to do it in a separate issue, and have it ensure the langcode key exists in all simple config.

I propose we do this one separately first, and then a new issue for all others. That allows us to learn here and apply the pattern across all of core.

or just have one big one that fixes this known schema compliance issue.

That sounds too broad. But this one is particularly simple: literally ALL config objects must have a langcode key per the config schema:

config_object:
  type: mapping
  mapping:
    _core:
      # This only exists for merging configuration; it's not required.
      requiredKey: false
      type: _core_config_info
    langcode:
      type: langcode

Update path and update path test added.

I think the update path and update path test are good, and the change to the existing config will prevent this from happening.

The direct post-update for dblog.settings is OK here because it will catch exported config but doesn't need to be applied to shipped config from modules/install profiles.

Committed/pushed to 11.x, thanks!

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