Exception when adding tab to a node managed by content moderation

What is it that convinces content moderation to believe that it *ought* to be trying to moderate a non-entity (string) parameter in the first place? I would expect content moderation to not even be invoked, if the parameter in question weren't an entity.

I'm wondering if we need a check in Content Moderation EntityTab class that $this->entity is actually an entity.

I've hit this exact issue with creating a tab which was a view. In that case you can't hint Node on a controller because views has it's own controller and argument processing. The solution in the end was a route subscriber which altered the route definition like so:

  protected function alterRoutes(RouteCollection $collection) {
    $route = $collection->get($views_route_id);
    $options = $route->getOptions();
    $options['parameters']['node']['type'] = 'entity:node';
    $route->setOptions($options);
  }

A check would be good, because even if the label becomes inconsistent on some routes created by the user, it doesn't fatal. Then some good docs around making this work when the tabs are extended should be enough.

pretty sure the issue here is the access controller runs before the paramconverter has upcast the ID to an entity.

I think as a quick fix for this we should just update \Drupal\content_moderation\Plugin\Menu\EditTab to check $this->entity is an instance of EntityInterface.

I have previously removed for Drupal\content_moderation\ModerationInformation this

 if (!$entity instanceof ContentEntityInterface) {
      return FALSE;
    }

but it's necessary for other calls.

1. +++ b/core/modules/content_moderation/src/Plugin/Menu/EditTab.php
   @@ -78,9 +78,11 @@ public function getRouteParameters(RouteMatchInterface $route_match) {
      public function getTitle() {
   

   Classes need to be imported at the top of the file.

2. +++ b/core/modules/content_moderation/src/Plugin/Menu/EditTab.php
   @@ -78,9 +78,11 @@ public function getRouteParameters(RouteMatchInterface $route_match) {
   +    if ($this->entity instanceof \Drupal\Core\Entity\EntityInterface) {
   +      if (!$this->moderationInfo->isModeratedEntity($this->entity)) {
   +        // Moderation isn't enabled.
   +        return parent::getTitle();
   +      }
   

   If this check fails and the entity is null it still falls through the the following lines:

   return $this->moderationInfo->isLiveRevision($this->entity)

   ..which reading the code, will still fatal.

Perhaps a test for this is needed after all.

+++ b/core/modules/content_moderation/src/Plugin/Menu/EditTab.php
@@ -78,15 +80,19 @@ public function getRouteParameters(RouteMatchInterface $route_match) {
+    if ($this->entity instanceof EntityInterface) {
+      if (!$this->moderationInfo->isModeratedEntity($this->entity)) {

Could this be merged into a single if?

Also, if the entity can't be determined we probably should return something generic like "Edit", not null.

Also NW based on tests.

Here is a test and fix for this which I think is correct. Originally it looked like we could just load the entity ourselves if it wasn't upcast, we have access to the ID and the entity_type_id, but a few things made me think this is possibly wrong:

• Possible route access control issue?
• The assumption that a non-upcast parameter named after an entity type ID is always going to load a valid entity to use.

Overall I think it's safest just to ignore non ContentEntityInterface params.

Sorry, didn't start from the original patch, hence no interdiff.

Looks like a nice solution to me!

Committed/pushed to 8.3.x and 8.2.x, thanks!

The test introduced by this issue is failing in HEAD:
https://www.drupal.org/pift-ci-job/553399

The patch in #22 is still fine for 8.2.x, but here's a patch for 8.3.x

What is the "lo" for??

No idea! Where'd that come from? Re-rolled without it.

Interdiff LGTM

Putting back to RTBC

Fails look unrelated, running again.

Should have been RTBC.

#38 is green again against 8.3.x so I think this is ready.

I don't think this code will last but it is good to have it fixed just in case we don't do #2843083: Select entity type / bundle in workflow settings.

Committed 0391e31 and pushed to 8.2.x. Thanks!

diff --git a/core/modules/content_moderation/src/Plugin/Menu/EditTab.php b/core/modules/content_moderation/src/Plugin/Menu/EditTab.php
index 6c30934..3a80879 100644
--- a/core/modules/content_moderation/src/Plugin/Menu/EditTab.php
+++ b/core/modules/content_moderation/src/Plugin/Menu/EditTab.php
@@ -3,7 +3,6 @@
 namespace Drupal\content_moderation\Plugin\Menu;
 
 use Drupal\Core\Entity\ContentEntityInterface;
-use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Menu\LocalTaskDefault;
 use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
 use Drupal\Core\Routing\RouteMatchInterface;

Fixed unused use on commit.

This isn't for the field API tabs, but the tabs you see when you are looking at a full node page, so I think this will actually stick around? Since #38 this applies, passes and is needed for 8.3.x as well.

Ah that explains it - I was surprised when this got committed to 8.2.x - I thought I had committed it there #51 - I assumed I'd messed up by committing it to both branches. Committed a76eef7 and pushed to 8.3.x. Thanks!

diff --git a/core/modules/content_moderation/src/Plugin/Menu/EditTab.php b/core/modules/content_moderation/src/Plugin/Menu/EditTab.php
index 6c30934..3a80879 100644
--- a/core/modules/content_moderation/src/Plugin/Menu/EditTab.php
+++ b/core/modules/content_moderation/src/Plugin/Menu/EditTab.php
@@ -3,7 +3,6 @@
 namespace Drupal\content_moderation\Plugin\Menu;
 
 use Drupal\Core\Entity\ContentEntityInterface;
-use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Menu\LocalTaskDefault;
 use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
 use Drupal\Core\Routing\RouteMatchInterface;

Unused use fixed on commit.