Disallow saving the current default revision as a non-default revision

Created a merge request, this should fail.

Fixed a few bugs in the tests, now there's a sensible amount of fails.

don't really understand what Drupal.KernelTests.Core.Entity.RevisionableContentEntityBaseTest is doing, but I don't think what it does is sensible.

\Drupal\KernelTests\Core\Entity\EntityDuplicateTest::testDuplicateNonDefaultRevision() is 🐛 ContentEntityBase::createDuplicate() should reset default revision flag Needs work , forgot about that, that was RTBC but then somehow it got lost once it was set to needs work. Probably a blocker for this.

\Drupal\Tests\content_moderation\Kernel\ContentModerationSyncingTest::testUpdatingPreviousRevisionDuringSync() is tricky, it's resaving an old revision that was default but no longer is. I'll need to double check if we have a way to detect the difference between current default revision and previous default revision. The migrate tests might be similar, not sure yet.

\Drupal\Tests\menu_ui\Functional\MenuUiContentModerationTest::testMenuUiWithPendingRevisions is the fail I expected. I have one idea that we can try, but it will complicate things a bit and it depends on content_moderation specifically. The idea is that we'd create that menu link to the latest route, where access is denied if you don't have access to preview the draft, and change it back. But it complicates things, we always need to look for that route, or at least if it's a non-default-revision node being edited and update the target on publish.

I have one idea that we can try, but it will complicate things a bit and it depends on content_moderation specifically.

If this idea works and it's not super hard to implement, I think it would be worth doing. It would be temporary anyway, until 📌 Allow for / implement simplified content workflow with workspaces Active is finished and Content Moderation will be able to properly track an entity and its dependencies.

Maybe postponed is more appropriate, but with known-failing tests, this is NW. Super minor nit as an MR suggestion.

This stuff really has rabbit holes inside rabbit holes.

The migrate EntityRevision destination plugin calls setDefaultRevision(FALSE) since 2014, i honestly don't know why. It loads an existing revision or the default revision, neither should have a reason to change whether or not it's currently a default revision, it should just update that revision. Lets see if removing that breaks any tests.

For the menu_ui stuff, the changes that only partially worked inside 📌 MenuTreeStorage shouldn't invalidate cache tags if the menu didn't change Active actually passed then here. The reason for that is again the partial and incorrect fix that was done in #2850022: Duplicating a non-default revision should produce a default revision for a newly created entity → , which allowed the partial and incorrect implementation in #3041326: 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 → . What we did is accept the changed value for isDefaultRevision() but then the return value ignored the current value and returned always TRUE if the entity is *currently* new. That means that it correctly saves the change as the default revision but then in post save, in \Drupal\menu_link_content\Entity\MenuLinkContent::postSave(), it now sees that it's actually supposed to be not a default revision, so it doesn't update the menu tree storage. Meaning, the menu link content entity exists as a regular enabled default revision, but it's just not in the menu tree. Saving it through any other means than the node form on a draft would then however resave it properly and it would become visible.

What I did there now is as suggested above, point those menu links to the /latest page, then they show up but only for users who are allowed to see the latest version. It's either that or not allowing to create new menu links on a draft. And I changed isDefaultRevision() to ignore a new value on a new entity, so that it doesn't change in the middle of saving.

I also removed the first check about new and not default revision, because both old and new implementation of isDefautRevision() do not allow for that to happen.

I see lots of new fails though, so I'll need to investigate where that comes from.

This should come back green now and is ready for feedback/reviews. Comes with extensive explanations on the MR about the changes. Could likely do with some adjusted inline comments, open for suggestions.

@berdir you opened several threads. Looking for feedback on those or just general comments?

The threads are mostly just explanations on the changes because several of them were very complex to figure out and resolve. I'm not sure how extensive the inline comments should be. I'm both looking for general reviews and feedback and suggestions on if and how code documentation should be extended/adjusted.

Ok I took a look through this. I had some high level suggestions to try to make it a bit more readable.

I honestly had more questions than answers, sorry for that. I'll keep an eye on this and try to follow up on the threads.

