Add validation constraints to olivero.settings

Initial commit adds validation for color_hex base type, and hence covers base_primary_color.

All feedback has been resolved with @see comments.

Before:
olivero.settings 83%

After:
olivero.settings 100%

I validated that the choices described in the schema constraints are the ones available at the form.
This looks great to me.

A doubt that I have is:
If e.g. I had to alter a form for adding an extra option in olivero settings form in my custom module, I probably need to implement hook_config_schema_info_alter to alter that constraint. I'm not sure if we have already docs/change records including that, but probably this is something worth to document.

Changes look great, rtbc ++

If e.g. I had to alter a form for adding an extra option in olivero settings form in my custom module, I probably need to implement hook_config_schema_info_alter to alter that constraint. I'm not sure if we have already docs/change records including that, but probably this is something worth to document.

This is a general issue with a lot of the validation constraints we are adding, I agree this makes some things more difficult.
I'm not sure how we can make this more strict and also keep the freedom the add extra options there. @Wim Leers might have an idea.

Sorry, I spotted a few small problems 😅

If e.g. I had to alter a form for adding an extra option in olivero settings form in my custom module, I probably need to implement hook_config_schema_info_alter to alter that constraint. I'm not sure if we have already docs/change records including that, but probably this is something worth to document.

+

I'm not sure how we can make this more strict and also keep the freedom the add extra options there. @Wim Leers might have an idea.

This is another reason that using the Choice constraint is so great: that makes it easy to declaratively add additional valid choices :)

Have created ✨ Allow three character hex colour definitions in Olivero settings Active to discuss making Olivero consistent with Image module's colour validation.

Postponing this issue based on the outcome of that one.

Whilst 3 character HEX codes are a thing, Olivero does not currently accept them

Well in that case … we don't need to wait for that! 😊

What matters is that the validation constraints match the expectations of the logic. 👍

Marking for that leading : in the message.

@wim-leers I think this still should be postponed, because this MR adds validation to the core config type of colour-hex.

That is used in two different places with different logic. Therefore we can't match the expectations of the logic until they are synchronised in ✨ Allow three character hex colour definitions in Olivero settings Active .

If we proceeded as-is with this change, config that used three character codes in image style configuration would suddenly become invalid.

Oh, that makes sense — I didn't realize that 😅! Thanks, @Eli-T 😊

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

Blocker's in!

Note: we should update this to use #config_target in Olivero's settings form (which only became usable after that MR was originally created, see the CR at https://www.drupal.org/node/3373502 → ), and remove the validation logic from the form.

I've removed the FullyValidatable from olivero, judging from the test failures, we first need to take care of theme_settings, but I don't think this issue needs to wait for that.

It's a bit odd that mobile_menu_all_widths is type: integer. I think type: boolean would make more sense. But that'd be both unnecessary and scope creep.

I've removed the FullyValidatable from olivero, judging from the test failures, we first need to take care of theme_settings, but I don't think this issue needs to wait for that.

I first wanted to disagree 😇🤓 But then I looked at the test failures and was forced to agree with you! 😄 Once type: theme_settings is fully validatable, we'll indeed be able to mark olivero.settings as fully validatable too 👍

Oops! I forgot the #config_target bit I mentioned in #20, and which convinced @longwave to land the blocker at ✨ Allow three character hex colour definitions in Olivero settings Active 😅

This MR should therefore update olivero_form_system_theme_settings_alter() to use #config_target to use constraint-based validation. Which also means we should be able to remove olivero_theme_settings_validate() here.

Since this is the first theme settings form using this, and

Colors must be 7-character string specifying a color hexadecimal format.

does not occur in any test, let's add a functional test to prove that the validation error correctly appears. That will also help with converting type: theme_settings to use validation constraints (especially given Olivero is the default front end theme) 👍

Akhil Babu → made their first commit to this issue’s fork.

I have updated the regex and added a test to valiate the hex codes.

Excellent work here, @Akhil Babu! 😊 Thanks for writing those tests!

There's one thing to nitpick (the assertion for the message could be more precise and hence more robust), but that's IMHO not commit-blocking. 👍

Thanks @Wim Leers.
Updated the method but now there is a strange Nightwatch test failure.
https://git.drupalcode.org/issue/drupal-3395555/-/jobs/661127

That looks like a random test failure — chromedriver-powered tests are notoriously fickle.

Re-running the Nightwatch tests — I expect those to pass now.

Turns out that single failure was a regression in Drupal core, and the commit has since been reverted: #3413665-10: Enable modules through Nightwatch API when not testing module enabling → .

Green CI again! 👍

For type: theme_settings: 📌 Add validation constraints to `type: theme_settings` Active .

Nearly committed this but actually have one question on the MR.

🏓

OK those are good reasons, but let's add them to the comment.

Re-RTBC'ing — I didn't touch this MR, I was only a reviewer. I provided the rationale for me RTBC'ing it despite the thing @catch rightfully questioned, and added that as a comment 👍

Committed/pushed to 11.x, thanks!

Yay!

This inspired a follow-up 🤓 — see [#3420770.

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