🇳🇱Netherlands @bbrala

This issue needs to be checked if the problem is solved in the child issues or not. Not always that clear.

Fixed the phpcs issues.

Adressed the feedback.

Changes look good, fixed one test, i think we should be green now. Preemptively setting to NR

I honestly hate how the error measages are. This was quite the challenge to update the expected errors.

I think we are green now, but ill have a look at the changes first to see if i didnt add changes that sign it might be a bit break.

Nightwatch tests fail sometimes ramdomly, at that point, just hit retry and they will succeed most of the time.

Made selection code better and added comments on how it works.

Getting a failure on the Configuration tests. But not sure why, perhaps because the fixtures need to change since default config changed for taxonomy config?

Another option for some will be suing AtLeastOneOf, where you can combine a validator with AllowedValues: - ''. Which would allow empty string. If we combine that with All which we can probably support the same way as we are doing ValidSequenceKeys that should at least be able to cover a lot of permutations of this.

📌 Add validation constraints to system.file Active landed.

See attached screenshot, some help tests.

When checking out the MR i still see quite a few mentioned of "Update Manager module". Also in doc comments, but that could be intentional. But also in a few help_topics.

I do think we need a little more changes, unless this was intentional, but don't really read that in the issue.

Now running on onSave in the updateAll method.

Also added an extra check for when we have no reference from system.rss and there is an available view_mode called default.

Still need to handle 2 things:

1. When default is the viewmode, and we have no config this will keep updateing to the first key. Oops.
2. Run this noSave

🐛 Fix installation error with JSON:API Extra in custom Drupal 11 profiles due to missing synthetic service `kernel` Active isnt that related?

If we can move to the more generic setRebuildNeeded that would be great. Don't see an issue in your approach.

For reference. This was the commit:
🐛 ResourceTypeRepository wrongly assumes that all entity reference fields have the setting "target_type" Fixed : Issue #3057545 by acbramley, hchonov, bbrala, bradjones1, larowlan, yogeshmpawar, Leon Kessler, gease, joachim, gabesullice, kfritsche, jibran, Wim Leers, Berdir, smustgrave, alexpott, catch: ResourceTypeRepository wrongly assumes that all entity reference fields have the setting "target_type"

Hmm, not sure about this fix, think I need some help here.

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

CR has been updated.

📌 Add validation constraints to core.menu.schema.yml Needs work landed.

I think i adressed the last comment and made it quite a bit simpler code.

I've got some questions about your approach, there might be more gotcha's (and possibly a bug :P)

fixed tests, so setting to NR

Yeah I think this one is very useful in a lot of cases. But there are more if you look at the linked meta. Which there you think would help most?

Well, seems i have a working test. While going through the motions i did find a one more view that use default still.

views.view.taxonomy_term.yml

      row:
        type: node_rss
        options:
          relationship: none
          view_mode: default

Also a little confused. If i look at system.rss.yml the default should be rss i think, since that was in the config. But when running the update test it gets set to title, i really don't understand how that is possible. What am i missing there?

Went through the threads, all good. Tried to go through credits, unsure of yash.rode and smustgrave.

All seems good, tests are green, change is minimal now :) Lets goo ^^

Cr needs update also since we do allow empty string when I look at the Mr comments by alex

📌 Remove the restriction from RecursiveContextualValidator that prevents using custom groups. Needs review has been committed. This needs a rebase and probably a change since the api changed a little.

Unrealted failure, all good imo.

All green after cleanup. But to much to set rtbc right away

LEts expand test coverage here yeah and validate.

Think nullanble is not allow, but willl cycle back later.

Fixed a few issues, but still need to make the constraint container aware to inject the entitty type manager and use that in the validator.

📌 Add validation constraints to system.file Active landed.

Test failure was unrelated nightwatch test. Rerunning, but seen that one too much to worry.

📌 [META] Support all composite Symfony contraints Active

bbrala → created an issue.

Since the changes were so minimal, setting back to RTBC, ill also make an issue for the Composite contraints

Published a change record →

Awesome. :)

We perhaps should make a meta issue to see what metrics would be usefull to expose in the different jobs. There is possibly a lot more :)

Think this is a release blocked then.

Little more context; https://drupal.slack.com/archives/C079NQPQUEN/p1744791038514039

Added a change record.

All tests are green. Which is great.

Not sure about the upgrade path. I'll ask around

Pushed to a MR with some small changes.

Upgrade path is tested afaik, so removing that tag. Has tests it seems, so removing that tag also. Did small update to IS, but don't see how we can make this more clear.

