Upcast node and node_revision parameters of node revision routes

This is indeed an issue, after some debugging I came to the same resolution: When on the revision confirmation page, the node object is not loaded, only the node id.

Not sure if this is by design, if it is the case, that the issue is on the side of ctools.

I did a little bit of debugging and noticed that at the following location, when there is no {node_revision} they get replaced by the object correct. But when it has the {node_revision} both are left as id only.
\Symfony\Cmf\Component\Routing\DynamicRouter::applyRouteEnhancers

Update:

I was able to trace it down to:
\Drupal\Core\ParamConverter\ParamConverterManager::convert

    if (!$parameters = $route->getOption('parameters')) {
      return $defaults;
    }

Seems that at this stage: \Drupal\Core\Routing\RouteProvider::getRoutesByPath
The serialized value does not contain any parameters:
C:31:"Symfony\Component\Routing\Route":1582:{a:9:{s:4:"path";s:45:"/node/{node}/revisions/{node_revision}/revert";s:4:"host";s:0:"";s:8:"defaults";a:2:{s:5:"_form";s:40:"\Drupal\node\Form\NodeRevisionRevertForm";s:6:"_title";s:26:"Revert to earlier revision";}s:12:"requirements";a:3:{s:21:"_access_node_revision";s:6:"update";s:4:"node";s:3:"\d+";s:7:"_method";s:8:"GET|POST";}s:7:"options";a:6:{s:14:"compiler_class";s:34:"\Drupal\Core\Routing\RouteCompiler";s:21:"_node_operation_route";b:1;s:12:"_admin_route";b:1;s:14:"_route_filters";a:1:{i:0;s:27:"content_type_header_matcher";}s:16:"_route_enhancers";a:2:{i:0;s:31:"route_enhancer.param_conversion";i:1;s:19:"route_enhancer.form";}s:14:"_access_checks";a:1:{i:0;s:26:"access_check.node.revision";}}s:7:"schemes";a:0:{}s:7:"methods";a:2:{i:0;s:3:"GET";i:1;s:4:"POST";}s:9:"condition";s:0:"";s:8:"compiled";C:33:"Drupal\Core\Routing\CompiledRoute":716:{a:11:{s:4:"vars";a:2:{i:0;s:4:"node";i:1;s:13:"node_revision";}s:11:"path_prefix";s:5:"/node";s:10:"path_regex";s:67:"#^/node/(?P<node>\d+)/revisions/(?P<node_revision>[^/]++)/revert$#s";s:11:"path_tokens";a:5:{i:0;a:2:{i:0;s:4:"text";i:1;s:7:"/revert";}i:1;a:4:{i:0;s:8:"variable";i:1;s:1:"/";i:2;s:6:"[^/]++";i:3;s:13:"node_revision";}i:2;a:2:{i:0;s:4:"text";i:1;s:10:"/revisions";}i:3;a:4:{i:0;s:8:"variable";i:1;s:1:"/";i:2;s:3:"\d+";i:3;s:4:"node";}i:4;a:2:{i:0;s:4:"text";i:1;s:5:"/node";}}s:9:"path_vars";a:2:{i:0;s:4:"node";i:1;s:13:"node_revision";}s:10:"host_regex";N;s:11:"host_tokens";a:0:{}s:9:"host_vars";a:0:{}s:3:"fit";i:21;s:14:"patternOutline";s:26:"/node/%/revisions/%/revert";s:8:"numParts";i:5;}}}}

So the issue is when the routes are stored in the database, could someone tell me where this happen? Even better, where the parameters are set?

A first try to see the implications.

I know that reviewers will argue that this patch is a BC break because some contrib code might expect a scalar when querying with \Drupal::service('current_route_match')->getParameter('node'). But I'm keeping my point of view: Their code is wrong - they should always use \Drupal::service('current_route_match')->getRawParameter('node') to get the node id.

This has potential small BC problems, as at least it should be 8.2.x only.

I'm wondering about the alternative approach: Typehint NodeInterface on those controllers. Would that be feasible as well?

@dawehner, In the case of CTools it just uses the context:

From \Drupal\ctools\Plugin\Condition\EntityBundle:

  public function evaluate() {
    if (empty($this->configuration['bundles']) && !$this->isNegated()) {
      return TRUE;
    }
    /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
    $entity = $this->getContextValue($this->bundleOf->id());
    return !empty($this->configuration['bundles'][$entity->bundle()]);
  }

So the param is not passed in the method signature

I see, so all beside the first one actually never have the node passed into any controller, because its not needed.

--- a/core/modules/node/node.routing.yml
+++ b/core/modules/node/node.routing.yml

+++ b/core/modules/node/node.routing.yml
@@ -50,6 +50,9 @@ entity.node.version_history:

@@ -50,6 +50,9 @@ entity.node.version_history:
     node: \d+
   options:
     _node_operation_route: TRUE
+    parameters:
+      node:
+        type: 'entity:node'
 

I think that one is optional. If you look into this example you'll see its already upcasted: \Drupal\node\Controller\NodeController::revisionOverview

On top of that I'm wondering whether it would make sense to somehow write tests for it.

@dawehner, I added the test (also a test-only patch) and removed that route.

The failure was from the test only patch. Tests are passing

I ran into this issue as well today, same findings as @harings_rob. In my case, I'm altering contextual links for custom entities being rendered on a node view page. In that method, I'm using the current_route_match service to get the 'node' object, and I found that on the node view route, I get the full node object, and on the node revision route, I only get the scalar. Would be nice if these were uniform. As a workaround, I'll do like @claudiu.cristea suggests and use getRawParameter('node') and upcast it myself.

As I chased this rabbit hole, I got confused by how and where the entitiy.node.canonical route's node parameter gets upcast. Seems to me the route is declared here: http://cgit.drupalcode.org/drupal/tree/core/modules/node/src/Entity/Node..., but the node parameter isn't declared as type entity:node here, so how does Drupal know to do that param conversion? It's somewhat off topic I realize, but would someone be so kind as to enlighten me?

@m4olivei, The green patch from #13 is fixing the bug. However, if you want to fix the bug without applying the patch to core, in your module (pretend that is named custom_module), you'll need to implement the following workaround:

In custom_module.services.yml, under services: add:

  custom_module.route_subscriber:
    class: Drupal\ custom_module\Routing\RouteSubscriber
    arguments: ['@module_handler']
    tags:
      - { name: event_subscriber }

The add a new file:

namespace Drupal\custom_module\Routing;

use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\Core\Routing\RouteSubscriberBase;
use Symfony\Component\Routing\RouteCollection;

/**
 * Listens to the dynamic route events.
 */
class RouteSubscriber extends RouteSubscriberBase {

  /**
   * The module handler service.
   *
   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   */
  protected $moduleHandler;

  /**
   * Constructs a route subscriber.
   *
   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   *   The module handler service.
   */
  public function __construct(ModuleHandlerInterface $module_handler) {
    $this->moduleHandler = $module_handler;
  }

