Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xtfer’s picture

Assigned: Unassigned » xtfer
xtfer’s picture

Assigned: xtfer » Unassigned
xtfer’s picture

Assigned: Unassigned » xtfer
xtfer’s picture

Status: Active » Needs work
FileSize
4.27 KB

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.

xtfer’s picture

Status: Needs work » Needs review
FileSize
4.25 KB
dawehner’s picture

+++ 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.

Status: Needs review » Needs work

The last submitted patch, node-view-controller-5.patch, failed testing.

xtfer’s picture

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...

xtfer’s picture

dawehner’s picture

+++ 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.

monan’s picture

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!

xtfer’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
4.35 KB

Re-rolling...

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, 1987768-node_view-controller-13.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, 1987768-node_view-controller-13.patch, failed testing.

dawehner’s picture

+++ 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.)

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
452 bytes
4.31 KB

Thanks for the review @dawehner.

1. Not sure why we need to have full namespace inside class. As we can use one namespace with that name, we should able to map it in doc generator. I have created #2026085: Remove full path reference of method argument/comment inside class for this.

2. Updated title back.

Status: Needs review » Needs work

The last submitted patch, 1987768-node_view-controller-18.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
4.78 KB

Seems title callback trying to get the node object from URL which is not happening. So removing title callback and it's function as we are doing same in controller.

 /**
- * 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();
-}

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, 1987768-node_view-controller-20.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, 1987768-node_view-controller-20.patch, failed testing.

dawehner’s picture

Assigned: xtfer » Unassigned
Status: Needs work » Needs review
FileSize
4.76 KB

This was easier to fix then expected

Status: Needs review » Needs work

The last submitted patch, drupal-1987768-24.patch, failed testing.

pguillard’s picture

Status: Needs work » Needs review
FileSize
4.81 KB

Re-rolled patch

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, drupal-1987768-26.patch, failed testing.

pguillard’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, drupal-1987768-26.patch, failed testing.

berliner’s picture

Status: Needs work » Needs review
FileSize
3.56 KB

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.

berliner’s picture

FileSize
5.39 KB

Forgot part of the patch.

Status: Needs review » Needs work

The last submitted patch, drupal-1987768-31.patch, failed testing.

berliner’s picture

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.

berliner’s picture

FileSize
6.64 KB

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, ...

ParisLiakos’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-1987768-34.patch, failed testing.

berliner’s picture

Status: Needs work » Needs review
FileSize
7.21 KB

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);
   }

Status: Needs review » Needs work

The last submitted patch, drupal-1987768-37.patch, failed testing.

dawehner’s picture

+++ 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.

Crell’s picture

Priority: Normal » Major

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

berliner’s picture

Status: Needs work » Needs review
FileSize
9.51 KB

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.

Status: Needs review » Needs work

The last submitted patch, drupal-1987768-41.patch, failed testing.

berliner’s picture

Status: Needs work » Needs review
FileSize
9.8 KB

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+
berliner’s picture

FileSize
11.35 KB

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.

piyuesh23’s picture

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

Status: Needs review » Needs work

The last submitted patch, drupal-1987768-44.patch, failed testing.

berliner’s picture

FileSize
13.24 KB
berliner’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-1987768-47.patch, failed testing.

ygerasimov’s picture

+++ 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?

ygerasimov’s picture

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.

berliner’s picture

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

berliner’s picture

Status: Needs work » Needs review
Issue tags: -WSCCI-conversion

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

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, drupal-1987768-47.patch, failed testing.

larowlan’s picture

Crell’s picture

Assigned: Unassigned » Crell

Claiming.

Crell’s picture

Status: Needs work » Closed (duplicate)