Add validation constraints to system.performance

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The changes to the kernel test are out of scope. The "fix" is wrong.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Dear bot, get lost.

1. +1 for @phenaproxima's proposal at https://git.drupalcode.org/project/drupal/-/merge_requests/7321#note_294324 to use DivisibleBy instead of Choice.

   IMHO it should be done here, not elsewhere, because none of the existing uses of the Choice constraint in Drupal core so far are using it to restrict it to a list of integers. In principle, there's nothing wrong with that, but it'll be very likely that values specified in config by hand will be considered incorrect. The "multiple of 60" rule that @phenaproxima proposes is much more elegant: it's lenient to allow any number, as long as it's with a granularity of 1 minute. That seems much better!

2. Also spotted one thing that's still lacking validation: https://git.drupalcode.org/project/drupal/-/merge_requests/7321#note_294477
3. And one thing whose validation is semantically incorrect: https://git.drupalcode.org/project/drupal/-/merge_requests/7321#note_294478 — but for which a correct example was introduced in Drupal core in #3412361: Mark Editor config schema as fully validatable :)

1. There's one last thing to address: https://git.drupalcode.org/project/drupal/-/merge_requests/7321#note_294486 :)
2. The MR needs to be updated to target 11.x.

Opened a new MR against 11.x, and added a constraint to validate whether a string is valid regex.

Can the MR be rebased? Believe the errors may be related to broken HEAD from the other day.

NW for Phenaproxima's comments.

That all sounds good to me. I also set 'regex' as a schema type so we don't have to add the constraint anywhere we might need to use it in the future.

As a side note, it seems weird to me that 'exclude paths' can't be empty/null, but I checked the code, and fast404 just doesn't do anything unless you exclude *something*. Seems like that could be adjusted, but out of scope here.

All feedback addressed. back to NR.

The failing test seems to be broken on HEAD, will rebase once it's fixed.

Rebased now that #3448036: InstallerTranslationExistingFileTest fails on 11.x branch is in. Tests should pass now.

I think this looks great, I agree that both html and regex being new types is a good idea for reusability. We should create change records for both of those new types.

Agreed on the CR.

Eventually would love if we could update the cheat sheet on https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... or start a new one.

CR's look good. I really like the sections "How to use?" even though pretty straightforward it's a nice touch.

Thanks!

Don't really agree with max age being limited to multiples of 60, left a comment on the MR.

The latest push makes the change I recommended above, constraining the value to an integer between 1 and 3156000 and reconfiguring the form to use a number field with the same constraints. I didn't see any specific tests around this form to adjust, but there might be a functional Javascript test somewhere that will need to be adjusted.

Reverted the form changes and moved them to a follow-up #3451592: Improve UX of system performance form max-age form input to determine how best to improve the form UX

Also reverted the changes to ConstraintManager that exposed DivisibleBy as we are no longer going to use it here.

Thanks to @catch and @phenaproxima for the slack discussion. Back to NR

Checked the CR and both read fine. "Generally speaking:" like those sections

Ran test-only feature and got https://git.drupalcode.org/issue/drupal-3437608/-/jobs/1740193 so definitely shows extensive test coverage.

Checking MR appear all feedback has been resolved.

Applied to 11.x with config inspector installed

LGTM!

I've added a couple of comments to the MR - minor test related stuff. Otherwise this looks great.

Back to rtbc now that the remarks have been fixed.

I think we need to think a bit more about HTML validation and what an html type means. Plus we have an issue with adding a new code dependency to the Utility component.

That makes sense.

IMHO, we should be looking at this as a snippet of HTML, rather than a full page. What would be a reasonable way of validating that, if we can't use a parser?

I also agree that we don't want to add accidental cross-component dependencies, so this probably needs some refactoring. Maybe a whole new constraint to confirm that something is a valid HTML snippet?

I'm going to move it to a full fledged constraint in Core/Validation to solve the dependency issue. To make it work for both snippets and full pages, I think we can have the constraint just wrap snippets in "<!DOCTYPE html><html><head></head><body>" .. "</body></html>". We could either define whether to do so in an argument to the constraint, or try to autodetect, but I'm going with a constraint argument that defaults to snippet because the schema would want to decide which it allows. The fast_404 html here would want a full page and want to reject a snippet so I don't see autodetecting being practical.

