Menu changes from node form leak into live site when creating draft revision

#2315773: Create a menu link field type/widget/formatter could potentially solve that

I saw catch's Planet post about this issue and/or related ones, and I had a thought, so posting it on this issue...

It seems like what this issue and #2856363: Path alias changes for draft revisions immediately leak into live site both need, as well as #2858431: Book storage and UI is not revision aware, changes to drafts leak into live site, is some kind of generic system that associates node revisions with the state of other systems, so that if you have revision A published, the menu/alias/book nav system is in state A, and if you later publish revision B (which could be a newer or older revision), the menu/alias/book nav system is in state B.

Would it be possible to define a type of plugin called a NodeRevisionAssociatedState (or whatever) that would handle this? So the Path module could define one plugin that would handle associating an alias with a revision, and the Menu Link module (or whatever module it is that does menu links these days) could define one that would handle associating a menu link with a revision. Methods this plugin would need:
- getState() -- given a node revision or a node form or something (??), return a PHP data structure, or a YAML string, or something (??) that could be saved with the revision to keep track of this plugin's data.
- setState() -- the reverse: pass in the saved state information (whatever it is) and initialize the plugin.
- publishRevision() -- this revision is being published, so actually make the necessary changes (save the path alias and get rid of the old one, or update the menus, or whatever)
- nodeFormAlter() -- what to put on the node form [the path alias form, the menu form, etc.] [how are these currently handled??]

Those methods are probably not quite right, but in any case, the revision system would need some way to save the "associated state" information for all plugins that are registered, and some way to publish a revision and have the plugins update their "associated state". I guess this could also be more generic than just nodes... could be EntityRevisionAssociatedState or whatever.

Just a thought... not sure that is the right approach, but it seems like it might be an avenue to pursue? Better than having each system make its own approach, it seems?

@alexpott, @catch, @cottser, @xjm and I considered this issue and agree this is suitably a critical priority bug because it is a data integrity issue when information is menus change when you don't intend them to change.

@jhodgdon, that's a very interesting suggestion. It dovetails a bit with the concept of workspaces that we're working on, in the sense that the 'State' from your proposal would be a workspace. Let's discuss this more in #2784921: Add Workspaces experimental module.

In the meantime, here's a patch that works the same as #2858431: Book storage and UI is not revision aware, changes to drafts leak into live site and prevents any change to the node menu link when a non-default revision is saved. And just like that issue, we could really use #2883868: Content Moderation decides to set a new revision as the default one way too late in the entity update process so we can throw an entity validation and not let the entity form be submitted instead of just showing an error message after the fact.

Tested the patch in #6 manually and getting error message as expected. +1 to RTBC, if we are agreed on the solution of restricting to change (solution 1 in issue summary).

