#Configuration schema

rebased, only to see if the unrelated failures have been fixed.

I think I've fixed all the feedback so far.

After some discussion with @lauriii, the scope here can be reduced further. Wim pointed out that the "rendering algorithm" came sorta out of nowhere, so I asked @lauriii about it and he came up with the rendering flow he would actually like XB to implement. It's not in scope here, so I'm punting that to a separate issue that I have yet to open.

So indeed, this issue is just about adding the ContentTemplate config entity (the class name was blessed by @lauriii), but not actually using it anywhere. That's a reasonable first step.

Wonder, maybe 📌 Add NotBlankAfterInstall constraint and use it for system.date:timezone.default Active is relevant for this.

I think this change is so minimal i can RTBC again. I ran the upgrade test after moving the hook code to SystemHooks.php and it upgraded fine.

Thanks, you are right. By now we have the OOP hooks. I think this issue is present in the other validation issues also. I'll do a round.

We need to move the hook implementation to the correct new OOP place... \Drupal\system\Hook\SystemHooks

Rebaed.

Importer::copyFileAssociatedWithEntity requires a file to exist on disk in the "default content" directory.

@lauriii has indicated many times now that that is a "hard no" from him. All XB config MUST be exportable, importable, shareable. That means #16's proposal does not meet his product requirements.

https://git.drupalcode.org/project/experience_builder/-/commit/8ecc46f91... solved it. We "just" need to port it into the XB codebase.

Rebased.

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.

OK, this is pretty much where I want it. After discussion with Lee and Matt, I don't think this needs anything more than kernel test coverage. We don't need to test actual rendering yet, since there's nowhere for the template to store the component tree (yet). We just need to ensure that the correct template is chosen in specific situations.

Left some comments, looking slick

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

I think this should be closed. When I look at the parent issue, there is loads of places where this validator is triggered in the tests showing it working.

Examples:
https://git.drupalcode.org/project/drupal/-/merge_requests/3047/diffs#25...
https://git.drupalcode.org/project/drupal/-/merge_requests/3047/diffs#25...
https://git.drupalcode.org/project/drupal/-/merge_requests/3047/diffs#25...

And quite a few more. Think this might be overly aggresive.

Self-assigning to start on this.

Ah, after rerun they are grene.

Test fail seems to be unrelated, nightwatch is fun.

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

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.

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'

Did a rebase.

Fun fact, the failing tests seem to indicate there is test coverage of some sort for this constraint. And might have surfaced a bug on one of the test modules (missing a dependency)

Simplified and adressed feedback.

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.

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.

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.

Using config inspector I see we are at 100% now.

Believe all feedback has been addressed

all failures unrelated.

Did all the promotions, time to go home :)

Left 1 question and 1 comment on the MR.

Pipe is all green after the upgrade stuff in the child. Think we are there.

All is green, gonna set to needs review for visibility on the one part that still needs decision based on review.

Closed MR, and fixed phpstan issue.

bbrala → closed merge request !7107

The failure is unrelated afaik, and it succeeds locally.

@callumharrod walked us (@lauriii, myself and David Bee) through the designs he crafted. All of us asked various questions. The PoC here is still aligned with the designs, with one exception: we're not going to be exposing a component instance as an exposed slot, but a slot in a component instance.

Walked @phenaproxima through that change, and captured this change with him together, as a way to hand this off to him: https://git.drupalcode.org/project/experience_builder/-/merge_requests/7... :)

(I'm still around, obviously!)

Gave some preliminary feedback on Slack, if it's not enough then we can quickly have a look in person in Leuven a week and a half from now. My only concern is that a too loose validation would allow config to contain values that the code does not handle well (or at all). So as long as that concern is taken care of, I'm fine with this.

After asking in slack it looks like we could document this over at https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... →

I think we can document it there.

Oops, thanks, juggling a lot of issues, will cycle back.

Child is committed

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

I pinged kristiaan in slack, and in the meanwhile I figured there may be something else that we could do.
Instead of doing a min check here, we just need to know that the value is > 0, because smaller is not an option.

So I think isPositive might be a simpler way to validate this instead of being this specific.
However, waiting to put this back to needs work based on feedback.

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.

Do we need to document this somewhere? If so, where? If not, should we close this?

rebased and left question regarding possible solition for file vs directory

Latest MR appears to have pipeline issues. Can 1 MR be closed or hidden also to make it super clear what should be reviewed

Followup was made, and this issue was du-plicated by: 📌 Tighten config validation schema of system.mail mailer_dsn Fixed .

Fixed credits and closing as dupe

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

Ok, im going to try and rebase this.

I think this looks good, I don't think we need specific test coverage for these values.

I wonder if we should ask for a user module maintainer for a review? Will ping kristiaan.

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

Handled all the comments and rebased. Think this is ok.

Let's first rebase and see what happend.

Extracted the validator so naming makes sense. Split tests also and did a rebase so we run the right tests.

Committed 849a687 and pushed to 11.x. Thanks!

This looks great, thanks for cleaning this up.

Postpone on child issue.

Added the changes from the parent. Added a change record. Added a test for the upgrade path.

📌 Remove system.file.path config from system.schema.yml Active created to do the deprecation and upgrade path

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

Ok, lets try again. Seems good now.