It seems to me the underlying issue is it is saving it as a non default revision, but it's not creating a new revision, which means you have split data.

A little more detail in steps to reproduce on what to look for might be helpful, e.g. should we set a field and compare two tables?

To manually reproduce problems with this, try these scenarios, always make sure that content moderation is enabled.

A)
1. Create a new published node with a menu link, name both Foo (or whatever, just as an example to reference later).
2. Create a draft of this node, also change the menu link title to Foo Draft. Note how the visible menu title doesn't change, like you'd expect as it's just a draft.
3. Check the menu_link_content_data and menu_link_content_field_revision tables. note how you actually only have one revision and no draft like for the node, and the data table has Foo while the field_revision table has Foo Draft.
4. Edit the menu, note how the title in the overview is Foo (persisted value in the menu tree table, but editing shows Foo Draft (the value you get from loading the menu_link_content entity. Save that menu link.
5. Note how it's now consistently Foo Draft.

B)
1. Create a new published node Bar, no menu link.
2. Create a draft of that node, add a menu link, name both Bar Draft.
3. No menu link shows up, kind of as expected. Nothing in the menu either.
4. Check the tables again, note how the entity actually exists as the default revision with Bar Draft.

c)
Mostly same as A, but with translations, for example on umami. Things get even weirder then, sometimes it's Foo, sometimes Foo Draft, also for anonymous users, so it's very much not a draft. This is because on multlingual sites, \Drupal\menu_link_content\Plugin\Menu\MenuLinkContent actually loads the menu_link_content entities and displays the title for the current language.

All these things happen due to the combination of problems, outlined in #7. For a new entity, new revision on HEAD is forced to TRUE, but it can switch in postSave(), which results in the menu link not being saved into the menu_tree table. For existing entities, it results in only updated the revision tables, resulting in inconsistent data (loading loads from revision tables, but for example entity query would then find the old value only, and you can't revert to the old revision).

With the changes here, things will be more consistent, although not perfect I'd assume. For B, you should see a menu link then, to the latest page, but only as a user with access to that page. For existing menu links, saving a draft should actually save it as a draft and never show, translations or not. However, I'd expect the menu edit experience might be strange, haven't actually tested that yet. menu_link_content entities don't actually support content_moderation, you can't officially manage drafts there, so you might see the default revision then on edit and saving that might result in losing access to the draft. The node edit form should consistently show the draft though and on publish make it the default revision. I think improving menu edit should be it's own issue, should maybe actually disallow edit in that scenario as there's no UI to really deal with that situation.

Thank you for that write up! I ran through A and B on both 11.x and this branch and confirmed the expected behavior on both the front end and the db level.

There is one remaining issue which is out of scope here, and exists on 11.x too.
If you publish a node with a draft menu item using the content moderation form instead of with a node save the menu item isn't published.

There are two action items on the MR still, but once those are done I think this is ready.

All comments have been addressed, I've also manually tested.

Test only fails as expected.

I think this one is good to go!

I've been following and reviewing this MR for a while, and even though the workaround for pending revisions of menu links is a bit awkward, I can't see any other way around it.

Suggested a small improvement for the exception message, otherwise +1 for RTBC.

One comment on the MR.

Addressed the reviewed.

Looks ready to me :)

The menu link hack looks out of place with the rest of the issue. Is it possible to split that out? I asked @amateescu about it in slack and he said it's because content_moderation does horrible things to menu items. However, the behaviour will (or at least might) affect other forward revision modules like workspaces that don't need it.

If it's really, really necessary and can't be disentangled, I think we need an explicit @todo to remove it again.

Opened a followup for #19 and added a @todo pointing to it :)

OK I still don't like the menu link hack but @amateescu pointed out it's replacing/fixing workarounds already in content_moderation module, and we can remove it once content moderation is built on top of workspaces. Would have been better if content_moderation didn't try to mess with things it shouldn't in the first place, but on the basis it makes things less broken, committed/pushed to 11.x, thanks!

catch → closed merge request !10880

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