+++ b/core/modules/menu_ui/menu_ui.module
@@ -366,6 +366,14 @@ function menu_ui_form_node_form_alter(&$form, FormStateInterface $form_state) {
+    drupal_set_message(t('This is not the default revision. You can only change the menu settings for the <em>published</em> version of this content.'), 'error');

This message is thrown even if the menu settings havn't changed.

Also, I'm not sure about the wording. It first mentions about not being the "default version" then talks about only being able change the "published version". Then with Content Moderation they can't go back and edit the published version, they can only edit the latest. So I think we need to say, publish to update the menu settings. Although that's not strictly true, they just need to change to a moderation state which updates the default revision, which might not publish the entity, or even be the published state.

(Same review as #2858431: Book storage and UI is not revision aware, changes to drafts leak into live site)

I was talking to Gabor about the wording, he suggested something like "You cannot change the menu options until you publish this Content."

Although with Content Moderation "publish" might not be the right verb, so it'd be nice if for Content Moderation we could swap it out with "You cannot change the menu options until you move this content to the moderation state: Published" (or whatever the state is).

Fixed #9. As with #2858431: Book storage and UI is not revision aware, changes to drafts leak into live site, I don't think this should be blocked on #2875154: Clarify and document wording around drafts, revisions, published & friends.

Like #2858431: Book storage and UI is not revision aware, changes to drafts leak into live site lets RTBC this too.

@catch form validation is not possible because of #2883868: Content Moderation decides to set a new revision as the default one way too late in the entity update process.

This is marked as MUST-HAVE in the roadmap.

I'm wondering whether there could be an API which you can call to "ask" whether there will be a new revision. This API could then be implemented somehow by content_moderation .

#2883868: Content Moderation decides to set a new revision as the default one way too late in the entity update process was merged, this can now be properly "fixed" with a validation error.

Postponing on #2894634: Allow entity-level validations to flag errors on form elements

That's been committed

Then let's go with a validation constraint! :)

Manually tested, works perfectly and code looks great.

+++ b/core/modules/menu_ui/tests/src/Functional/MenuUiContentModerationTest.php
@@ -0,0 +1,102 @@
+  public function testMenuUiWithForwardRevisions() {

This is not covering parent, description and weight changes.

@plach, indeed, let's add test coverage for those as well.

Nice!

Just found a bug with this issue when working on #2766957: Forward revisions + translation UI can result in forked draft revisions.

Steps to reproduce:

1. Create english (draft)
2. Create french (publish)
3. Create french (draft)

The only field I change in these three saves is the title field. On the last save it'll throw You can only change the menu settings for the published version of this content.

Looking for the cause of this now.

Turns out this is not translation related, it happens to any forward revision which has not ever had menu link settings, the validator assumes the settings have been deleted.

This seems to fix it, doesn't seem like the most elegant solution though.

Oops, thanks for finding this @timmillwood :)

+++ b/core/modules/menu_ui/src/Plugin/Validation/Constraint/MenuSettingsConstraintValidator.php
@@ -30,7 +30,7 @@ public function validate($entity, Constraint $constraint) {
-      elseif (empty($values['enabled'] && $values['entity_id'])) {
+      elseif ($defaults['entity_id'] && empty($values['enabled'] && $values['entity_id'])) {

The first check, on $defaults['entity_id'] is not needed, we have to fix the following empty() call instead :)

And let's make the 'update' hunk a bit more clear.

Ouch, sorry for missing the no-menu case, I tested #32 and I confirm it handles that correctly now. However, while manually testing #32 I found a case that was not covered by tests. Attaching a patch to fix that.

+++ b/core/modules/menu_ui/src/Plugin/Validation/Constraint/MenuSettingsConstraintValidator.php
@@ -26,7 +26,7 @@ public function validate($entity, Constraint $constraint) {
-      if ($defaults['entity_id'] != $values['entity_id']) {

So the reason this check didn't work well is because a new menu link is created *after* the validation step, so $values['entity_id'] is not populated at this time. The changes in #33 are perfect, thanks @plach!

I think collectively between @amateescu, @plach, and myself, we've reviewed and improved eachothers code, and now ready for RTBC.

RTBC +1

Thanks, the changes make sense and the added test coverage looks good. In the future, please open a follow up issue for issues that already have commits. Otherwise its hard to follow backlinks from commits, maintain credits, etc.

I cannot actually translate menu items because of this change. It always says "You can only change the menu settings for the published version of this content.".

@Rob230 - Please open a new issue for the issue you have along with steps to reproduce.
However, the MenuSettingsConstraint was introduced because menu links are not revisionable, meaning you can't have a different menu link for you published content, compared to your draft content. Maybe there is something we can do to assist the translation related issues, but we'll need a new issue opened, a better idea of the issue, and a test to reproduce the issue.

Take a look at \Drupal\menu_ui\Plugin\Validation\Constraint\MenuSettingsConstraintValidator, \Drupal\menu_ui\Plugin\Validation\Constraint\MenuSettingsConstraint, and \Drupal\Tests\menu_ui\Functional\MenuUiContentModerationTest to better understand the implementation.

I spent hours looking in those files. If I allowed it to be changed then it changed it on the original page instead of the translation, so that obviously isn't the right thing to do.

I finally worked out how to translate the menu items. You should not enable the option to translate menu link on node (in /admin/config/regional/content-language) because it is literally impossible to do so. If you have content moderation enabled then a new revision will always be created, and because it's a new revision, it does not allow you to edit the menu.

I found out the way to do it is to enable translation of "Custom menu link" and then go into Structure > Menu and translate the menu item there, It's not very intuitive for editors that they can configure the menu link when editing the node, but not when translating it, but it is at least possible.