Convert node_page_view() to a new style controller

First pass...

I expect this to fail, but I need someone to look at the failing tests and I have to get some sleep, so posting it anyway.

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeViewController.phpundefined
@@ -0,0 +1,59 @@
+   * @todo This should be a NodeInterface, when Nodes are actually changed over.
...
+  public function nodeView(EntityInterface $node) {

So Node already implements NodeInterface ... so we can already use NodeInterface, right?

+++ b/core/modules/node/node.routing.ymlundefined
@@ -0,0 +1,8 @@
+    _permission: 'access content'

If someone has "access content" this will allow access on the node, even node_access fails. I hope we have test coverage for that!

+++ b/core/modules/node/node.routing.ymlundefined
@@ -0,0 +1,8 @@
\ No newline at end of file

... we need a newline at the end of the file.

So Node already implements NodeInterface ... so we can already use NodeInterface, right?

No, its still using the EntityBCInterface shim. I had NodeInterface here originally.

If someone has "access content" this will allow access on the node, even node_access fails. I hope we have test coverage for that!

That's how it works now, is it not?

Anyway, reroll coming...

This is blocked by #1987778: Convert node_show() and node_page_view() to a new style controller.

+++ b/core/modules/node/node.moduleundefined
@@ -1697,14 +1697,7 @@ function node_menu() {
-    'access callback' => 'node_access',
-    'access arguments' => array('view', 1),

This is how it worked before ..., so access content has not been used before, as far as I see.

There is a patch posted for #1987778: Convert node_show() and node_page_view() to a new style controller. It's got a typecasting testing problems/issues that I'm not sure how to fix. Any input would be welcome so that it can unblock this...

Thanks!

so access content has not been used before, as far as I see.

You are correct, not sure how I snuck that in there. Thanks.

It's got a typecasting testing problems/issues that I'm not sure how to fix. Any input would be welcome so that it can unblock this.

I'll try and take a look before Friday, so we can resolve both of them if we can.

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeViewController.phpundefined
@@ -0,0 +1,57 @@
+   * @param NodeInterface $node

Should contain the full namespace.

+++ b/core/modules/node/node.moduleundefined
@@ -1634,14 +1634,7 @@ function node_menu() {
-    'title callback' => 'node_page_title',
-    'title arguments' => array(1),

I guess there should be still a title callback for menu links? (not sure tbh.)

This was easier to fix then expected

Re-rolled patch

#26: drupal-1987768-26.patch queued for re-testing.

As far as I can see, some tests fail that rely on node_access_test module, e.g.BookTest.php. The problem as I understand it, is that whenever a module implements hook_access_grants() NodeAccessController->accessGrants() is called with the wrong langcode resulting in a query that searches for nodes with langcode 'UND' in the node_access table. The wrong language code comes from the Entity class where the LANGCODE_DEFAULT is hardcoded into the arguments of the entities AccessController instance. If we change that to the actual langcode of the entity then it seems to work.

   public function access($operation = 'view', AccountInterface $account = NULL) {
     return \Drupal::entityManager()
       ->getAccessController($this->entityType)
-      ->access($this, $operation, Language::LANGCODE_DEFAULT, $account);
+      ->access($this, $operation, $this->language()->id, $account);
   }

Besides, it seems only logical to me, that the entities langcode is passed along to the access checker, but then again, I do not completely understand the implications of this.

Forgot part of the patch.

Ok, this is obviously too complicated for me. After the last test fail, I just picked the first fail and tracked the error down to the fact, that converted router entries do not have a loading function stored in table menu_router anymore, which effectively breaks node_is_page(). That is because menu_get_object() relies on the existence of load_functions.

Another trial with a proposal from fubhy to temporarily solve the problem with menu_get_object() which effects a lot of other places in the code, e.g. theme.inc, comments, ...

Same change to EntityNG access method as in #30:

   public function access($operation = 'view', AccountInterface $account = NULL) {
     return \Drupal::entityManager()
       ->getAccessController($this->entityType)
-      ->access($this, $operation, $this->activeLangcode, $account);
+      ->access($this, $operation, $this->language()->id, $account);
   }

+++ b/core/modules/node/node.moduleundefined
@@ -1321,14 +1321,7 @@ function node_menu() {
   $items['node/%node'] = array(
-    'title callback' => 'node_page_title',
-    'title arguments' => array(1),

@@ -1404,21 +1397,6 @@ function node_menu_local_tasks(&$data, $router_item, $root_path) {
 /**
- * Title callback: Displays the node's title.
- *
- * @param \Drupal\Core\Entity\EntityInterface $node
- *   The node entity.
- *
- * @return
- *   An unsanitized string that is the title of the node.
- *
- * @see node_menu()
- */
-function node_page_title(EntityInterface $node) {
-  return $node->label();
-}

I think we have to keep the title callback/argument for now to support menu links of nodes.

This is a blocker for #2019123: Use the same canonical URI paths as for HTML routes, so bumping to major.

Comment tests (CommentInterfaceTest.php) for example has failed because the test includes a GET request to /node/NID/COMMENT_ID which results in a 404.
How are fallback callbacks implemented in the new routing system? E.g. before we could do node/3/4 and if no menu item existed for that it stripped the last argument and tried node/3. How is this working in the new system?

Trying another patch, hopefully reducing failures.

Breadcrumb test failed because there was no route inheritance for /node/% vs. /node/%/view as it was possible in the old system. I don't know how that can be done using the new routing system, so I added a second route for the moment.

   $items['node/%node/view'] = array(
+    'route_name' => 'node_view_default_task',
     'title' => 'View',
     'type' => MENU_DEFAULT_LOCAL_TASK,
   );

+node_view_default_task:
+  pattern: '/node/{node}/view'
+  defaults:
+    _content: '\Drupal\node\Controller\NodeViewController::render'
+  requirements:
+    _entity_access: 'node.view'
+    node: \d+

Found #1995620: [policy, no patch] Document how to handle routes for MENU_DEFAULT_LOCAL_TASK, so second route is no option. I removed the now obsolete test parts from BreadcrumbTest.php.

The patch is working as expected. Tested it for the node/{node} route. Attaching a screenshot for reference.

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeViewController.phpundefined
@@ -0,0 +1,57 @@
+/**
+ * @file
+ * Contains \Drupal\node\Controller\NodeController
+ */

Should be "Contains ...\NodeViewController"

After applying this patch, when I open /node/X/view page I got 404. Worst thing is that I still see tabs View and Edit. I do not think it is good, so the approach with having "empty" menu link to "view" tab is not really nice. Can we reuse same route for node/X in node/X/view?

Tabs to be displayed on 404 page should be fixed in #2004334. Probably breadcrumbs will be better as well. We need to test this patch after applying 2004334.

The 404 for /node/%/view is ok I suppose. This behavior complies with https://drupal.org/node/1953346.

#47: drupal-1987768-47.patch queued for re-testing.

Related #2040265: Add a _entity_view routing default for viewing an entity

Claiming.

On further review, I've folded this into #1987778: Convert node_show() and node_page_view() to a new style controller.