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 ✨ [PP-1] Mark parts of CKEditor 5 and Editor config schema as fully validatable Postponed :)

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.

mikelutz → changed the visibility of the branch 3437608-add-validation-constraints to hidden.

mikelutz → changed the visibility of the branch 11.x to hidden.

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 🐛 InstallerTranslationExistingFileTest fails on 11.x branch Active 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 📌 Improve UX of system performance form max-age form input Active 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.

Feedback addressed.

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 "" .. "". 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 📌 Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface Needs review 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.