Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Files: 
CommentFileSizeAuthor
#47 drupal-1987768-47.patch13.24 KBberliner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1987768-47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#45 Screen Shot 2013-07-03 at 9.40.41 PM.png216.41 KBpiyuesh23
#44 drupal-1987768-44.patch11.35 KBberliner
FAILED: [[SimpleTest]]: [MySQL] 56,977 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#43 drupal-1987768-43.patch9.8 KBberliner
FAILED: [[SimpleTest]]: [MySQL] 56,804 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#41 drupal-1987768-41.patch9.51 KBberliner
FAILED: [[SimpleTest]]: [MySQL] 56,773 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#37 drupal-1987768-37.patch7.21 KBberliner
FAILED: [[SimpleTest]]: [MySQL] 56,491 pass(es), 13 fail(s), and 0 exception(s).
[ View ]
#34 drupal-1987768-34.patch6.64 KBberliner
FAILED: [[SimpleTest]]: [MySQL] 56,359 pass(es), 66 fail(s), and 4 exception(s).
[ View ]
#31 drupal-1987768-31.patch5.39 KBberliner
FAILED: [[SimpleTest]]: [MySQL] 55,926 pass(es), 297 fail(s), and 35 exception(s).
[ View ]
#30 drupal-1987768-30.patch3.56 KBberliner
FAILED: [[SimpleTest]]: [MySQL] 55,848 pass(es), 666 fail(s), and 44 exception(s).
[ View ]
#26 drupal-1987768-26.patch4.81 KBpguillard
FAILED: [[SimpleTest]]: [MySQL] 57,564 pass(es), 339 fail(s), and 35 exception(s).
[ View ]
#24 drupal-1987768-24.patch4.76 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,422 pass(es), 344 fail(s), and 39 exception(s).
[ View ]
#20 1987768-node_view-controller-20.patch4.78 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 57,641 pass(es), 465 fail(s), and 49 exception(s).
[ View ]
#20 1987768-diff-18-20.txt1.81 KBvijaycs85
#18 1987768-node_view-controller-18.patch4.31 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 54,222 pass(es), 1,648 fail(s), and 1,523 exception(s).
[ View ]
#18 1987768-diff-13-18.txt452 bytesvijaycs85
#13 1987768-node_view-controller-13.patch4.35 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 57,473 pass(es), 465 fail(s), and 49 exception(s).
[ View ]
#5 node-view-controller-5.patch4.25 KBxtfer
FAILED: [[SimpleTest]]: [MySQL] 57,207 pass(es), 404 fail(s), and 41 exception(s).
[ View ]
#4 node-view-controller-4.patch4.27 KBxtfer
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

Assigned:Unassigned» xtfer

Assigned:xtfer» Unassigned

Assigned:Unassigned» xtfer

Status:Active» Needs work
StatusFileSize
new4.27 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new4.25 KB
FAILED: [[SimpleTest]]: [MySQL] 57,207 pass(es), 404 fail(s), and 41 exception(s).
[ View ]

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

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

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

Status:Needs work» Needs review
StatusFileSize
new4.35 KB
FAILED: [[SimpleTest]]: [MySQL] 57,473 pass(es), 465 fail(s), and 49 exception(s).
[ View ]

Re-rolling...

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

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

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.

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

Status:Needs work» Needs review
StatusFileSize
new452 bytes
new4.31 KB
FAILED: [[SimpleTest]]: [MySQL] 54,222 pass(es), 1,648 fail(s), and 1,523 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.81 KB
new4.78 KB
FAILED: [[SimpleTest]]: [MySQL] 57,641 pass(es), 465 fail(s), and 49 exception(s).
[ View ]

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.

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.

Assigned:xtfer» Unassigned
Status:Needs work» Needs review
StatusFileSize
new4.76 KB
FAILED: [[SimpleTest]]: [MySQL] 56,422 pass(es), 344 fail(s), and 39 exception(s).
[ View ]

This was easier to fix then expected

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new4.81 KB
FAILED: [[SimpleTest]]: [MySQL] 57,564 pass(es), 339 fail(s), and 35 exception(s).
[ View ]

Re-rolled patch

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

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

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.

Status:Needs work» Needs review
StatusFileSize
new3.56 KB
FAILED: [[SimpleTest]]: [MySQL] 55,848 pass(es), 666 fail(s), and 44 exception(s).
[ View ]

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.

StatusFileSize
new5.39 KB
FAILED: [[SimpleTest]]: [MySQL] 55,926 pass(es), 297 fail(s), and 35 exception(s).
[ View ]

Forgot part of the patch.

Status:Needs review» Needs work

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

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.

StatusFileSize
new6.64 KB
FAILED: [[SimpleTest]]: [MySQL] 56,359 pass(es), 66 fail(s), and 4 exception(s).
[ View ]

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new7.21 KB
FAILED: [[SimpleTest]]: [MySQL] 56,491 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

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.

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

Priority:Normal» Major

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

Status:Needs work» Needs review
StatusFileSize
new9.51 KB
FAILED: [[SimpleTest]]: [MySQL] 56,773 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new9.8 KB
FAILED: [[SimpleTest]]: [MySQL] 56,804 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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+

StatusFileSize
new11.35 KB
FAILED: [[SimpleTest]]: [MySQL] 56,977 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

StatusFileSize
new216.41 KB

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.

StatusFileSize
new13.24 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1987768-47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs work» Needs review

Status:Needs review» Needs work

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

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

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.

Assigned:Unassigned» Crell

Claiming.

Status:Needs work» Closed (duplicate)