  /**
   * {@inheritdoc}
   */
  public function alterRoutes(RouteCollection $collection) {
    // Node revision routes parameter {node} is not converted into its entity.
    // @see https://www.drupal.org/node/2730631
    // @todo Remove this alter once https://www.drupal.org/node/2730631 is in.
    if ($this->moduleHandler->moduleExists('node')) {
      $node_revision_routes = [
        'entity.node.version_history',
        'entity.node.revision',
        'node.revision_revert_confirm',
        'node.revision_revert_translation_confirm',
        'node.revision_delete_confirm',
      ];
      foreach ($node_revision_routes as $route) {
        $parameters = $collection->get($route)->getOption('parameters');
        $parameters = $parameters ?: [];
        $parameters['node']['type'] = 'entity:node';
        $collection->get($route)->setOption('parameters', $parameters);
      }
    }
  }

}

Thanks for pointing that out @claudiu.cristea, I learned something!

added link to related issue #2495703: Node route parameter is inconsistent, which also has a patch for review. This issue was created after that one, so maybe this is a duplicate?

I will go ahead and just mark the other issue as a duplicate for the following reasons:

• both issues refer to same bug;
• both issues try to address the bug in exactly the same manner;
• patch in this issue has a larger scope and fixes more affected routes;
• patch in this issue has tests;
• this issue is simply longer and provides more context.

I am currently using this patch on Drupal 8.3.6 and it seems to work great!

Thanks!

Is it possible the same issue is occurring on the entity.node.preview route?

Adding a possible related issues > https://www.drupal.org/node/2890758 and https://www.drupal.org/node/2885278

Experiencing the same issue in #2915528: Avoid error on revision page node or term page.. I added the workaround suggested in #18.
Totally agree with #7. \Drupal::service('current_route_match')->getParameter('node') should always return the $node object.

A change record could be sufficient for the BC break, and advance this issue ?

I am trying to review this patch. It seems simple enough, but there are a few things that surprise and/or confuse me. Clearly you guys know more about the routing system than I do.

Parameter upcasting

