View-latest tab rendering wrong

1. +++ b/workbench_moderation.module
   @@ -90,3 +90,29 @@ function workbench_moderation_local_tasks_alter(&$local_tasks) {
   +  $variables['page'] = $variables['page'] || workbench_moderation_is_latest_page($variables['node']);
   

   Nice. :-)

2. +++ b/workbench_moderation.module
   @@ -90,3 +90,29 @@ function workbench_moderation_local_tasks_alter(&$local_tasks) {
   +function workbench_moderation_is_latest_page(NodeInterface $node) {
   +  $route_match = \Drupal::routeMatch();
   +  if ($route_match->getRouteName() == 'entity.node.latest_version') {
   +    $page_node = $route_match->getParameter('node');
   +  }
   +  return (!empty($page_node) ? $page_node->id() == $node->id() : FALSE);
   +}
   

   Does this make sense to move to a method of a service? I'd prefer to have no non-trivial code outside of a service if possible.

I don't believe so. Seems too one-off to be put into one of the handlers. The question is whether it's worth the effort to move that to a new service class for the experience. :-)

1. +++ b/src/IsLatestPage.php
   @@ -0,0 +1,47 @@
   +class IsLatestPage {
   

   I wouldn't make the class this specific. Rather, what is appropriate to live here? What else might we add?

   My inclination would be to rename this to WorkbenchPreprocess, and then move the entire preprocess hook into a preprocessNode() method, which in turn can just call isLatestPage() as now.

   A class that's name is the same as its one method is generally a sign that the class is too specific.

2. +++ b/src/IsLatestPage.php
   @@ -0,0 +1,47 @@
   +  public function isLatestPage(Node $node) {
   

   Better name: isLatestVersionPage(). (isLatestPage to me implies "is this the latest page of something else", like a most-recently-updated view or something.)

3. +++ b/src/IsLatestPage.php
   @@ -0,0 +1,47 @@
   +    if ($this->routeMatch->getRouteName() == 'entity.node.latest_version') {
   +      $pageNode = $this->routeMatch->getParameter('node');
   +    }
   +    return (!empty($pageNode) ? $pageNode->id() == $node->id() : FALSE);
   

   The need for !empty() suggests this could be restructured. Actually, I'm not clear on why we need to compare the IDs. When would those not match?

   Assuming we still need that comparison, then we can take advantage of boolean short-circuiting, as elsewhere in the module, and the fact that = evaluates to the value assigned:

   return 
     $this->routeMatch->getRouteName() == '...'
     && $page_node = ...
     && $page_node->id() == $node->id();
   

1. +++ b/src/WorkbenchPreprocess.php
   @@ -0,0 +1,58 @@
   +class WorkbenchPreprocess {
   

   As a service, this class is now testable. Yay! It involves an entity and routes, so probably a KernelTest, but it's definitely doable. Let's.

2. +++ b/src/WorkbenchPreprocess.php
   @@ -0,0 +1,58 @@
   +   * @todo I don't like passing $variables by reference here, but I did it
   +   *       because it matches the way hook_preprocess_HOOK() works. Is this OK?
   

   I don't like it either, but that's what we've done elsewhere in the module. Everything else is doing proper returns, but this method is really just the hook itself copied into a class keyword to mirroring the hook's API makes the most sense.

3. +++ b/src/WorkbenchPreprocess.php
   @@ -0,0 +1,58 @@
   +  public function isLatestPage(\Drupal\node\Entity\Node $node) {
   

   "use" the class name at the top of the file, then the short-name "Node" here.

Hm, yeah getting the route match in the hook method and passing that to isLatestPage() is even better, and makes testing easier, too.

This is a node-specific bug due to the way nodes get themed. That's why it's a node-specific solution.

I agree with Bec. This is node-specific code to fix a node-specific bug, using a node-specific core oddity because nodes are still special, even in D8. :-(

Merged. Thanks, Bec!