Problem/Motivation

MenuSettingsConstraintValidator was introduced to prevent changes to menu items in node forms "leaking" into the published version of the website in draft pending revisions of entities, primarily created via content moderation.

While it's not the expectation that content moderation will control drafts for all entities referenced or included as part of the main entities form (this is the role of workspaces), the associated menu link is special because:

  • The fields are presented inline, with all other node content.
  • There is a 1 to 1 composite relationship between a menu link and the associated node, unlike common "library" entities like media and terms which share a 1 to many relationship distributed amongst possibly menu different entities.
  • Users particularly expect this content to be part of the draft, or the constraint would not have been introduced.

The UX for the current approach is also pretty sub-optimal for a few reasons:

  • Changes to the menu link title can't be drafted, having this feature would be quite neat.
  • If an editor changes some menu link text, along with a large amount of other content changes, their whole form is essentially broken with no path forward. Unless they remember the original values and restore them manually, the work is lost.

This issue was brought to my attention based on point 2 above, so I think it dips into the realm of a major issue, since it concerns work completed by an editor that is potentially lost.

Proposed resolution

Now that menu links are revisionable, make them draftable alongside the entity the user is drafting.

Remaining tasks

Discuss, validate and agree on implementation.

User interface changes

An error message on the node edit form will now be absent when changes are made to menu links in a draft.

API changes

None.

Data model changes

Pending revisions will be created for MenuLinkContent entities, when their host entity revision is also pending.

Release notes snippet

CommentFileSizeAuthor
#11 3041326-11.patch10.83 KBSam152
#7 3041326-7.patch973 bytesSam152
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sam152 created an issue. See original summary.

Sam152 credited amateescu.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext for Brisbane City Council commented

Sam152 credited Berdir.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext for Brisbane City Council commented
Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext for Brisbane City Council commented
Issue summary: View changes
Related issues: +#3041327: Investigate preventing users from triggering MenuSettingsConstraintValidator by toggling access to the menu settings when targeting a state that creates a non-default revision
Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext for Brisbane City Council commented
Status: Active » Needs review
FileSize
3041326-7.patch973 bytes

This was all that needed to be changed to provide a first pass at this. With this implementation, any changes made to the associated menu link in the draft content will only appear in the entity form until a published state is reached.

Seeing what tests are impacted by this change, but I'm pretty optimistic about this approach.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext for Brisbane City Council commented
Issue summary: View changes
Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext for Brisbane City Council commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 7: 3041326-7.patch, failed testing. View results

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext for Brisbane City Council commented
Title: Investigate removing MenuSettingsConstraintValidator when used with content moderation by creating a draft of menu link content when a draft of it's parent content is created » Remove 'title' and 'description' from MenuSettingsConstraintValidator when used with content moderation by creating a draft of menu link content when a draft of it's parent content is created
Status: Needs work » Needs review
FileSize
3041326-11.patch10.83 KB

Here is an approach that makes 'title' and 'description' work and also allows new items to be created inside a draft (and not leak). Here are the ops that are currently still prevented:

  • Removing a menu item, since that results in a delete.
  • Updating the parent or weight, since those aren't revisionable.

If those were ever revisionable in the future, I believe they would also work in CM drafts. CM doesn't actually have the same constraints as workspaces with regards to making the entire menu tree revisionable, for previewing the entire scope of changes in a new workspace, so I wonder if these could have actually been made revisionable with MenuTreeHierarchyConstraintValidator being moved into workspaces, since only workspaces have the requirement of the whole menu tree being revisionable/previewable.

I do think that discussion is out of scope in this issue though and the changes here are still a nice incremental improvement over the status quo.

Current tests ensure:

  • The existing properties are still covered by the validator.
  • Added menu items don't leak into the live site.
  • The changed title doesn't leak into the live site.

The diff is a bit hard to read since I needed to reorder some asserts in the test, but should hopefully make sense when applied.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Component: content_moderation.module » menu_ui.module
Category: Feature request » Bug report
Status: Needs review » Reviewed & tested by the community
Issue tags: +Workflow Initiative

CM doesn't actually have the same constraints as workspaces with regards to making the entire menu tree revisionable

It does have that constraint though, because we still need to prevent re-parenting in the menu overview form and also from API calls :)

This change allows the menu item title and description to be edited in non-default workspaces from the node form, so I'm reclassifying it as a bug report.

The patch form #11 looks great to me!

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext for Brisbane City Council commented

Drafted a CR: https://www.drupal.org/node/3047051

  • larowlan committed bc73813 on 8.8.x 
    Issue #3041326 by Sam152, amateescu, Berdir: Remove 'title' and '...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed bc73813 and pushed to 8.8.x. Thanks!

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Version: 8.8.x-dev » 8.7.x-dev
Status: Fixed » Reviewed & tested by the community

It would be good to have this in 8.7.0 as well, as it will provide a good user experience with Workspaces (paired with #3040258: Menu link content changes are not visible on non-live workspaces).

  • larowlan committed 111fb75 on 8.7.x 
    Issue #3041326 by Sam152, amateescu, Berdir: Remove 'title' and '...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

C/p to 8.7.x as 111fb75870

Updated change record

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at ezCompany commented

Updated the CR to reflect this will be available in 8.7.3, not 8.7.0

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.