It seems that the parameter is "upcasted" (as @dawehner noted in #11) automatically if the following conditions are met:

1. The parameter is named after an entity ("node" or "user", for example).
2. The default controller gives the parameter a suitable type hint.

When these conditions are met, the Entity object is automatically provided to the controller and to \Drupal::routeMatch()->getParameter(). The object is also available to the access callback, although a type hint there does not trigger automatic upcasting.

(Related questions: the controller or form is specified under the defaults key. Is there a way to override the default controller? If so, then what happens if the default controller gives a type hint and the override does not, or vice versa?)

I guess that is the way it works, although it seems confusing to me and not clearly documented at Structure of routes nor Using parameters in routes. Not only is it confusing, but what if I use the parameter "foo" and then later a module gets added to the site that creates the "foo" entity type? I guess in that case, I did not use a type hint, so nothing breaks, maybe.

In all of the routes that this patch modifies, the controller (or form) does not provide a type hint for the "node" parameter, because the controller does not even use that parameter. Question: If we declare type: entity:node in node.routing.yml, will Drupal load the node object when it is not used?

Update: Looking at @dawehner's comments above, it seems that I am repeating some things that were already said on this issue, but at least I seem to have it right. The behavior I describe as "confusing" was debated at length and added in #1906810: Require type hints for automatic entity upcasting. I do not see a change record for that issue.

Requested changes

1. According to Parameter upcasting in routes,

Note: entity:entity_type must not be surrounded by quotation marks.

The patch for this issue proves that "must" is too strong, but still I think you should remove the quotation marks to be consistent with other routing files.

2. CodeSniffer complains that this array is too long:

  public static $modules = ['node_routes_test', 'language', 'content_translation', 'block'];

Please break it into multiple lines.

3. Aren't tests the place where dependency injection matters the most? I think you should inject the service container instead of

    $route_match = \Drupal::routeMatch();

I suppose the other approach is to move in the other direction: rename the parameter from {node} to {nid} for the routes that do not need a Node object. This would remove the expectation that a Node object is available.

I am not recommending that approach. I just want to consider the alternatives. That would be more of a BC problem.

While testing this, I noticed the following behavior on the path /node/{node}/revisions/{node_revision}/view:

1. {node_revision} is a revision of {node}: as expected
2. {node_revision} is a revision of some other node, but node/{node} exists: looks the same, except that "Revisions" (links to node/{node}/revisions in the first case) is missing
3. node/{node} does not exist: Page not found (404)

So I guess the code validates that {node} is a valid node ID, but not that it corresponds to the {node_revision}. Is this worth a follow-up?

BTW, in case anyone else was wondering: the option _node_operation_route: TRUE means that we should use the admin theme on this route if the option "Use the administration theme when editing or creating content" on /admin/appearance has been selected.

I have implemented the changes I requested in Comment #30. I also fixed another CodeSniffer complaint from NodeRoutesTestBlock.php.

Patch, test-only version, and interdiff are attached.

Nits:

1. +++ b/core/modules/node/src/Tests/NodeRoutesTest.php
   @@ -15,7 +15,8 @@ class NodeRoutesTest extends NodeTestBase {
   +  public static $modules
   +    = ['node_routes_test', 'language', 'content_translation', 'block'];
   

   This doesn't look nice from our coding standards perspective. You should move each array item on its line. Don;t forget to add a comma also after the last item. See https://www.drupal.org/docs/develop/standards/coding-standards#array

2. +++ b/core/modules/node/tests/modules/node_routes_test/src/Plugin/Block/NodeRoutesTestBlock.php
   @@ -12,22 +15,60 @@
   +
   

   Useless empty line. Keep it more compact.

@claudiu.cristea:

New patch attached, along with test-only version and an interdiff.

I assume the empty line you were quoting is the one in NodeRoutesTestBlock::__construct(). There are four other empty lines in that hunk.

As long as I was splitting the array into multiple lines, I decided to sort the list.

You forgot to set the status back to NW, which saves me the trouble of returning it to NR. ;)

Can you answer any of the questions I asked in #30 and #31?

Ran into an issue in a contrib module where this was causing an error (https://www.drupal.org/project/simple_amp/issues/2915890). Patch in #35 worked as expected. Thanks!

This is RTBC but I cannot set it as I contributed with code here.

I can also confirm the patch works.

This is almost RTBC, I just found a super small NIT, I will provide a new patch in my next comment.

+++ b/core/modules/node/src/Tests/NodeRoutesTest.php
@@ -0,0 +1,70 @@
+    $node = Node::create([
+      'type' => 'article',
+      'title' => 'Foo',
+      'status' => NODE_PUBLISHED,
+    ]);

NODE_PUBLISHED is deprecated, we shouldn't use it anymore.

While working on this I found another issue:

+++ b/core/modules/node/src/Tests/NodeRoutesTest.php
@@ -0,0 +1,70 @@
+
+namespace Drupal\node\Tests;
+

The namespace should be Drupal\Tests\node\Functional;

So we can use Drupal\Tests\node\Functional\NodeTestBase because the other is deprecated as well.

I fixed that in my patch.

Ok, to me this is RTBC :-)

Thanks!

Patch looks good. And it fixes a problem with social_simple where viewing revisions would break.

#8 notes that this change introduces a small bc break. I think we need to address that.

I think we need a change record. @alexpott, would that be enough to address the fact that this issue is not BC (as pointed out in #7 and #8)?

I will draft a change record.

I wrote a draft change record, and I updated the issue summary. Two comments on that update:

1. The list of routes (or paths) did not match what was in the patch.
2. I left it as is, but I do not see how \Drupal\ctools\Plugin\Condition\EntityBundle::evaluate() is related to this issue.

Note that when applying the patch in #41 that it only applies for the /node/{node}/revisions/{node_revision}/* routes.
It won't work for your own routes, for example defined by a view (e.g. node/%node/my_custom).

So we have now a change record and it seems that this does not introduce a bc break.

Can we move this to RTBC?

Ok, we have a change record now, this might fix #45.

So I think this is RTBC again :-)

There is a BC break. If code is doing \Drupal::routeMatch()->getParameter('node'); before this would return a nid and now it'll return a node. The question is how much code will this break. The annoying thing is that any code that has worked around this might now break. Given the potential to break existing sites this issue needs release manager review.

I've tried to think if there is anyway that we can do this in a non disruptive way and I've not thought of anything.

I feel like 'the route parameters on a specific route' falls under 'data structures' if it's covered at all by the bc policy, so this should be eligible.

I do wonder a bit about getting this in early in 8.7.x when it opens up so there's more time to flush out contrib breakage though.

Given how much custom and contrib code I've seen that does getParameter('node') without checking the type, it might fix just as much code/fatal errors on revision pages as it breaks.

However, there's another thing to consider here. 'node' is actually basically meaningless on the revision page. It is *not* the node being viewed, the revision that's actually being viewed might be very different. The only reason it's there really is breadcrumb, so that the page is below the revision page/node page. The thing that matters is the node revision id argument.

So anything that would use that node object, for example for tokens or checking things somewhere in page preprocess might actually do things based on the current default revision and not the revision that is actually being looked at.

What I did in our distribution is this:

/**
 * Returns the current node.
 *
 * Supports the standard node page, node previews and revisions.
 *
 * @return \Drupal\node\NodeInterface
 *   The node.
 */
function xyz_get_node_from_route() {
  $node = NULL;
  if (\Drupal::routeMatch()->getParameter('node')) {
    $node = \Drupal::routeMatch()->getParameter('node');
  }
  if (\Drupal::routeMatch()->getParameter('node_preview')) {
    $node = \Drupal::routeMatch()->getParameter('node_preview');
  }
  if ($node instanceof NodeInterface) {
    return $node;
  }

  // The node revision page does not upcast the node.
  if (\Drupal::routeMatch()->getParameter('node_revision')) {
    $revision_id = \Drupal::routeMatch()->getParameter('node_revision');
    if ($revision_id > 0) {
      return \Drupal::entityTypeManager()->getStorage('node')->loadRevision($revision_id);
    }
  }

}

So this would break it, because I would have to move my revision check above the node_revision parameter (I suppose I should have done that initially..)

@Berdir are there cases where

It is *not* the node being viewed, the revision that's actually being viewed might be very different.

this is actually desired -where the node and revision are not the same node?

Not sure I get the question. Under normal circumstances, it is the same node, but not the same revision. That is the expected behavior. I guess it could even be a different node if you provide a random revision ID, but that's not supposed to be happen.

So for me this begs a question. One advantage of the current situation is that the developer has to work for the node if there is a node revision - so they have to work out whether they want the default revision or the revision the route is for. If we fix this then there's a good chance things will operate on the node object via the node parameter when they really wanted the node_revision parameter.

#58 reminds me of #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing

Works, thanks for the patch.

This should be added asap, if you ask me. It's better to have at least the default node in the route parameters then a node id string. This breaks with the current API, so this will cause errors.

I think this fix is important, because when you ask for the node parameter, you should get given it. People who have written code that relies on the broken behaviour will need a change record though.

If you want the node_revision then you ask for that instead. It makes no sense to get the ID in this one instance, especially when the reason I found this issue is because of some custom code breaking the revision pages which made me come here looking for a bug report.

Applying the patch gave me the expected behaviour for my code, so add another +1 to the RTBC.

> Applying the patch gave me the expected behaviour for my code, so add another +1 to the RTBC.

Are you sure about that?

If the code does something by getting the node and then for example the value of a certain field from it to do something, then it's possible that the value in the default node revision and the revision being looked at is actually different, and then what you are seeing on the page might be wrong (e.g. wrong block being displayed, wrong class, ...

Blindly assuming that a node route parameter is a node object is wrong and a bug in your code. It's common, but not something code should rely on.

Blindly assuming that a node route parameter is a node object is wrong and a bug in your code. It's common, but not something code should rely on.

Ah, that's news to me. (I went back and checked the API and realise now that of course we should never assume any one named parameter is always going to return the thing you expect since it's just a label anyone can use for anything.)

Consistency in core would be good though, but I shall go back and rewrite my code to actually check that the node and node_revision parameters are the correct objects expected. Thanks.

What we could do improve the DX in this problem-space is do add something like this to our RouteMatch class:

public function getEntityParameter($entity_type_id) {
  $candidates = [];
  if ($this->hasParameter($entity_type_id . '_revision')) {
    $candidates[] = $this->getParameter($entity_type_id . '_revision');
  }
  if ($this-hasParameter($entity_type_id)) {
    $candidates[] = $this->getParameter($entity_type_id);
  }

  foreach ($candidates as $candidate) {
    if ($candidate instanceof EntityInterface) {
      return $candidate;
    }
  }
}

This would save people a lot of headaches, in my opinion and would have the added side benefit of being able to properly typehint EntityInterface. A similar problem to the one described here exists with the node_preview which very little code takes into account, so we could also handle that case with such a function.

So basically putting #55 in core, is what I'm proposing, as I am now realizing ;-)

Also, I don't think the actual patch here is RTBC per the discussion above.

Yes, was wondering about something like that as well. My version also supports the node preview page, which is node specific, not sure if we want to have that there as well or not.

Adding getEntityParameter definitely makes sense to me.

What is wrong with for now at least make a (small) step forward and after that an other by improving it even more? At the moment it is broken....

Hi,

I've tested the patch in #41 against 8.5.5 and it's looking good.

There is an issue that this loads the current node revision rather than the revision you are viewing.

Steps to re-produce
- Install core with patch applied
- Add node
- Create second revision and change the title
- View both revisions side by side
- Both have the same title

What should happen
- The revisions should have different titles as they have changed between revisions

Cheers

Tom

I did some research if it is possible to have the node parameter to contain the current revision viewed. I found two things:

1. I tried to change the node_revision to an node object, but this has major code implications as everywhere this is expected to be an number and converted to a loaded revision. We could invest some time to convert this right away, so this is the right node revision.

