Convert custom menu links to be revisionable

Looks like this just depends on one issue now, #2860654: Field storage CRUD operations must use the last installed entity type and field storage definitions, which is RTBC.
If we can get this ready maybe it can get in 8.4.0 too?

It looks as though the patch in #4 still applies, so queuing a test for it.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentResourceTestBase.php
@@ -106,6 +106,11 @@ protected function getExpectedNormalizedEntity() {
+      'revision_id' => [
+        [
+          'value' => 1,
+        ],
+      ],

@@ -172,6 +177,11 @@ protected function getExpectedNormalizedEntity() {
+      'revision_created' => [
+        $this->formatExpectedTimestampItemValues((int) $this->entity->getRevisionCreationTime()),
+      ],
+      'revision_user' => [],
+      'revision_log_message' => [],

See #2880154-11: Convert comments to be revisionable.

This is technically a BC break for API-First… I'll let core committers decide here.

I'll try to help out with #4 tonight.

Menu Link Content has a bug with current base field definitions.

#2981915: Update MenuLinkContent to use EntityPublishedInterface is in.

Would have belonged more in the other issue, but it's not something to solve for either, just mentioning as related for now.

The weird thing about this one, for historical reasons, is that it's called enabled and not status.

The problem is that content_translation additional field logic predates having a status/published entity key, and as a result, it hardcodes the field name to status, what it does is use that if it exists and if not, then it creates it own. So you have a then a translation status and an enabled key, and neither works as expected for menu links (It's not possible to have an unpublished menu link for a specific translation, as enabled is not translatble and nothing looks at translation_status when it matters). Afaik the menu system actually can't handle a status that varies per language at all.

So basically, if we'd decide to "fix" content_translation to look at the published key instead of hardcoding a field named "status", then things would break in interesting ways..

Here's the patch from #31 re-rolled on top of #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data (#29). The for-review part hasn't changed.

You meant #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data right? You referenced this issue.

Looks good +1 to RTBC. Here is the detailed summary:

Case 1:
1. Applied patch in #45 locally on an existing 8.7.x site.
2. drush updb shows the post_update and performs without any error.
3. Created new menu item (under footer menu) and works as usual.

Case 2:
1. Applied patch in #45 locally and installed a 8.7.x site.
2. Created/Updated/deleted a menu item (under footer menu) and works as usual.
3. Editing system provided default item works and no impact on the menu_link_data_* tables as expected.

+++ b/core/modules/menu_link_content/menu_link_content.module
@@ -29,6 +29,16 @@ function menu_link_content_help($route_name, RouteMatchInterface $route_match) {
+  //   for them.
+  //   @see https://www.drupal.org/project/drupal/issues/2350939

nitpick: too many spaces.

he extra spaces are there to point out that the @see tag is part of the @todo above. With the extra spaces, IDEs like PHPStorm will highlight the @see in the same color as the @todo, which is usually different than the color for regular comments. We also have other examples of this pattern in core if you search for // @see :)

Cool.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -531,6 +531,8 @@ protected function postUpdateEntityTypeSchema(EntityTypeInterface $entity_type,
   +      $this->storage->setTableMapping($new_table_mapping);
   +
   

   can you elaborate on why this change is needed?

2. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -127,19 +127,11 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   -    else {
   -      $this->messenger()->addError($this->t('There was an error saving the menu link.'));
   -      $form_state->setRebuild();
   -    }
   +    $menu_link->save();
   

   any reason why we dropped this?

3. +++ b/core/modules/menu_link_content/src/MenuLinkContentStorage.php
   @@ -0,0 +1,44 @@
   +    $inner_select = $this->database->select($this->getRevisionDataTable(), 't');
   +    $inner_select->condition("t.$rta_field", '1');
   +    $inner_select->fields('t', [$id_field, $langcode_field]);
   +    $inner_select->addExpression("MAX(t.$revision_field)", $revision_field);
   +    $inner_select
   +      ->groupBy("t.$id_field")
   +      ->groupBy("t.$langcode_field");
   +
   +    $query->join($inner_select, 'mr', "mlfr.$revision_field = mr.$revision_field AND mlfr.$langcode_field = mr.$langcode_field");
   +
   +    $query->groupBy("mlfr.$id_field");
   +
   +    return $query->execute()->fetchAllKeyed(1, 0);
   

   would be good to see the EXPLAIN output on this query

4. +++ b/core/modules/menu_link_content/src/Plugin/Validation/Constraint/MenuTreeHierarchyConstraintValidator.php
   @@ -0,0 +1,61 @@
   +      if (!$entity->parent->isEmpty() && !$original->parent->isEmpty() && !$entity->parent->equals($original->parent)) {
   +        $this->context->buildViolation($constraint->message)
   

   what if one has no parent, but the new one now does? shouldn't that trigger the message too? if so, missing tests for that too.

5. +++ b/core/modules/menu_link_content/tests/src/Functional/Update/MenuLinkContentUpdateTest.php
   @@ -59,6 +59,43 @@ public function testPublishedEntityKeyAddition() {
   +    $this->runUpdates();
   

   can we assert before running the updates the state we expect the test fixture to initialize the site in to guard against accidental updates to the fixture that contain the revision support.

6. +++ b/core/modules/menu_link_content/tests/src/Kernel/MenuLinkContentDeriverTest.php
   @@ -26,6 +26,7 @@ class MenuLinkContentDeriverTest extends KernelTestBase {
   +    $this->installEntitySchema('user');
   
   +++ b/core/modules/menu_link_content/tests/src/Kernel/PathAliasMenuLinkContentTest.php
   @@ -26,6 +26,7 @@ class PathAliasMenuLinkContentTest extends KernelTestBase {
   +    $this->installEntitySchema('user');
   

   should we use the new current user trait here ?

7. +++ b/core/modules/menu_ui/src/MenuForm.php
   @@ -267,6 +279,30 @@ protected function buildOverviewForm(array &$form, FormStateInterface $form_stat
   +    $pending_menu_link_ids = $this->menuLinkContentStorage->getMenuLinkIdsWithPendingRevisions();
   

   This menu form is for one menu, yet we prevent table drag if there are any menu links with pending revisions (ie. we don't filter this by menu).

   That feels wrong, and probably indicates missing test coverage?

   We definitely should be filtering to the menu being edited in my book.

   Otherwise you get the message, but have no idea which link is pending because its in a different menu.

8. +++ b/core/modules/menu_ui/src/MenuForm.php
   @@ -283,6 +319,12 @@ protected function buildOverviewForm(array &$form, FormStateInterface $form_stat
   +          $form['links'][$id]['#attributes']['class'][] = 'menu-link-content-pending-revision';
   

   should this use BEM --pending-revision instead of -pending-revision?

9. +++ /dev/null
   @@ -1,21 +0,0 @@
   -namespace Drupal\Tests\system\Functional\Update;
   -
   -/**
   - * Runs UpdatePathWithBrokenRoutingTest with a dump filled with content.
   - *
   - * @group Update
   - * @group legacy
   - */
   -class UpdatePathWithBrokenRoutingFilledTest extends UpdatePathWithBrokenRoutingTest {
   -
   -  /**
   -   * {@inheritdoc}
   -   */
   -  protected function setDatabaseDumpFiles() {
   -    parent::setDatabaseDumpFiles();
   -    $this->databaseDumpFiles[0] = __DIR__ . '/../../../../tests/fixtures/update/drupal-8.filled.standard.php.gz';
   -  }
   

   any reason for removing this?

10. +++ b/core/tests/Drupal/KernelTests/Core/Menu/MenuLinkTreeTest.php
    @@ -41,6 +41,7 @@ class MenuLinkTreeTest extends KernelTestBase {
    +    'user',
    
    @@ -49,6 +50,7 @@ class MenuLinkTreeTest extends KernelTestBase {
    +    $this->installEntitySchema('user');
    

    same here - can we use the new current user trait?

+++ b/core/modules/menu_ui/src/MenuForm.php
@@ -279,9 +280,19 @@ protected function buildOverviewForm(array &$form, FormStateInterface $form_stat
+    $edited_ids = array_filter(array_map(function ($element) {
+      return is_array($element) && isset($element['#item']) && $element['#item']->link instanceof MenuLinkContent ? $element['#item']->link->getMetaData()['entity_id'] : NULL;
+    }, $links));
+    $pending_menu_link_ids = array_intersect($this->menuLinkContentStorage->getMenuLinkIdsWithPendingRevisions(), $edited_ids);

we're still not filtering by menu here?

1. +++ b/core/modules/menu_link_content/src/Plugin/Validation/Constraint/MenuTreeHierarchyConstraintValidator.php
   @@ -0,0 +1,61 @@
   + * Constraint validator for changing term parents in pending revisions.
   ...
   +   * Creates a new TaxonomyTermHierarchyConstraintValidator instance.
   

   Liar :D

2. +++ b/core/modules/menu_link_content/tests/src/Kernel/MenuLinksTest.php
   @@ -310,4 +310,99 @@ public function testModuleUninstalledMenuLinks() {
   +    // Check that changing the parent or the weight in a pending revision is not
   +    // allowed.
   +    $child2_pending_revision = $storage->createRevision($child2, FALSE);
   +    $child2_pending_revision->set('parent', $child1->id());
   +    $child2_pending_revision->set('weight', 500);
   +    $violations = $child2_pending_revision->validate();
   +    $this->assertCount(2, $violations);
   

   Can we also assert that changing either of the two triggers the same validation error?

3. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -987,4 +987,38 @@ protected function doTestMenuBlock() {
   +    $child_1 = $this->addMenuLink($root_1->getPluginId(), '/', $menu_1->id());
   

   $child_1 seems unused.

.

This still needs a CR and release note snippets. Additionally the IS should mention the plan to make the hierarchy revisionable.

I manually tested this by manually restoring Content Moderation support (git hash 2c166a50a611b06d85c8d8ae482c6a76a196a4c6 + #59) and it doesn't seem to work properly in a few scenarios:

• I tried to save a root menu link as draft and I got the You can only change the hierarchy for the published version of this menu link. validation error even if I didn't touch the parent or weight fields.
• I then tried to save a child menu link as draft and it did work, however when I got back to the menu page, the "enabled" checkbox was still available. That should be disabled as well for drafts, because it saves a default revision overriding the draft. Additionally it triggers a notice: Notice: Undefined index: weight in Drupal\menu_ui\MenuForm->submitOverviewForm() (line 515 of core/modules/menu_ui/src/MenuForm.php).

I'm attaching the test DB dump in case it helps reproducing the issues.

The PG failure was caused by #3038892: Drupal\Tests\migrate_drupal\Kernel\d6\FieldDiscoveryTest randomly fails on PostgreSQL.

Thanks, I think this is ready to go now.

Saving credits.

Trying again.

Committed 5ad8f59 and pushed to 8.8.x and to 8.7.x as fbdccdc952c53fce12a81ac6640514c52e5fc3af.

Celebration time now!

Huge thanks to everyone involved in coding, reviewing, triaging, committing, promoting and cheering on! Such a great milestone!