Add validation constraints to core.menu.schema.yml

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.

Update path from the start — well done!

But a review revealed some of the validation logic added here is incorrect. And sadly, that review unveiled that needs to be added to the growing list at 🐛 book.settings, contact.settings, filter.settings, search.settings all violate the basic rule of simple config: they cannot have dependencies Active 😬

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.

Can the MR be updated for 11.x please

We're almost there, but … I spotted several problematic things that suggest there's ecosystem disruption in this MR. If we're lucky, then most of my feedback can be addressed by adding comments and deleting unnecessary changes 🤓🤞

See commends on the MR re: the upgrade path.

@catch: I see the pattern, but in this case we're not dealing with config entities, but simple config:

core.menu.static_menu_link_overrides:
  type: config_object

So AFAICT your concern just doesn't apply here? 😄

Errr good point.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

No wait it does... committed then reverted again.

See the changes in standard and umami shipped config. I think it needs an event listener for the config save event instead of a hook_ENTITY_TYPE_presave(). Otherwise how do install profiles/distributions know that their config needs to be updated?

Ahhh, you're right — it's not a hook_ENTITY_TYPE_presave() we need, but a generic config event save listener indeed 🙈

My bad, sorry! 🫣

I am not sure, what should be written on config save:

    $config_name = $event->getConfig()->getName();
    if ($config_name === 'core.menu.static_menu_link_overrides') {
      // What needs to be done here?
    }

@narendraR we'd move the logic from config_post_update_set_menu_parent_value_to_null to change the values when it detects they're wrong, then the post update would just load and save the config.

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

Looks good, just some minor docs and test changes required.

Looks good. 👍🏻

Couple of comments on the MR about the bc layer.

My only question is the wording of the deprecation "and will not be allowed in " I didn't anything example of that in core but can't think of what should replace it. Maybe "Will be required in.." Will leave in review for other thoughts.

So don't want my one question to hold this one up.

I've added some review comments to the MR.

Feedback from #3441434-40: Add validation constraints to core.menu.schema.yml → are addressed, only a small nit, after that it can be self-RTBCed

Deprecation should point to CR and not issue link.

Change record created [#3458722].

The change record looks good, except the status needs to be published before used in a deprecation.

Change the status of CR to be published.

Looking good.

CR should not be in published state, it will be published once issue is fixed

My bad #46 📌 Add validation constraints to core.menu.schema.yml Needs work , changed the CR status

I think we need to allow empty strings for parent as this should not be changing APIs.

I tried to use AtLeastOneOf constraint here but I faced two problems here :-

1. AtLeastOf constraint requires an array of constraint objects and we were getting constraint arrays here. So what I did was implemented a plugin class for AtLeastOneOf, and converted the constraint arrays into constraint objects.
2. RecursiveContextualValidator prevents us from using custom groups. But we want to use AtLeastOneOf constraint there and I think it's validator requires every constraint to have a group. So for that I created this follow-up 📌 Remove the restriction from RecursiveContextualValidator that prevents using custom groups. Needs review .