2. In the current patch the node id is converted by the \Drupal\Core\ParamConverter\ParamConverterManager, this seems to me to be the way to go for this parameter. There is also a revision id converter, but this needs the current revision id that is in the node_revision parameter, so there is no easy way to convert the node id to the currently viewed revision. Solution to the problem would be to write a custom converter that uses the entity revision id to load the currently viewed entity. As far as I know most entities have the parameter {entity_type} and {entity_type}_revision when it's an revision, for node this would translate into the expected node & node_revision parameter. Then we can use this to load the currently viewed revision on the revision routes.

On the other hand the media module in core has the following route:

entity.media.revision:
  path: '/media/{media}/revisions/{media_revision}/view'
  defaults:
    _controller: '\Drupal\Core\Entity\Controller\EntityViewController::viewRevision'
    _title_callback: '\Drupal\Core\Entity\Controller\EntityController::title'
  options:
    parameters:
      media:
        type: entity:media
      media_revision:
        type: entity_revision:media
  requirements:
    _access_media_revision: 'view'
    media: \d+

This differences from the approach that is suggested in this thread. Would this not be a better solution to pursue? This way we keep solutions more in sync with each other and with a small added bonus that the revision is loaded already if you need it somewhere else as this has no static caching as far as I could find.

It makes sense to be in line with media in core so I added the revision parameter as described in #72.

I updated the change record.

Reference the correct comment (#18, not #11) in the issue summary.

When I applied #72, I got this message when trying to view a node revision via /node/{nid}/revisions/{revision_id}/view:

The website encountered an unexpected error. Please try again later.
TypeError: Argument 1 passed to Drupal\Core\Entity\EntityManager::getTranslationFromContext() must implement interface Drupal\Core\Entity\EntityInterface, null given, called in /home/vagrant/docroot/web/core/modules/node/src/Controller/NodeController.php on line 129 in Drupal\Core\Entity\EntityManager->getTranslationFromContext() (line 370 of core/lib/Drupal/Core/Entity/EntityManager.php).
Drupal\Core\Entity\EntityManager->getTranslationFromContext(NULL) (Line: 129)
Drupal\node\Controller\NodeController->revisionShow(Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 582)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 666)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

NodeController->revisionShow expects an integer revision ID to be passed through. I have updated the patch and provided and interdiff between this work and #72.

I messed up my root location for my file. But I also found another spot where a similar update was required. I've included that update here. Again made against #72 since the work in #75 isn't against the correct root directory.

Trying to fix the tests. There were more changes needed in the revision forms in the core that broke many tests.
Locally, fixing two single points fixed the tests but I will wait to see online as well.

Attaching the interdiff as well.
This issue needs a change record for sure.

@SKAUGHT thanks.
Also, as a sidenote for the patch, in our project, we ran across this patch because we are using the og module and we were trying to determine the context given the route. The route issue for the node revisions hit us.
The above patch, solved that issue as well without any changes which is kinda encouraging :)

Works like a charm!

We still need to address #55. My guess is that the current approach would still break that. At the very least we need a +1 from @Berdir as an entity system maintainer.

@alexpott true, I would also agree that something like that would require more people to vote for an RTBC.
As for #55, the approach changed after comment #71. The final patch is actually loading the revision being viewed instead of the latest one.

As for the code breaking other parts, it is indeed a very sensitive case. there might be many issues that will be caused by it. However, as @claudiu.cristea mentioned at #7, the code that uses the ->getParameter instead of ->getRawParameter is not entirely right since the route sets a parameter entity type.
Anyway, just food for thought and recaps here.

Looks like this needs a re-roll.

The last patch work for me in D8.7.5.

About the comment #7, the code comments in ->getParameter say:

/**
* Returns the processed value of a named route parameter.
*
* Raw URL parameters are processed by the parameter conversion system, which
* does operations such as converting entity ID parameters to fully-loaded
* entities. For example, the path node/12345 would have a raw node ID
* parameter value of 12345, while the processed parameter value would be the
* corresponding loaded node object.
*
* @param string $parameter_name
* The parameter name.
*
* @return mixed|null
* The parameter value. NULL if the route doesn't define the parameter or
* if the parameter value can't be determined from the request.
*
* @see \Drupal\Core\Routing\RouteMatchInterface::getRawParameter()
*/

If somebody is using it to get a scalar value is wrong, a "bug exploit".

I have not had a chance to test the above patch, but I didn't see anyone mention this. The same bug exists if one wrote the following code.

$node = \Drupal::request()->attributes->get('node');

In the code above, when viewing a revision it loads the node id while when viewing the node it loads the node entity object. Again, I have not tested this with the patch.

Closed #3083132: On revision pages, $variable['node'] should be a node object instead of a string as a duplicate issue.

EDIT:
sorry #88 missread.

@frob that is what the patch solves. Normally the node parameter would be the loaded node, but only on this route it is not.