(Ironically, I was resurrecting #2852463: Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface the other day and I realized the we nearly did the same intercomponent dependency in the patch we committed and reverted all those years ago. We were going to call Component\Utility\NestedArray from Component\Plugin and didn't notice)

Found a lot of small things I think could be improved. But in general I agree with the approach here and the usefulness of these new constraints.

Did a codestyle fix and a rebase, since it was behind so much. That will create issues with lint jobs.

Processed all the comments, rebased, updated the CR's. Think this is ready.

I have one tiny nitpick on the comment, but I think @bbrala is right, this looks ready.

Some review comments to address. Two I would have fixed on commit. One which needs a little bit of thought hence back to needs work.

The suggestion in #53 makes complete sense to me, it's now updated so moving it back to rtbc.

The failure is unrelated afaik, and it succeeds locally.

I've tried to find out in the issue comments but I can't see where it is decided but I'm really surprised by the schema changing when the fast 404 is enabled or disabled. This is not a regular thing. Normally you can disable something and leave the other config in place. I'm not sure why the complexity has been introduced. Setting back to needs review to get an answer.

Hmm, the pattern here is indeed old. Didnt notice that. Used it in the other issues to just use $options instead. Can work with that. Wonder though if this pattern is inconsistent in other places also, would be nice to have it all the same.

The fast404 is indeed rather unique, i'll dig a bit, there is another branch that was closed, that split was committed bu phenaproxima. I can imagine why, where those fields are really only relevant when the configuration is set. But looking at the other schema commits it is not really standard. I'll dive into it, see if i can give you an awnser from the history here (or somewhere else0.

This is what i found out. It was committed by @phenaproxima https://git.drupalcode.org/issue/drupal-3437608/-/commit/faea4b55fedea2e...

I did find some other examples (yay):

core.date_format.*:

Where depending on the value is does

# Unlocked date formats should use the translatable type.
core_date_format_pattern.0:
  type: date_format
  label: 'Date format'

# Locked date formats are just used to transport the value.
core_date_format_pattern.1:
  type: string
  label: 'Date format'

Oh and another one!

editor.image_upload_settings.*:

So perhaps the idea was, lets only require those field when fast404 is enabled. But that could mean we should also tell the schema what the data should be if it is not enabled? Maybe something like same, but nullable. Then you don't really need to fill the keys.

Simplified and adressed feedback.

Hmm, I've been doing some things to see if i can double check how this works. I would say we should be able to not fill the other fields when disabled, which is kinda important. But after trying a few different iterations it seems it might now validate if i try to handle enabled and disabled properly.

I'll dive into this a little more and make sure i can prove validation works as expected when disabled. We could also still choose to first always validate the fields. But i'll see if i can proove this first.

Codedump for later

Fast404Test.php

    // Make sure we cannot end up with invalid config
    $config = $this->config('system.performance');
    $config->set('fast_404', ['enabled' => FALSE])
      ->clear('fast_404.exclude_paths')
      ->clear('fast_404.paths')
      ->clear('fast_404.html')
      ->save();

    # Should not be able to enable without the proper settings.
    $this->expectException(SchemaIncompleteException::class);
    $config->set('fast_404', ['enabled' => true])
      ->save();

system.schema.yml

# When `system.performance:fast_404.enabled = false`.
system.performance.fast_404.*:
  type: mapping
  label: 'Fast 404'
  constraints:
    FullyValidatable: ~
  mapping:
    enabled:
      type: boolean
      label: 'Fast 404 enabled'

# When `system.performance:fast_404.enabled = true`.
system.performance.fast_404.1:
  type: system.performance.fast_404.*
  label: 'Fast 404 settings'
  constraints:
    FullyValidatable: ~
  mapping:
    paths:
      type: regex
      label: 'Regular expression of paths to match'
    exclude_paths:
      type: regex
      label: 'Regular expression of paths to NOT match'
    html:
      # @see \Drupal\Core\EventSubscriber\Fast404ExceptionHtmlSubscriber
      type: html_document
      label: 'Fast 404 page HTML'

# When `system.performance:fast_404.enabled = false`.
system.performance.fast_404.0:
  type: system.performance.fast_404.*
  label: 'Fast 404 settings'
  constraints:
    FullyValidatable: ~
  mapping:
    paths:
      requiredKey: false
      type: config_entity
      label: 'Regular expression of paths to match'
    exclude_paths:
      requiredKey: false
      type: regex
      label: 'Regular expression of paths to NOT match'
    html:
      # @see \Drupal\Core\EventSubscriber\Fast404ExceptionHtmlSubscriber
      requiredKey: false
      type: html_document
      label: 'Fast 404 page HTML'

Ok, i've tested the results.

When defining the .0 state also we can validate the values, but allow them to be empty. This way we know for sure the config is valid, but if the values are not set and we enable we get an error.

1. When disabled it allows clearing the keys, but also validates if there is values. So i tried to add invalid HTML to the html key and got an error.
2. When disabled and cleared (emtpy keys), you can save, but if you enable again it will error out since the config is not valid.

For now the test is in the existing test, which although efficient, does seem as the wrong place, so we need to move that. Also i would prefer if we do some tests for the different possible combinations, showing it validates the fields when set, even when disabled.

Hmm, maybe the placement for the test is not really wrong, i'll keep it there for now.

Test fail seems to be unrelated, nightwatch is fun.

Ah, after rerun they are gone.

There's an open question on the MR, can we answer that?

Awnsered. :)

This looks great, thanks for answering that question.

Two nits, I think "fragment" is a more recognisable and well-defined term than "snippet".