Problem/Motivation

Steps to reproduce

  1. Enable content_moderation
  2. Allow articles to be added to menus.
  3. Create an article, don't add a menu item.
  4. Create a new draft revision of the node, this time create a menu item.
  5. After saving, note the node immediately appears in the menu.

Proposed resolution

Either:

1. Don't allow nodes to be added/changed/removed in menus when saving a non-default revision

2. Change the node/menu integration to somehow be revision-aware

Remaining tasks

User interface changes

Comments

catch created an issue. See original summary.

dawehner’s picture

catch’s picture

jhodgdon’s picture

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?

cilefen’s picture

Issue tags: +Triaged D8 critical

@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.

amateescu’s picture

@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: Introduce the concept of workspaces.

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: Followup: 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.

The last submitted patch, 6: 2858434-6-test-only.patch, failed testing. View results

vijaycs85’s picture

Issue summary: View changes
FileSize
98.54 KB

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).

timmillwood’s picture

Issue summary: View changes
+++ 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)

timmillwood’s picture

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).

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs review
diff --git a/core/modules/menu_ui/menu_ui.module b/core/modules/menu_ui/menu_ui.module
index cffccde..53bd456 100644
--- a/core/modules/menu_ui/menu_ui.module
+++ b/core/modules/menu_ui/menu_ui.module
@@ -368,6 +368,26 @@ function menu_ui_form_node_form_submit($form, FormStateInterface $form_state) {
   $node = $form_state->getFormObject()->getEntity();
   if (!$form_state->isValueEmpty('menu')) {
     $values = $form_state->getValue('menu');
+
+    // Prevent changes to the menu settings if the node being saved is not the
+    // default revision.

Is there a reason this can't be done in validation vs. submit?

timmillwood’s picture

plach’s picture

Issue tags: +WI critical

This is marked as MUST-HAVE in the roadmap.

dawehner’s picture

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 .

plach’s picture

Status: Needs review » Needs work
timmillwood’s picture

Status: Needs work » Postponed
plach’s picture

Status: Postponed » Needs work

That's been committed

amateescu’s picture

Status: Needs work » Needs review
FileSize
3.7 KB
8.08 KB
5.76 KB

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

plach’s picture

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’s picture

Status: Needs review » Needs work
amateescu’s picture

Status: Needs work » Needs review
FileSize
9.29 KB
4.89 KB

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

plach’s picture

Status: Needs review » Reviewed & tested by the community

Nice!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 7755d43 and pushed to 8.4.x. Thanks!

  • catch committed 7755d43 on 8.4.x
    Issue #2858434 by amateescu, vijaycs85, plach, timmillwood, catch: Menu...
timmillwood’s picture

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.

timmillwood’s picture

Status: Fixed » Needs review
FileSize
1.14 KB

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.

timmillwood’s picture

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

The last submitted patch, 28: 2858434-28-test-only.patch, failed testing. View results

amateescu’s picture

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.

timmillwood’s picture

plach’s picture

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.

amateescu’s picture

+++ 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!

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

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

plach’s picture

RTBC +1

  • Gábor Hojtsy committed 249e525 on 8.4.x
    Issue #2858434 by amateescu, timmillwood, plach: (Followup to) menu...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

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.