1. It's be great to get a +1 on the approach from @Berdir as an entity subsystem maintainer

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -141,8 +141,7 @@ public function add(NodeTypeInterface $node_type) {
      public function revisionShow($node_revision) {
   
   @@ -159,8 +158,7 @@ public function revisionShow($node_revision) {
      public function revisionPageTitle($node_revision) {
   

   As this is a controller we can add typehints here.

3. +++ b/core/modules/node/tests/modules/node_routes_test/src/Plugin/Block/NodeRoutesTestBlock.php
   @@ -0,0 +1,73 @@
   +    $is_article_page = FALSE;
   +    if ($node = $this->routeMatch->getParameter('node')) {
   +      $is_article_page = $node->getType() == 'article';
   +    }
   +    $markup = $is_article_page ? 'article page' : 'not an article page';
   +    return [
   +      [
   +        '#markup' => $markup,
   +        '#cache' => ['max-age' => 0],
   +      ],
   +    ];
   

   This is adding markup to a page - but I don't understand how this markup is being used in a test or what it proves. I would have thought we'd be adding the node title so we can test against which revision $node = $this->routeMatch->getParameter('node') is returning.

4. +++ b/core/modules/node/tests/src/Functional/NodeRoutesTest.php
   @@ -0,0 +1,70 @@
   +    $paths = [
   +      "node/$nid/revisions",
   +      "node/$nid/revisions/$current_rid/view",
   +      "node/$nid/revisions/$initial_rid/revert",
   +      "node/$nid/revisions/$initial_rid/revert/it",
   +      "node/$nid/revisions/$initial_rid/delete",
   +    ];
   +
   +    foreach ($paths as $path) {
   +      $this->drupalGet($path);
   +    }
   

   There are no asserts against each page we're getting. So we prove the page is working I guess but for all we know they could be returning 403s.

Also the issue summary needs to outline exactly what revision is loaded in the Node param and so does the change record. It's not very clear at the moment.

This patch caused a fatal error when trying to view a revision on my site:

The website encountered an unexpected error. Please try again later.
TypeError: Argument 1 passed to Drupal\Core\Entity\EntityRepository::getTranslationFromContext() must implement interface Drupal\Core\Entity\EntityInterface, string given, called in /app/web/core/modules/node/src/Controller/NodeController.php on line 144 in Drupal\Core\Entity\EntityRepository->getTranslationFromContext() (line 99 of core/lib/Drupal/Core/Entity/EntityRepository.php).

@RoloDMonkey You may need to clear your cache as the route definitions are changing. I was seeing the same issue until I cleared my cache.

The patch in #85 worked for me with Drupal 8.8.2. Wanted to embed a split_fields diff on a revision page (/node/{nid}/revisions/{vid}/view), but route_match was returning the node ID versus the loaded node, as expected. Currently, not seeing any side affects. Happy to add another vote for RTBC, while waiting for the powers to be to decide on the remaining unknowns in #92.

We keep running into this, last week on another project.

Perhaps a somewhat off topic questions for the infrastructure team but wouldn't it be useful to have a way to test a core patch against *all* of contrib?

That way we can:
- identify the scope of this issue, how much of contrib is affected
- identify hidden issues
- notify maintainers of a CR that impacts them before its released
- increase the value of having well tested code

edit: had a conversation about this thought experiment on Slack in #drupal-infrastructure with @mikelutz and @moshe and the conclusion is that it would be (way) too expensive to set this up. Also @drumm noted that "one can always rig up something as an experiment and find/buy a place to run it. DrupalCI works locally, too.".

@mpp I am sure that would cost quite a bit, even if only on contrib which has tests. Also, seeing as how this isn't an existing feature there is no real way of knowing if that feature would create false positives.

Address coding feedback from #92.

Fixing test case failure issue in #101, please review.

The patch from #103 should have its own {@inheritdoc}

Updating issue title to reflect language of EntityConverter / EntityRevisionParamConverter

+++ b/core/modules/node/node.post_update.php
@@ -14,3 +14,9 @@ function node_removed_post_updates() {
+ * Update field names for multi-value base fields.

This needs to be updated to match the function name.

Reroll from #2730631-85: Upcast node and node_revision parameters of node revision routes reverted a part of #3008446: Complete the deprecation of format_date(), this patch now uses undefined format_date(). Its quite concerning existing test coverage didn't catch this. Node revision view routes should be blowing up.

This reveals there is no coverage for \Drupal\node\Controller\NodeController::revisionPageTitle, for which I've created #3154069: Remove NodeController::revisionPageTitle and associated _title_callback.

Nice work on addressing the feedback.

1. +++ b/core/modules/node/tests/modules/node_routes_test/src/Plugin/Block/NodeRoutesTestBlock.php
   @@ -0,0 +1,74 @@
   +    if ($node = $this->routeMatch->getParameter('node')) {
   

   Just like node we can check for node_revision and load it. Then we can update the generated output and add nid and rid to the page.

2. +++ b/core/modules/node/tests/src/Functional/NodeRoutesTest.php
   @@ -0,0 +1,76 @@
   +      "node/$nid/revisions/$current_rid/view",
   +      "node/$nid/revisions/$initial_rid/revert",
   +      "node/$nid/revisions/$initial_rid/revert/it",
   +      "node/$nid/revisions/$initial_rid/delete",
   

   We should verify these nid and rids on the page as well.

3. +++ b/core/modules/node/tests/src/Functional/NodeRoutesTest.php
   @@ -0,0 +1,76 @@
   +    foreach ($paths as $path) {
   

   This loop is not useful we can just drop it and visit individual URLs.

Fixed points 2 and 3 of #109 working on first point. Once fixed I will update here.

Addressed #109.1 and updated test accordingly.

1. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -112,15 +112,14 @@ public function addPage() {
       *   The node revision ID.
   
   @@ -130,15 +129,16 @@ public function revisionShow($node_revision) {
       *   The node revision ID.
   

   This should be "The node revision" or "The node revision entity", or something similar.

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -130,15 +129,16 @@ public function revisionShow($node_revision) {
   +    $date_formatter = \Drupal::service('date.formatter');
   

   Why can we no longer use $this->dateFormatter?

3. +++ b/core/modules/node/tests/src/Functional/NodeRoutesTest.php
   @@ -0,0 +1,90 @@
   +      'status' => Node::PUBLISHED,
   

   Shouldn't it be "NodeInterface::PUBLISHED", cannot find any occurrence of "Node::PUBLISHED", but there are some with NodeInterface::PUBLISHED.

Hi @johnwebdev
Updated patch please review.

Fixing failed test cases.

Fixed failed test cases.

1. +++ b/core/modules/node/node.routing.yml
   @@ -69,6 +71,12 @@ entity.node.revision:
        node: \d+
   +  options:
   +    parameters:
   +      node:
   +        type: entity:node
   +      node_revision:
   +        type: entity_revision:node
    
    node.revision_revert_confirm:
   

   nothing has really changed about the fact that this _will_ break modules and sites out there that currently hardcode node being an integer and do a Node::load(getParameter('node')).

   Some examples that I found with a quick search are dynamic enough, e.g. webform_node_preprocess_page_title().

   But code like http://grep.xnddx.ru/node/29484616#line-460 and http://grep.xnddx.ru/node/30324552#line-31 is going to break, hard.

   However, it's probably going to fix at least as many places that currently result in a fatal error on a revision page, code like http://grep.xnddx.ru/node/29433835#line-49 and http://grep.xnddx.ru/node/29451799#line-78, that just blindly assumes that the node parameter is a node entity.

   There is no solution where we can make everyone happy. I don't know what our BC policy is in regards to route definitions and parameters, https://www.drupal.org/core/d8-bc-policy just mentiones route controllers and filters, not the existence of routes themself.

   And see also the discussion in #55-58, where I also explain the second possible problem. That this will break things in more subtle ways, because code might work with the default revision of the node instead of the specific revision being displayed on that page.

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -130,15 +129,17 @@ public function revisionShow($node_revision) {
       */
   -  public function revisionPageTitle($node_revision) {
   -    $node = $this->entityTypeManager()->getStorage('node')->loadRevision($node_revision);
   -    return $this->t('Revision of %title from %date', ['%title' => $node->label(), '%date' => $this->dateFormatter->format($node->getRevisionCreationTime())]);
   +  public function revisionPageTitle(NodeInterface $node_revision) {
   +    /** @var \Drupal\Core\Datetime\DateFormatterInterface $date_formatter */
   +    return $this->t('Revision of %title from %date', ['%title' =>
   +    $node_revision->label(), '%date' => $this->dateFormatter
   +    ->format($node_revision->getRevisionCreationTime())]);
      }
   

   $date_formatter doesn't exist anymore, I guess was a reroll that converted things back to an earlier version. The only change on this line should be $node to $node_revision.

3. +++ b/core/modules/node/tests/modules/node_routes_test/src/Plugin/Block/NodeRoutesTestBlock.php
   @@ -0,0 +1,84 @@
   +  public function build() {
   +    $cacheable_metadata = CacheableMetadata::createFromObject($this);
   +    $article_page_markup = 'A page without node';
   +    $revision_page_markup = ' and without revision';
   +    // Upcasted node object.
   +    if ($node = $this->routeMatch->getParameter('node')) {
   +      $article_page_markup = sprintf('A page with node: (%s)', $node->id());
   +      $cacheable_metadata->addCacheableDependency($node);
   +    }
   +    // Upcasted node revision object.
   +    if ($node_revision = $this->routeMatch->getParameter('node_revision')) {
   +      $revision_page_markup = sprintf(' and node revision: (%s)', $node_revision->getRevisionId());
   +    }
   +
   +    $build = [
   +      '#markup' => $article_page_markup . $revision_page_markup,
   +    ];
   +    // @todo Remove this in https://www.drupal.org/node/2879244.
   +    $cacheable_metadata->setCacheMaxAge(0);
   +    // Apply cacheability.
   +    $cacheable_metadata->applyTo($build);
   +    return $build;
   +  }
   +
   

   As I commented in that issue, that won't do anything to fix this. So the reason this isn't working is because this is using the parameters from the route match directly. That means it needs to use the route cache context.

As a start, one thing we should do is flesh out https://www.drupal.org/node/2942013, explain that those routes will have *two* node entities as parameters, and code needs to decide which one to use, node_revision or node. Sometimes it doesn't matter, for example for if you just want to check the node type, that can't change, but if you display information from that node or decide based on other fields, it might be wrong.

So given the example/test block being added here, if that would use the plugin context system to get the "current node", should it receive node or node_revision? Right now that doesn't work at all because \Drupal\node\ContextProvider\NodeRouteContext::getRuntimeContexts() doesn't support this route. This patch will "fix" that, and it will be good enough for the block visiblity condition, but not if you actually pass the node to the block and e.g. display the node title. So maybe we should actually update \Drupal\node\ContextProvider\NodeRouteContext() to specifically return the node_revision parameter if that exists and is a node entity? Just like #2890758: Block visibility node type not working on preview/revision routes adds support for node_preview.

Last comment for now. We could also split out the change for NodeRouteContext, add support for node_revision as both a node entity and a revision ID and just support that explicitly, as I think we should consider that anyway there. That might solve the problem for a lot of the 100 people subscribed here, which care about the related ctools, display suite and the 10 duplicate core bug reports about node type block visibility conditions not working. And would have no/minimal BC impact.

Thank you for the thorough feedback @Berdir. This is a lot to process. :D
RE: #119:

1. This is of course a difficult situation but IMO if people are using getParameter instead of getRawParameter then that's not for core to fix or care about. We can make it clear in change notice and add to release notes as well.
   About #55 - #58 I think that is for the custom code to decide either to use node or node_revision as node this is an existing problem this fix is just making it evident.
2. Fixed.
3. I thought I tried that but it seems like I was not doing it the right way I tried it again and it works. Thanks for looking into it.

Removing "Needs subsystem maintainer review" tag after @Berdir's review. We do need to sum up all the discussion in IS so added IS update tag.

Thanks for the updates.

I also commented in Slack here: https://drupal.slack.com/archives/C1BMUQ9U6/p1593806552394300:

I honestly don't know what to do about https://www.drupal.org/project/drupal/issues/2730631#comment-13732728. 100 people are following that issue and want a fix, it's going to break some existing modules and custom code out there, and it's going to "fix" others, but might at the same time also result in more subtle bugs (or replace a fatal error with that subtle bug) of operating on the default revision instead of the one being displayed. E.g. a block showing author information would show the wrong kind of information. But it would also fix the block from not showing up at all when configured with a node type visibility condition. @catch, @xjm any thoughts on BC for route parameter changes?

Got some feedback from @catch:

catch: @berdir I don't have great ideas. The change is definitely allowable so it's down to how much we want to break/fix things.
I started thinking about adding new versions of the routes, changing links etc. but that seems worse than just doing it. Another way to do it would be to say make it 10.0-only, but that leaves it sitting in the queue for a while then.
Your last comment on splitting out NodeRouteContext sounds worth doing to take the pressure off this one.

berdir: yes, actually wondering about just adding it to the preview issue, I think it's just another if condition in there and a handful of lines extra for test coverage. not sure if fixing both in the same issue would make it harder to get committed

So given the comment from @catch above, we do have the official feedback that is indeed allowed. Fair enough to me. To be honest, I never bothered with getRawParameter() when I got a non-upcasted object from getParameter() and I'm sure plenty of others didn't do that either.

> this is an existing problem this fix is just making it evident.

It's not really an existing problem though. The code snippet in #55 is not very nice, but it's a real-world example from our application and it will have a subtle behavior change as it will now return early from the node parameter, but that's not really the version of the node that you're actually seeing. That might or might not affect your application but it could be very hard to track down if it does. But yeah, adding this to the release nodes sounds like a good idea, so replacing the release manager feedback tag with release note.

About the NodeRouteContext service, if you have a block or condition plugin that requires a node context to be injected, wouldn't you expect to get the revision on the node revision page? So as proposed, lets fix that first, I think I can include that in #2890758: Block visibility node type not working on preview/revision routes easy enough and as a separate issue it would conflict pretty hard. I feel like I scared people of with me pretty big changes over there, will work on a patch with that now, would really appreciate a review there. Then this will have one surprise change less than now.

+++ b/core/modules/node/tests/modules/node_routes_test/src/Plugin/Block/NodeRoutesTestBlock.php
@@ -0,0 +1,83 @@
+ * Provides a testing block for node routes.
+ *
+ * @Block(
+ *  id = "node_routes_test_block",
+ *  admin_label = @Translation("Node routes test block")
+ * )
+ */
+class NodeRoutesTestBlock extends BlockBase implements ContainerFactoryPluginInterface {
+

The other issue landed now. I would suggest you extend this test to pass in the node context and then assert which revision we get, possibly additionally to the route parameters.

We can also simplify the implementation in \Drupal\node\ContextProvider\NodeRouteContext::getRuntimeContexts() now.

Hm, I did already add a block using context in my issue for testing it, so possibly we can skip that here and stick with the route parameters. But we can definitely simplify the context provider.

This addressed #125. Updated IS and added release notes. I have also updated change notice with node revision example.

+++ b/core/modules/node/src/ContextProvider/NodeRouteContext.php
@@ -45,10 +45,8 @@ public function getRuntimeContexts(array $unqualified_context_ids) {
-        $value = \Drupal::entityTypeManager()->getStorage('node')->loadRevision($revision_id);
+      if ($revision = $this->routeMatch->getParameter('node_revision')) {
+        $value = $revision;
       }

we should add a check for route contexts like node below or instanceof I guess, in case there's a node_revision argument out there that is not upcasted?

Addressed #127.

This is a blocker for #3043321-51: Use generic access API for node and media revision UI

1. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -112,15 +112,14 @@ public function addPage() {
       *   An array suitable for \Drupal\Core\Render\RendererInterface::render().
       */
   -  public function revisionShow($node_revision) {
   -    $node = $this->entityTypeManager()->getStorage('node')->loadRevision($node_revision);
   -    $node = $this->entityRepository->getTranslationFromContext($node);
   +  public function revisionShow(NodeInterface $node_revision) {
   +    $node = $this->entityRepository->getTranslationFromContext($node_revision);
        $node_view_controller = new NodeViewController($this->entityTypeManager(), $this->renderer, $this->currentUser(), $this->entityRepository);
        $page = $node_view_controller->view($node);
        unset($page['nodes'][$node->id()]['#cache']);
   

   just like the default param converter, \Drupal\Core\ParamConverter\EntityRevisionParamConverter should do the getTranslationFromContext() call automatically and we can remove that here.

   We should make sure that we have tests for that though. Maybe a test-only patch that calls getUntranslated() to force the default translation and see if something breaks?

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -130,14 +129,14 @@ public function revisionShow($node_revision) {
       */
   -  public function revisionPageTitle($node_revision) {
   -    $node = $this->entityTypeManager()->getStorage('node')->loadRevision($node_revision);
   +  public function revisionPageTitle(NodeInterface $node_revision) {
   +    $node = $this->entityRepository->getTranslationFromContext($node_revision);
        return $this->t('Revision of %title from %date', ['%title' => $node->label(), '%date' => $this->dateFormatter->format($node->getRevisionCreationTime())]);
   

   same here then.

3. +++ b/core/modules/node/tests/modules/node_routes_test/src/Plugin/Block/NodeRoutesTestBlock.php
   @@ -0,0 +1,83 @@
   +  public function build() {
   +    $cacheable_metadata = CacheableMetadata::createFromObject($this)
   

   createFromObject($this) shouldn't be needed. That is already collected in \Drupal\block\BlockViewBuilder::viewMultiple()

   Which also means that you can actually just implement getCacheContects and do this:

   return Cache::mergeContexts(parent::getCacheContexts(), ['route']);

4. +++ b/core/modules/node/tests/src/Functional/NodeRoutesTest.php
   @@ -0,0 +1,79 @@
   +   */
   +  public static $modules = [
   +    'block',
   +    'content_translation',
   +    'language',
   +    'node_routes_test',
   +  ];
   

   what are we using content translation for here? I see you create a language, but I don't see anything that actually uses language or creates translations?

Just found a duplicate that does mostly the same as this issue #2678492: Convert the node revision converter to the generic entity one. Too many issues :-/

Addresssed #130 but not sure about

We should make sure that we have tests for that though. Maybe a test-only patch that calls getUntranslated() to force the default translation and see if something breaks?

Any ideas here, but do we really want to do this? IMO, testing EntityRevisionParamConverter here should be out of scope.

Closed #2678492: Convert the node revision converter to the generic entity one as duplicte and transferred the credits here.

Marked change notice as duplicate as well.

1. +++ b/core/modules/node/tests/modules/node_routes_test/src/Plugin/Block/NodeRoutesTestBlock.php
   @@ -58,17 +60,21 @@ public static function create(ContainerInterface $container, array $configuratio
      public function build() {
   -    $cacheable_metadata = CacheableMetadata::createFromObject($this)
   -      ->addCacheContexts(['route']);
   +    $cacheable_metadata = new CacheableMetadata();
   

   $cacheable_metadata is now unused, you can remove all of that.

2. +++ b/core/modules/node/tests/modules/node_routes_test/src/Plugin/Block/NodeRoutesTestBlock.php
   @@ -58,17 +60,21 @@ public static function create(ContainerInterface $container, array $configuratio
   -      $article_page_markup = sprintf('A page with node: (%s)', $node->id());
   +      assert($node instanceof NodeInterface);
   +      if ($langcode = $this->routeMatch->getRawParameter('langcode')) {
   +        $node = $node->getTranslation($langcode);
   +      }
   +      $article_page_markup = sprintf('A page with node: (%s), langcode: (%s),', $node->id(), $node->language()->getId());
          $cacheable_metadata->addCacheableDependency($node);
   

   Hm, this isn't what I meant. why the langcode query argument, what is that, where is it coming from?

   If you want to test the translation from context stuff then you instead should set up path language negotiation properly and access it as it/node/...

   But I did not mean to suggest we need new test coverage. I'm just asking to verify that we do have existing functional test coverage for the actual revision controller that it displays things in the right translation, so my proposal was to break it on purpose, to see what happens. A single-line patch that just removes th getTranslationFromContext() line would be sufficient for that. Or, we can also try to find it by hand. A bit of searching pointed me to \Drupal\Tests\node\Functional\NodeRevisionsTest::testRevisions, so yes, we should have test coverage, and that should fail if we would break it.

   So IMHO, none of the test and block changes are necessary, as you said, it is out of scope. I just wanted to ensure that we don't have a regression here.

1. We do want to cache block output per node so this is how it has been used.
   

       $cacheable_metadata->addCacheableDependency($node);
       ....
       // Apply cacheability.
       $cacheable_metadata->applyTo($build);
   

2. Remove the langcode from the block but added an assert to test \Drupal\node\Form\NodeRevisionRevertTranslationForm is working fine.

1. Ah sorry, I missed that. Yes, that's needed then in this case, the neat thing about plugin contexts is that it would automatically get the necessary cache contexts and tags, so both $cacheable_metadata->addCacheableDependency($node); and the route thing wouldn't be needed and it would "just work", now that the other issue is fixed. So for a real block that displays info about the node on node pages, you'd just that. But for this test, it makes sense to check the two arguments explicitly. Although to be honest, we're kind of already testing that with the actual forms and controllers, making sure that _those_ receive the right revision and translation. So now that the block context stuff works separately from this, that test isn't adding too much benefit anymore IMHO. Also the revert/it part, that's tested in \Drupal\Tests\node\Functional\NodeRevisionsTest::testRevisionTranslationRevert.

Should we remove the tests altogether?

Did two things:

1. Cleaned up the NodeRoutesTestBlock, removed $cacheable_metadata & route context (as suggested in #141).
2. Rewrote the comment in NodeRouteContext to actually add some extra context instead of just saying the same as the code below it.

Hopefully we can commit this soon, I have been using the patches from this issue for ages now!

@pdenooijer thanks for the patch but these code changes are not needed atm. @Berdir, @alexpott and I are trying to figure out the next steps here and will update the issue and patch soon.

1. +++ b/core/modules/node/src/ContextProvider/NodeRouteContext.php
   @@ -44,7 +44,7 @@
   -      // Check for a node revision parameter first.
   +      // Try to find the most specific node.
          if (isset($route_contexts['node_revision']) && $revision = $this->routeMatch->getParameter('node_revision')) {
   

   This is not a node this is a version of the node. $route_contexts['node_revision'] means that we indeed want to load the node revision so the comment changes is missleading.

2. +++ b/core/modules/node/tests/modules/node_routes_test/src/Plugin/Block/NodeRoutesTestBlock.php
   @@ -60,14 +58,12 @@
   -    $cacheable_metadata = new CacheableMetadata();
   ...
   -      $cacheable_metadata->addCacheableDependency($node);
   
   @@ -80,14 +76,5 @@
   -    $cacheable_metadata->applyTo($build);
   

   These are needed as explained in #140.

3. +++ b/core/modules/node/tests/modules/node_routes_test/src/Plugin/Block/NodeRoutesTestBlock.php
   @@ -80,14 +76,5 @@
   -  public function getCacheContexts() {
   -    return Cache::mergeContexts(parent::getCacheContexts(), ['route']);
   -  }
   

   This change will fail the tests becasue the block is suppose to change the output wrt different routes.

Please don't make any further changes to the patch. We'll post an update as soon as we have one.

Sure, just trying to help :)

Apparently nobody wants to make that decision, then I'll do. Lets remove the test. I don't think it's worth the amount of lines of code. Sure it would fail, but this isn't actually fixing a bug anymore IMHO, not since we fixed the other issue. It's just a refactoring task and if this fixes your code then your code is broken, sorry :)

So changing to a task and needs work to remove the test related code.

Removed the tests as requested by Berdir. I also had an other look if there is nothing left to do, seems not to me. Let's see if the current test suite agrees with this patch.

Thanks. The patch is as ready as it can be now. Now we'll need a definitive definition from release/framework managers about whether we want to this. This changes routes, controller methods and a form, which technically is alllowed based on our BC policy, but we're often also nicer than we define in those rules.

I'm also slightly extending the issue summary, the main purpose here is no longer bugfixing, routes may have node and node_revision parameters that aren't upcasted and code _must_ deal with that and check the type and/or use the raw parameters. Either way, this will break some modules and fix others on the routes that we're changing. And the bugs around node context/conditions on preview as well as revision pages has been fixed by by #2890758: Block visibility node type not working on preview/revision routes.

Instead, the main reason is now to unblock #3043321: Use generic access API for node and media revision UI, we need upcasted objects to introduce a generic (route) entity revision access API, or more specifically, we need to do these changes if we want to use that API for nodes.

Looks like unrelated test failure, so back to RTBC as per comment #150

Hi, thanks for all the patches
We've been using the patches provided here for our site and they worked beautifly.

Just a quick question, we just updated our site to Drupal 9.0.x recently and wondering which patch should we use for 9.0.x ? The latest patch doesn't seem to be applyable for 9.0.x.

Did you try the latest 8.x patch? As Drupal 9.0 is basically the same as 8.9 but without the deprecated code.

> We've been using the patches provided here for our site and they worked beautifly.

What I'm wondering is why you are using this patch. What I've said before is that sites should _not_ rely on this patch to avoid errors. I changed it to a task because this might fix such errors, but that's not/no longer the goal, just a side effect. Depending on your code, it might result in more subtle bugs when looking at revisions for example.

See #150.

If you were using it for block visibility, then that was fixed by #2890758: Block visibility node type not working on preview/revision routes, unfortunately, that hasn't been backported to 9.0 (yet and I guess won't happen anymore now)

I still feel about the same as #23#123. Route definitions are 'data structure' rather than API to me - so having to update for a change like this is similar to updating for render arrays or preprocess variables https://www.drupal.org/about/core/policies/core-change-policies/drupal-8...

However this is going to fail a lot harder than say changing an access check or the route title or similar.

However, route definitions aren't listed on the bc policy at all, and we make this kind of change far less often. So... I think we need to update the bc policy with something for routes so they're listed explicitly.

Still also pondering whether this should be 10.0.x-only too. Will get a second or third opinion.

Ok. Currently the issue to unify revision access is postponed on this. And a generic revision UI is then in turn postponed on the access issue.

If we decide that this is 10.0.x material then we would either need to postpone all those issues to 10.x as well *or* continue for now only with test and entities that do not yet have revision routes like media, block_content and so on. IMHO that would be an acceptable way forward, and then we would refactor node to use this in 10.x. I've been wondering before if we shouldn't have done that long ago, only major downside IMHO is that we for now wouldn't have a real-life use case for that revision access system until we start to add a generic UI and use it for new entity types.

To be clear, I perfectly understand the reason for why we'd want to push it back, not arguing against it, just outlining the impact of that decision :) I am well aware that this is a tricky change and I already changed the issue from a bugfix to just a task/refactoring.

One advantage of committing now-ish is we're still quite early in the 9.2.x cycle, so it gives us a few months run-up before alpha/beta for contrib modules to be updated. But yeah feels like early-in-a-minor or delay-until-major.

I believe @catch in #158 intended to refer to #123 not #23.

Re-uploading 2730631-149.patch so it gets retested every 2 days against 9.2.x-dev.

Currently it's tested every 2 days against 9.1.x-dev.

Going to add [9.3.x] to the title here - we either need to commit this at the earliest possibly point in a minor branch once it's opened, or the same for 10.0.x. Leaving the 'needs release manager review' tag on so that @xjm can comment.

Rerolled patch in #162.

On this case is a long line with more than 80 chars

return $this->t('Revision of %title from %date', ['%title' => $node_revision->label(), '%date' => $this->dateFormatter->format($node_revision->getRevisionCreationTime())]);

Please could you split the array in another line? Is better to read the code. E.g.:

return $this->t('Revision of %title from %date', [
  '%title' => $node_revision->label(),
  '%date' => $this->dateFormatter->format($node_revision->getRevisionCreationTime()),
]);

This is important for the code to be more readable and is easier to future maintenance

Made the change suggested in #168. Please review.

Thanks @ayushimishra206,

Probably best to indent the closing brackets on the same indentation as the return statement

+    return $this->t('Revision of %title from %date', [
+      '%title' => $node_revision->label(),
+      '%date' => $this->dateFormatter->format($node_revision->getRevisionCreationTime()),
+    ]);

Address #171.

Splitting the array over multiple lines is fine, but this change to indentation looks unrelated:

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -235,8 +235,8 @@
-                Url::fromRoute('node.revision_revert_translation_confirm', ['node' => $node->id(), 'node_revision' => $vid, 'langcode' => $langcode]) :
-                Url::fromRoute('node.revision_revert_confirm', ['node' => $node->id(), 'node_revision' => $vid]),
+              Url::fromRoute('node.revision_revert_translation_confirm', ['node' => $node->id(), 'node_revision' => $vid, 'langcode' => $langcode]) :
+              Url::fromRoute('node.revision_revert_confirm', ['node' => $node->id(), 'node_revision' => $vid]),
             ];

The 9.3.x branch is open, so this could (finally) be committed now. Discussed this briefly with @xjm and I think she agreed an 9.3.x commit was the least worst option at this stage, although it was a few weeks ago and I've changed my own mind here more than once. Tentatively untagging.

Addressed #174. There have been just coding fixes since #167 that is why setting it back to RTBC.

Committed c6adb7c and pushed to 9.3.x. Thanks!

If this breaks a lot of contrib modules, we might have to roll back, but at least by committing now we're leaving the maximum amount of time for them to get fixed.

Thanks! This unblocks #3043321: Use generic access API for node and media revision UI.

#2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider needs review but it is not for the faint of heart.