- Issue created by @narendraR
- First commit to issue fork.
- Status changed to Needs review
5 months ago 6:34am 5 April 2024 - Status changed to Needs work
5 months ago 7:12am 5 April 2024 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.
- Status changed to Needs review
5 months ago 8:06am 5 April 2024 - Status changed to Needs work
5 months ago 8:47am 5 April 2024 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.
- Status changed to Needs review
5 months ago 6:44am 8 April 2024 - Status changed to Needs work
5 months ago 7:27am 8 April 2024 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.
- Status changed to Needs review
5 months ago 10:15am 8 April 2024 - Status changed to Needs work
5 months ago 11:39am 8 April 2024 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.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
The changes to the kernel test are out of scope. The "fix" is wrong.
- Status changed to Needs review
5 months ago 2:09pm 9 April 2024 - Status changed to Needs work
5 months ago 2:52pm 9 April 2024 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.
- Status changed to Needs review
5 months ago 3:05pm 9 April 2024 - Status changed to Needs work
5 months ago 3:47pm 9 April 2024 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.
- Status changed to Needs review
5 months ago 3:49pm 9 April 2024 - Status changed to Needs work
5 months ago 4:23pm 9 April 2024 - 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
- +1 for @phenaproxima's proposal at https://git.drupalcode.org/project/drupal/-/merge_requests/7321#note_294324 to use
DivisibleBy
instead ofChoice
.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! - Also spotted one thing that's still lacking validation: https://git.drupalcode.org/project/drupal/-/merge_requests/7321#note_294477
- 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 for @phenaproxima's proposal at https://git.drupalcode.org/project/drupal/-/merge_requests/7321#note_294324 to use
- Status changed to Needs review
4 months ago 5:01pm 2 May 2024 - Status changed to Needs work
4 months ago 10:25pm 2 May 2024 - 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
- There's one last thing to address: https://git.drupalcode.org/project/drupal/-/merge_requests/7321#note_294486 :)
- The MR needs to be updated to target
11.x
.
- First commit to issue fork.
- 🇺🇸United States mikelutz Michigan, USA
mikelutz → changed the visibility of the branch 3437608-add-validation-constraints to hidden.
- 🇺🇸United States mikelutz Michigan, USA
mikelutz → changed the visibility of the branch 11.x to hidden.
- Status changed to Needs review
4 months ago 10:58pm 8 May 2024 - 🇺🇸United States mikelutz Michigan, USA
Opened a new MR against 11.x, and added a constraint to validate whether a string is valid regex.
- Status changed to Needs work
4 months ago 12:32pm 10 May 2024 - 🇺🇸United States smustgrave
Can the MR be rebased? Believe the errors may be related to broken HEAD from the other day.
- Status changed to Needs review
4 months ago 3:09pm 10 May 2024 - Assigned to mikelutz
- Status changed to Needs work
4 months ago 11:44am 18 May 2024 - Issue was unassigned.
- Status changed to Needs review
4 months ago 1:10pm 18 May 2024 - 🇺🇸United States mikelutz Michigan, USA
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.
- 🇺🇸United States mikelutz Michigan, USA
The failing test seems to be broken on HEAD, will rebase once it's fixed.
- 🇺🇸United States mikelutz Michigan, USA
Rebased now that 🐛 InstallerTranslationExistingFileTest fails on 11.x branch Active is in. Tests should pass now.
- 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
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.
- Status changed to Needs work
4 months ago 10:11pm 20 May 2024 - 🇺🇸United States smustgrave
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.
- Status changed to Needs review
3 months ago 8:05am 29 May 2024 - Status changed to RTBC
3 months ago 2:38pm 29 May 2024 - 🇺🇸United States smustgrave
CR's look good. I really like the sections "How to use?" even though pretty straightforward it's a nice touch.
Thanks!
- Status changed to Needs work
3 months ago 9:10am 30 May 2024 - 🇬🇧United Kingdom catch
Don't really agree with max age being limited to multiples of 60, left a comment on the MR.
- Status changed to Needs review
3 months ago 11:57am 31 May 2024 - 🇺🇸United States mikelutz Michigan, USA
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.
- 🇺🇸United States mikelutz Michigan, USA
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
- Status changed to RTBC
3 months ago 7:15pm 7 June 2024 - 🇺🇸United States smustgrave
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
- Status changed to Needs work
2 months ago 9:00am 27 June 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
I've added a couple of comments to the MR - minor test related stuff. Otherwise this looks great.
- Status changed to Needs review
2 months ago 3:58pm 27 June 2024 - Status changed to RTBC
2 months ago 6:11am 28 June 2024 - 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
Back to rtbc now that the remarks have been fixed.
- Status changed to Needs work
2 months ago 7:31am 28 June 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
- 🇺🇸United States phenaproxima Massachusetts
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?
- Assigned to mikelutz
- 🇺🇸United States mikelutz Michigan, USA
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)
- Issue was unassigned.
- Status changed to Needs review
about 2 months ago 1:59pm 11 July 2024 - Status changed to Needs work
about 2 months ago 7:31pm 15 July 2024 - 🇺🇸United States phenaproxima Massachusetts
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.