Problem/Motivation

At the moment most of the time node route parameter is node object but in some cases it is entity id which is inconsistent and makes people write extra logic whenever they have to use that parameter.

To get entity id on node page:

 $node = \Drupal::routeMatch()->getParameter('node');
 $nid = $node->id();

and on node revision page:

 $nid = \Drupal::routeMatch()->getParameter('node');

Proposed resolution

Decide whether Node route parameter should be entity id or node object and make all use cases implement the chosen method.

Remaining tasks

-

User interface changes

-

API changes

-

CommentFileSizeAuthor
#10 node_route_parameter_inconsistent-2495703-10.patch473 bytesMichelle
#7 node_route_parameter_inconsistent-2495703-7.patch466 bytesMichelle
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

Could you give an example of where this goes wrong?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer and at Druid commented
Issue summary: View changes
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer and at Druid commented

This is bad because examples like this will break node revisions: http://drupal.stackexchange.com/questions/145823/how-to-get-current-node...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented

Good observation.

We certainly want to have a full entity object available. The reason why the node object is not converted on the revision route is because, the controller looks like the following;

  public function revisionShow($node_revision) {
    $node = $this->entityManager()->getStorage('node')->loadRevision($node_revision);
    $node_view_controller = new NodeViewController($this->entityManager, $this->renderer);
    $page = $node_view_controller->view($node);
    unset($page['nodes'][$node->id()]['#cache']);
    return $page;
  }

and $node is not used there. One way to fix that is by setting the following bit onto the route definition:

  options:
    parameters:
      node:
        type: 'entity:node'
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer and at Druid commented
Version: 8.0.x-dev » 8.2.x-dev

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Michelle’s picture

Michelle
she/her
Primary language English
Location Wisconsin, USA
CreditAttribution: Michelle at Mediacurrent commented
Status: Active » Needs review
FileSize
node_route_parameter_inconsistent-2495703-7.patch466 bytes

I made a patch based on dawehner's suggestion in #4 and it works to be able to use $node = \Drupal::routeMatch()->getParameter('node'); on both node view and node revision view without error.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer and at Druid commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks for the patch! It would be nice to see test coverage for this

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

We should use 

   options:
     parameters:
       node:
         type: entity_revision:node

now. This loads even the right entity object.

Michelle’s picture

Michelle
she/her
Primary language English
Location Wisconsin, USA
CreditAttribution: Michelle at Mediacurrent commented
FileSize
node_route_parameter_inconsistent-2495703-10.patch473 bytes

I made the suggested change in #9 and tested it and it's still working. I don't know how to write tests so leaving this at Needs Work.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Great, well, to be honest I'm not sure what kind of test coverage @lauriii is asking for.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

I don't understand. Isn't the {node} parameter of that route the node ID and should use the entity: upcasting, not entity_revision:?

Can someone explain?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Oh you are totally right, of course, so do you think we should just upcast to the node and be done with it?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

Yeah, I think #7 is the way to go.

nicrodgers’s picture

nicrodgers
Primary language English
Location Monmouthshire, UK
CreditAttribution: nicrodgers at Welsh Government commented
Related issues: +#2730631: Upcast node and node_revision parameters of node revision routes
20th’s picture

20th CreditAttribution: 20th commented
Status: Needs work » Closed (duplicate)

Changing status to duplicate of the above issue for the following reasons.