This will need a CR though. Dont think we need to deprecate anything really, since this is pretty much broken functionality you cant rely on.

Sorry, didnt switch branch for another issue. Reverted

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

Currently the validation is not enabled from the config import event. See 📌 Validate configuration schema before importing configuration Needs work . So your usecase is currently not supported. We can validate in the linked issue, but that would also now need to adjust to validate when we have a fully compatible and well defined schema for the config in question. Right now not all config is validatable yet, which would probably mean havoc everywhere.

You say you get -0 as a result, or just 0? What is the value in the config you have? Since -0 does feel impossible to set through UI.

Fixed theretical bug that might occur when we run out of phpstan errors. Unlikely, but possiblee.

Keeping RTBC since the change was so minimal.

I should confess i first read your message as the validation crashed, but you mean it is not 100% :)

This goed a little deeper. I dont see any third_party_settings validated in other config, only the actualyl settings when set, so this seems expected. There is also;

🐛 third_party_settings in THEME.settings.yml cannot be dependency managed Needs work
✨ 3rd party should be allowed to add config dependencies Active

Which expose some issues around third part settings. So i don't think we should block on the fact the parent key is not validated.

Yeah, might as well, good one.

Applied suggestion for grammar and adjusted contructor.

We can decide the namespace, i think this placement is all good.

Ahh, there.

Hmm, ok, seems weird but can look into that. I'd assume that is validated since it is not overwritten for the theme_settings type.

Thanks, I cab work with this.

Can you help me reproduce locally?

I tried to reproduce this, but eve if i import with a string it ends up as integer and i can do changes.

Can you make a better reproduce step so i can see it locally?

What was the message there?

The validation should still be OK, since:

olivero.settings:
  type: theme_settings

Which means core.data_types.schema.yml

theme_settings:
  type: config_object
  mapping:
...
    third_party_settings:
      # Third party settings are always optional: they're an optional extension
      # point.
      requiredKey: false
      type: sequence
      label: 'Third party settings'
      sequence:
        type: theme_settings.third_party.[%key]

Which then should resolve into:

shortcut.schema.yml

# Schema for theme settings.
theme_settings.third_party.shortcut:
  type: mapping
  label: 'Shortcut settings'
  mapping:
    module_link:
      type: boolean
      label: 'Add shortcut link'

I also tried to proove the issue in the test, but seems fine. I'm confused.

I love i see metrics report now in the mr. Marked as new as expected :)

Approved.

Added some feedback to the MR

Go away nr bot.

I think the review bot is wrong, the text file also stated no errors. Still merged 11.x again though.

Hmm, not sure yet how to do this. CKEditor uses HTMLRestrictions for a lot of generation of the lists of tags. There is also the config form, and the SmartDefaultSettings. Those probably need adjusting to make sure we get sorted tags.

Good idea.

Getting the tags sorted in ckeditor requires some extra work since it is only implicitly coupled to the filterfotmat in core. There is an issue around that but lets do make sorting available publicly so we can also do it in that part of the code.

Ok, the failures are in sort order, which is great! Adjusting the tests to match the new order is a good novice issue. Go through the tests that fail and update the expected order.

I'm not sure if we need an upgrade path, since an export will just save it the new order easily. :)

This issue confuses me, there was a patch, but looking at the current MR i see quite a few things from there missing. This issue needs some investigating to see if we are not missing loads.

Not unexpeceted since its so old :)

bbrala → changed the visibility of the branch 2385047-remove-disabled-filters-10.2.x to hidden.

bbrala → changed the visibility of the branch 2385047-remove-disabled-filters-11.x to hidden.

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

All green.

1. Skip validation on an intentional broken view.
2. Fix multiple views where plugins were not existing.
3. Adjust one test which seemed reasonable to have that as output. Think that is no BC

Think this is good to go.

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

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

Added an update hook and test, also tested it locally on a clean install.

Went through your (thorough) review, this made the a few things way better. I have a response to two of your suggestions which i think might have issues if we do that.

How did you run into thie @wimleers? And did you end up doing something to support it in xb?

Than you so much for the review :)

Issue itr was postponed on was fixed, this can actually be worked on.

Kinda like what the AnyOf of symfony does. Have 2 places where something similar is coming:

📌 Add validation constraints to core.extension Needs review
ValidSequenceKeysConstraintValidator

And 📌 Remove the restriction from RecursiveContextualValidator that prevents using custom groups. Needs review .

Kinda like the compound validation errors there. We did need a added __cone method there, but seems a resonable way to do multiple.

Seems the open comments are all correct, closed one. Seems this is the right way, but needs work still.