Committers, a few additional people need commit credit on this patch because #1987768: Convert node_page_view() to a new style controller was merged into this issue (see #62):

berliner, pguillard, vijaycs85

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
#105 node_show-1987778-105.patch11.48 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,957 pass(es).
[ View ]
#105 interdiff.txt2.71 KBdawehner
#99 1987778-99.patch10.05 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,465 pass(es).
[ View ]
#99 interdiff-1987778-99.txt742 bytesdamiankloip
#95 1987778-95.patch10.11 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,824 pass(es).
[ View ]
#95 interdiff-1987778-95.txt1.74 KBdamiankloip
#90 1987778-90.patch9.74 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,413 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#90 interdiff-1987778-90.txt696 bytesdamiankloip
#86 1987778-86.patch9.7 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,435 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#86 interdiff-1987778-86.txt6.46 KBdamiankloip
#83 1987778-node-show-85.patch6.19 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,707 pass(es).
[ View ]
#78 node-1987778-78.patch22.83 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,198 pass(es), 57 fail(s), and 1 exception(s).
[ View ]
#78 interdiff.txt5.14 KBtim.plunkett
#73 node-1987778-73.patch19.43 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,765 pass(es), 15 fail(s), and 0 exception(s).
[ View ]
#71 interdiff.txt2.84 KBtim.plunkett
#71 custom-blocks-2062439-17.patch18.39 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,703 pass(es).
[ View ]
#68 1987778-node-controller.patch20.5 KBCrell
FAILED: [[SimpleTest]]: [MySQL] 57,300 pass(es), 116 fail(s), and 9,379 exception(s).
[ View ]
#68 interdiff.txt4.61 KBCrell
#65 1987778-node-controller.patch21.68 KBCrell
FAILED: [[SimpleTest]]: [MySQL] 57,889 pass(es), 25 fail(s), and 1 exception(s).
[ View ]
#65 interdiff.txt775 bytesCrell
#62 1987778-node-controller.patch21.65 KBCrell
FAILED: [[SimpleTest]]: [MySQL] 52,927 pass(es), 393 fail(s), and 710 exception(s).
[ View ]
#62 interdiff.txt15.97 KBCrell
#57 1987778-node-show-controller-57.patch11.74 KBdokumori
PASSED: [[SimpleTest]]: [MySQL] 57,556 pass(es).
[ View ]
#54 convert-node-show-1987778-54.patch12.67 KBjessebeach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert-node-show-1987778-54.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#48 1987778-node-show-48.patch10.55 KBjessebeach
FAILED: [[SimpleTest]]: [MySQL] 56,978 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#48 interdiff_41-to-48.txt8.75 KBjessebeach
#47 1987778-node-show-47-do-not-test.patch8.77 KBscor
#45 1987778-node-show-45-do-not-test.patch9.73 KBjessebeach
#41 1987778-node-show-41.patch8.63 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,780 pass(es).
[ View ]
#41 1987778-diff-36-41.txt1.33 KBvijaycs85
#36 1987778-node-show-36.patch8.62 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,405 pass(es).
[ View ]
#36 1987778-diff-29-36.txt895 bytesvijaycs85
#32 1987778-32.patch10.4 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 58,079 pass(es), 4 fail(s), and 357 exception(s).
[ View ]
#32 interdiff.txt1.8 KBjibran
#29 stagg-commandor-4-dmx-commandor.jpg183.08 KBdawehner
#29 artikel87.jpg36.67 KBdawehner
#29 interdiff.txt750 bytesdawehner
#29 node-1987778-29.patch8.6 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,784 pass(es).
[ View ]
#25 node-1987778-25.patch8.6 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,178 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#25 interdiff.txt7.13 KBtim.plunkett
#21 node_show_controller-1987778-21.patch7.67 KBmonan
FAILED: [[SimpleTest]]: [MySQL] 58,516 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
#18 node_show_controller-1987778-18.patch7.63 KBmonan
FAILED: [[SimpleTest]]: [MySQL] 58,402 pass(es), 3 fail(s), and 6 exception(s).
[ View ]
#13 node_show_controller-1987778-13.patch8.25 KBmonan
PASSED: [[SimpleTest]]: [MySQL] 56,004 pass(es).
[ View ]
#10 node_show_controller-1987778-10.patch7.11 KBmonan
PASSED: [[SimpleTest]]: [MySQL] 56,006 pass(es).
[ View ]
#3 node_show_controller-1987778-3.patch6.96 KBmonan
FAILED: [[SimpleTest]]: [MySQL] 56,038 pass(es), 0 fail(s), and 6 exception(s).
[ View ]

Comments

Assigned:Unassigned» monan

Currently trying to work on this.

Here is a node that talks about adding a custom access controller: http://drupal.org/node/1851490

Slow going... Getting familiar with things...

Should node_show() be refactored out of the node.module and into the controller?

Status:Active» Needs review
StatusFileSize
new6.96 KB
FAILED: [[SimpleTest]]: [MySQL] 56,038 pass(es), 0 fail(s), and 6 exception(s).
[ View ]

Here is a patch for this issue. An open question: I'm getting watchdog 'non-fatal' errors on the typing of the parameters - not sure how to fix this. Tried converting in the routing yml file to no avail.

Also should be noted that this issue is blocking issue #1987768: Convert node_page_view() to a new style controller.

Guessing the controller's node_show function should be moved to a service so that we can call it from other places in the module.

Status:Needs review» Needs work

The last submitted patch, node_show_controller-1987778-3.patch, failed testing.

Status:Needs work» Needs review

I know this failed, but I don't know how to fix the issue, as I reported in my previous comment.

Any thoughts?

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,57 @@
+  public function nodeShow(NodeInterface $node, integer $node_revision) {

NodeInterface doesnt have a use statement at the top of the file so it's import fails, but it will fail anyway, as Node hasnt been converted yet. EntityInterface works, and is probably preferable at this point.

Integer isn't a valid type hint. PHP thinks you are hinting a class called "\Drupal\node\Access\integer".

Those should fix your type problems, I hope.

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,57 @@
+    $node = node_revision_load($node_revision);

Use EntityManager for this.

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,57 @@
+    $nodes = array('nodes' => node_view_multiple(array($node->nid => $node), 'full'));

And this.

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,57 @@
+    if (module_exists('history')) {

This should use:

drupal_container()->get('module_handler')->moduleExists('history');

instead.

+++ b/core/modules/node/node.routing.ymlundefined
@@ -0,0 +1,10 @@
+  defaults:

You can specify that the args should be numeric by using:

defaults:
node: \d+
node_revision: \d+

This will stop this router from triggering on paths like node/add/revisions/...

Also, to get the patches into a committable state you will need to clean up the whitespace, comments, etc.

Status:Needs review» Needs work

This should use:

drupal_container()->get('module_handler')->moduleExists('history');

instead.

I think you mean \Drupal::moduleHandler()->moduleExists('history'). Or, better yet, inject the module handler directly.

I think you mean \Drupal::moduleHandler()->moduleExists('history'). Or, better yet, inject the module handler directly.

Yes, I mean that. Thanks.

Status:Needs work» Needs review
StatusFileSize
new7.11 KB
PASSED: [[SimpleTest]]: [MySQL] 56,006 pass(es).
[ View ]

Ok, here is a fixed patch. I think issues are resolved, but we'll see what testbot says..

Thanks xtfer and rbayliss for the direction!

Status:Needs review» Needs work

Testbot seems to be struggling...

A manual test of the functionality seems to work, however the patches need a bit of a coding standards clean-up. Mostly its ensuring line's wrap at the correct width and that all the functions are documented correctly. Some of the inline comments could also be checked for accuracy, as they are a bit out of date now.

Some selective points...

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,93 @@
+<?php

Needs a @file docblock

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,93 @@
+  /**
+   * This method returns TRUE if this checker will want to control access to this route
+   * or FALSE if it doesn't want to be involved in this route.

Some methods in this class need a one-line summary of what they do as well as the description.

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,93 @@
+    $account = $GLOBALS['user'];

Apparently global $user is sufficient here, but it also seems to be duplicated further down...

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,93 @@
+    if(empty($node->type)) {

One space after "if"

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,93 @@
+?>

Rogue PHP closing tag. Remove it.

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,58 @@
+class NodeController implements ControllerInterface {

This class needs docblocks for the missing methods. They can just use {@inheritdoc} if they match the parent.

Thanks... Clearly need to brush up on my Drupal coding standards...

I'll put a final one together and submit again...

Status:Needs work» Needs review
StatusFileSize
new8.25 KB
PASSED: [[SimpleTest]]: [MySQL] 56,004 pass(es).
[ View ]

Ok, hit all the points that xtfer made, cleaned up docs, fixed line wrapping where appropriate according to coding-standards. Hopefully this will be the final final.

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,108 @@
+  /**
+   * This method returns TRUE if this checker will want to control access to this
+   * route or FALSE if it doesn't want to be involved in this route.
+   *
+   * @param \Symfony\Component\Routing\Route $route
+   *   The route to consider attaching to.
+   *
+   * @return bool
+   *   TRUE if the check applies to the passed route, FALSE otherwise.
...
+  /**
+   * Determines access to node revisions based on existence of _node_revision_access.
+   * This method is only called if applies() returned TRUE for this route.
+   *
+   * @param \Symfony\Component\Routing\Route $route
+   *   The Symfony-based route that we are checking access for.
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The Symfony-based request that we received.
+   *
+   * @return bool|null
+   *   TRUE if access was granted, FALSE if access was denied or NULL if this
+   *   module has no input about whether or not access should be granted.

Both long function blocks could just use {@inheritdoc} so you don't have to copy all of the docs.

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,108 @@
\ No newline at end of file

Please add a newline at the end of the file.

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,78 @@
+ * Definition of Drupal\node\Controller\NodeController.

This should be "Contains \Drupal\..."

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,78 @@
+    if (\Drupal::moduleHandler()->moduleExists('history')) {

Let's also inject moduleExists in there.

Status:Needs review» Needs work

Hi dawehner, can you clarify what you mean by injecting moduleExists?

Are you saying to do it similarly to how the entity manager is getting passed into the constructor?

So it would end up being something like this:

protected $entityManager;
protected $moduleExistsMethod;

public function __construct(EntityManager $entity_manager, $moduleExists) {
$this->entityManager = $entity_manager;
$this->moduleExistsMethod = $moduleExists;
}

... and later in nodeShow ...

// Update the history table, stating that this user viewed this node.
if (call_user_func($this->moduleExistsMethod, 'history')) {
history_write($node->nid);
}

I'm trying to use http://symfony.com/doc/current/components/dependency_injection/introduct... as a guide...

Is there a better intro?

Thanks!

I think that's pretty much it, except that you'd be injecting the whole module handler. So :

<?php
public function __construct(EntityManager $entity_manager, ModuleHandler $moduleHandler) {
 
$this->entityManager = $entity_manager;
 
$this->moduleHandler = $moduleHandler;
}
...
// Update the history table, stating that this user viewed this node.
if ($this->moduleHandler->moduleExists('history')) {
 
history_write($node->nid);
}
?>

Status:Needs work» Needs review
StatusFileSize
new7.63 KB
FAILED: [[SimpleTest]]: [MySQL] 58,402 pass(es), 3 fail(s), and 6 exception(s).
[ View ]

Great, thanks rbayliss...

Ok, let's try again. Incorporated all suggested changes so far.

Fingers crossed...

Thanks everyone!

Status:Needs review» Needs work

The last submitted patch, node_show_controller-1987778-18.patch, failed testing.

Almost there.

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,86 @@
+  public static function create(ContainerInterface $container) {
+    return new static(
+      $container->get('plugin.manager.entity')
+    );

This is where the moduleHandler should get pulled out of the container and injected into the controller. So the second argument should be $container->get('module_handler')

Status:Needs work» Needs review
StatusFileSize
new7.67 KB
FAILED: [[SimpleTest]]: [MySQL] 58,516 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

Doh... My bad...

Let's try that one last time...

Thanks!

Status:Needs review» Needs work

The last submitted patch, node_show_controller-1987778-21.patch, failed testing.

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,91 @@
+      $num_revisions = db_query('SELECT COUNT(vid) FROM {node_field_revision} WHERE nid = :nid', array(':nid' => $node->nid))->fetchField();

Let's inject the database connection here.

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,87 @@
+   * @var \Drupal\Core\Extension\ModuleHandler
...
+  public function __construct(EntityManager $entity_manager, ModuleHandler $module_handler) {

The module handler should be documented as ModuleHandlerInterface

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,87 @@
+   * @param \Drupal\Core\Entity\EntityInterface $node
...
+  public function nodeShow(EntityInterface $node, $node_revision) {

What about type hinting for NodeInterface instead?

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,87 @@
+      array('%title' => $node->label(), '%date' => format_date($node->revision_timestamp))),

format_date is a service now, so it can be injected.

Unless something has changed in the last week, NodeInterface doesn't actually work for the Entity yet, since the node is still using the NG shim.

Status:Needs work» Needs review
StatusFileSize
new7.13 KB
new8.6 KB
FAILED: [[SimpleTest]]: [MySQL] 58,178 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Yep, the NodeBCDecorator went in so we can use NodeInterface now.

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

The last submitted patch, node-1987778-25.patch, failed testing.

Status:Needs work» Needs review

#25: node-1987778-25.patch queued for re-testing.

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

The last submitted patch, node-1987778-25.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.6 KB
PASSED: [[SimpleTest]]: [MySQL] 56,784 pass(es).
[ View ]
new750 bytes
new36.67 KB
new183.08 KB

According to google this is a conroller:

stagg-commandor-4-dmx-commandor.jpg

but I guess we want to have something like that:
artikel87.jpg

Status:Needs review» Reviewed & tested by the community

LOL @dawehner, best review ever.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.8 KB
new10.4 KB
FAILED: [[SimpleTest]]: [MySQL] 58,079 pass(es), 4 fail(s), and 357 exception(s).
[ View ]

Sorry for hijacking the issue. Let's see people like the idea in #2026081-1: Clean up ForumBreadcrumbBuilder::build().

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.phpundefined
@@ -47,28 +47,21 @@ public function __construct(EntityManager $entity_manager, ConfigFactory $config
+    if ($attributes['_route'] == 'node_show') {

I don't really like it, that it is now hardcoded to a route. Just imagine what happens if you override the path with a panel (or however it will be called in drupal 8). The route name will most probably change. maybe it is better to use something like $route->attributes->get('system_path')

Status:Needs review» Needs work

The last submitted patch, 1987778-32.patch, failed testing.

I think we should name the route different as this is about the revision somehow not simple /node/%node

Status:Needs work» Needs review
StatusFileSize
new895 bytes
new8.62 KB
PASSED: [[SimpleTest]]: [MySQL] 58,405 pass(es).
[ View ]

dawehner++

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,110 @@
+      $num_revisions = $this->connection->query('SELECT COUNT(vid) FROM {node_field_revision} WHERE nid = :nid', array(':nid' => $node->nid))->fetchField();

On the long run this should be certainly replaced by a entity count query, so what about a follow up?

+++ b/core/modules/node/node.routing.ymlundefined
@@ -1,3 +1,13 @@
+    _content: '\Drupal\node\Controller\NodeController::nodeShow'

Let's change the function name as well.

Not sure we need to change the method name as it is just render node.

I think it would be best if it were renamed nodeRevisionView() to match.

StatusFileSize
new1.33 KB
new8.63 KB
PASSED: [[SimpleTest]]: [MySQL] 58,780 pass(es).
[ View ]

ok, here is update...

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,110 @@
+ * Access check for user registration routes.

This documentation is wrong.

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,110 @@
+class NodeAccessCheck implements AccessCheckInterface {

This name is bad, as we are dealing with just the revision check.

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,110 @@
+        $access[$cid] = node_access($op, node_load($node->nid), $account, $langcode) && ($node->isDefaultRevision() || node_access($op, $node, $account, $langcode));

let's inject the entity manager in order to load the node.

StatusFileSize
new9.73 KB

Rerolled because of a conflict with node.services.yml.

Status:Needs work» Needs review

StatusFileSize
new8.77 KB

rerolling after #2039199 was committed.

StatusFileSize
new8.75 KB
new10.55 KB
FAILED: [[SimpleTest]]: [MySQL] 56,978 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

I address @dawehner three comments in #41 and Crell in chat.

  1. Documentation is updated
  2. The class is renamed from NodeAccessCheck to NodeRevisionAccessCheck.
  3. The node_load() is removed and replaced with an injected $entityManager, from which I grab the node storage controller and use to load the node from its ID.
  4. $GLOBALS['user'] is replaced with $request->attributes->get('account').

The crux of the replacement node load code is below. I'm not 100% sure this is best practice. It's the best I could surmise from existing examples.

$access[$cid] = node_access($op, node_load($node->nid), $account, $langcode) && ($node->isDefaultRevision() || node_access($op, $node, $account, $langcode));

to this

$storage_controller = $this->entityManager->getStorageController('node');
$access[$cid] = node_access($op, $storage_controller->load($node->nid), $account, $langcode) && ($node->isDefaultRevision() || node_access($op, $node, $account, $langcode));

The $entityManager is passed into the NodeRevisionAccessCheck as a service. I must admit, the logic here has me a bit confused. The comment could be improved by someone who understands what is happening a little better than I.

Also, we ran into an issue with $request->attributes->get('account'). It doesn't consistently return an object. This issue is being addressed in #2040065: Remove _account from request and use the current user service instead.. Until then I left this @todo in the code.

// @todo, when https://drupal.org/node/2040065 is committed, remove this
// return null statement and allow an Exception to be thrown if the
// account object is not defined.
return null;
throw new \Exception('Account object is not set.');

My thinking is that the access check should throw an error for now. Ideally the account object will always be included in the request, so when it is not, we want an error to alert us to this, rather than simply returning false and moving on. If my assumption is wrong; if we can return false when the account object is null without hiding a deeper error, then we can remove the throw and just return false.

tim.plunkett, we should just focus on #1946434: Convert all of confirm_form() in node.admin.inc and node.pages.inc to the new form interface then. It seems to be a superset of functionality proposed in this issue here.

Status:Needs review» Needs work

The last submitted patch, 1987778-node-show-48.patch, failed testing.

Status:Needs work» Needs review

#1946434: Convert all of confirm_form() in node.admin.inc and node.pages.inc to the new form interface was committed. I'm rerolling this issue to remove the duplicated code.

I'm trying to pull apart the dependencies in these two very similar, but differently structured Classes: NodeAccessController and NodeRevisionAccessCheck.

NodeAccessController
  ex. EntityAccessController
    im.  EntityAccessControllerInterface
      mth. access(EntityInterface, $operation, $langcode, AccountInterface)
      mth. resetCache()
  im. NodeAccessControllerInterface
      mth. acquireGrants(NodeInterface);
      mth. writeGrants(NodeInterface, $delete);
      mth. writeDefaultGrant();
      mth. deleteGrants();
      mth. countGrants();
      mth. checkAllGrants(AccountInterface);
  im. EntityControllerInterface
      mth. createInstance(ContainerInterface, $entity_type, $entity_info)
  mth. access(EntityInterface, $operation, $langcode, AccountInterface)
  mth. checkAccess(EntityInterface, $operation, $langcode, AccountInterface)
  mth. getCache(EntityInterface, $operation, $langcode, AccountInterface)
  mth. setCache($access, EntityInterface, $operation, $langcode, AccountInterface)
  mth. resetCache()
NodeRevisionAccessCheck
  im. AccessCheckInterface
    mth. applies(Route)
    mth. access(Route, Request)
  mth. applies(Route)
  mth. access(Route, Request)
  mth. checkAccess(NodeInterface, $op, AccountInterface, $langcode)

StatusFileSize
new12.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert-node-show-1987778-54.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I've removed the addition of NodeRevisionAccessCheck from the patch. It was added in #1946434: Convert all of confirm_form() in node.admin.inc and node.pages.inc to the new form interface.

If you read comment #53, you'll see a very subtle inconsistency:

NodeAccessController::access(EntityInterface, $operation, $langcode, AccountInterface)
NodeRevisionAccessCheck::checkAccess(NodeInterface, $op, AccountInterface, $langcode)

The signatures of these methods have an inverted order for $langcode and AccountInterface. I've updated NodeRevisionAccessCheck::checkAccess to match the code found in other parts of core.

NodeRevisionAccessCheck::checkAccess(NodeInterface, $op, $langcode, AccountInterface)

This required changing a few lines in tests:

- $this->assertTrue(_node_revision_access($revision, $case['op'], $case['account']), "{$this->map[$case['op']]} granted.");
+ $this->assertTrue(_node_revision_access($revision, $case['op'], $revision->language()->id, $case['account']), "{$this->map[$case['op']]} granted.");

I replaced instances of _access_node_revision with _node_revision_access in order to remove inconsistencies.

- _access_node_revision: 'update'
+ _node_revision_access: 'update'

I did not remove the reference to $GLOBALS['user']. Getting the account from the request just flat out doesn't work yet, so I left a comment.

// @todo, remove this check and the reference to $GLOBALS['user'] once
// https://drupal.org/node/2040065 is committed.
if (!isset($account)) {
  $account = $GLOBALS['user'];
}

We are now at least passing the account from the request into the checkAccess function, so once #2040065 is committed we can attempt to remove the if statement indicated by the comment.

-    return $this->checkAccess($revision, $route->getRequirement('_access_node_revision')) ? static::ALLOW : static::DENY;
+    return $this->checkAccess($revision, $route->getRequirement('_node_revision_access'), $revision->language()->id, $request->attributes->get('account')) ? static::ALLOW : static::DENY;

And very oddly I had to make the following change:

class NodeRevisionsAllTestCase extends NodeTestBase {
   protected $nodes;
   protected $logs;
-  protected $profile = "standard";

to remove the $profile variable from the NodeRevisionsAllTestCase because if the profile is declared, then NodeTestBase won't set up the default Page and Article content types, so the entire test fails to set up properly. I'm not sure how this is working in HEAD right now :/ But it was blowing up locally for me. Removing that line causes the test suite to run and everything passes.

Issue tags:-WSCCI-conversion

#54: convert-node-show-1987778-54.patch queued for re-testing.

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

The last submitted patch, convert-node-show-1987778-54.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+LONDON_2013_JULY
StatusFileSize
new11.74 KB
PASSED: [[SimpleTest]]: [MySQL] 57,556 pass(es).
[ View ]

Rerolling the patch

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

The last submitted patch, 1987778-node-show-controller-57.patch, failed testing.

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

+++ b/core/modules/node/node.routing.ymlundefined
@@ -19,19 +19,30 @@ node_delete_confirm:
-    _access_node_revision: 'update'
+    _node_revision_access: 'update'

I don't understand this change.

_access_aggregator_categories
_access_edit_entity_field
_access_edit_entity
_access_menu_delete_menu
_access_overlay_dismiss_message
_access_rest_csrf
_access_shortcut_link_delete
_access_system_cron
_access_taxonomy_term_create
_access_user_register

etc...

Status:Needs review» Needs work

It should also use appliesTo() now, but without the rename, we no longer touch that method, so probably a separate issue.

Assigned:monan» Crell
Priority:Normal» Major
Status:Needs work» Needs review
StatusFileSize
new15.97 KB
new21.65 KB
FAILED: [[SimpleTest]]: [MySQL] 52,927 pass(es), 393 fail(s), and 710 exception(s).
[ View ]

I've decided it makes sense to merge this issue with #1987768: Convert node_page_view() to a new style controller. The attached patch contains code from both issues, plus some updates.

Committers: Please be sure to also credit berliner, pguillard, and vijaycs85 from the other issue.

It may still fail tests, but let's see.

Also bumping to major as this blocks #2019123: Use the same canonical URI paths as for HTML routes.

Interdiff is against #57, and is mostly the result of merging in the other issue.

Title:Convert node_show() to a new style controllerConvert node_show() and node_page_view() to a new style controller

Retitling.

Status:Needs review» Needs work

The last submitted patch, 1987778-node-controller.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new775 bytes
new21.68 KB
FAILED: [[SimpleTest]]: [MySQL] 57,889 pass(es), 25 fail(s), and 1 exception(s).
[ View ]

This should fix all of those notices.

I don't know about some of the fatals. Unfortunately my local test environment seems to be acting up and giving me different errors than what's on testbot. Possibly due to me running PHP 5.4. If someone wants to take a crack at it I'd much appreciate it. :-)

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -280,7 +280,7 @@ public function access($operation = 'view', AccountInterface $account = NULL) {
-      ->access($this, $operation, Language::LANGCODE_DEFAULT, $account);
+      ->access($this, $operation, $this->language()->id, $account);
+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -420,7 +420,7 @@ public function access($operation = 'view', AccountInterface $account = NULL) {
-      ->access($this, $operation, $this->activeLangcode, $account);
+      ->access($this, $operation, $this->language()->id, $account);

How is this related?

+++ b/core/modules/node/lib/Drupal/node/Access/NodeRevisionAccessCheck.phpundefined
@@ -66,7 +66,7 @@ public function __construct(EntityManager $entity_manager, Connection $connectio
-    return array_key_exists('_access_node_revision', $route->getRequirements());
+    return array_key_exists('_node_revision_access', $route->getRequirements());

Out of scope. Also backwards. I left a big long review on this either here or on the other issue you merged in.

+++ b/core/modules/node/lib/Drupal/node/NodeBreadcrumbBuilder.phpundefined
@@ -0,0 +1,55 @@
+  public function __construct(TranslationManager $translation) {
+    $this->translation = $translation;

Needs docblock. Also, this has been either $this->translationManager in 80% of core, or $this->translator in the other 20%, but not $this->translation

+++ b/core/modules/node/lib/Drupal/node/NodeBreadcrumbBuilder.phpundefined
@@ -0,0 +1,55 @@
+    // @todo This only works for legacy routes. Once node/% has been converted
+    //   to the new router this code will need to be updated.

Isn't this converting node/%?

+++ b/core/modules/node/node.routing.ymlundefined
@@ -19,19 +19,40 @@ node_delete_confirm:
+  options:
+    defaults:
+      node: \d+
+      node_revision: \d+
...
+  options:
+    defaults:
+      node: \d+

Isn't that a thing for under requirements?

Status:Needs review» Needs work

The last submitted patch, 1987778-node-controller.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.61 KB
new20.5 KB
FAILED: [[SimpleTest]]: [MySQL] 57,300 pass(es), 116 fail(s), and 9,379 exception(s).
[ View ]

How is this related?

I'm not sure, frankly. It was in the other dupe issue so when I merged them I kept it in. I'm going to assume it's necessary or at least OK to include unless someone pushes back on it or we find it's responsible for some errors.

Attached patch fixes the other issues Tim mentioned. I tried a few of the tests that testbot didn't like, but couldn't replicate. Testing again. I'm really getting sick of testbot and my computer disagreeing.

Status:Needs review» Needs work

The last submitted patch, 1987778-node-controller.patch, failed testing.

Assigned:Crell» Unassigned

OK, that makes no sense. Someone with 5.3 please lend me a hand here. :-/

Status:Needs work» Needs review
StatusFileSize
new18.39 KB
PASSED: [[SimpleTest]]: [MySQL] 57,703 pass(es).
[ View ]
new2.84 KB

Pretty sure this has nothing to do with 5.3, unless array keys work different in the future ;)

WTF? Then why did those tests pass for me? *sigh* Well, someone needs to review/RTBC this now. :-) Thanks Tim.

Scratch that. I really need to pay more attention. That's not even the right patch for this issue. *sigh*

StatusFileSize
new19.43 KB
FAILED: [[SimpleTest]]: [MySQL] 57,765 pass(es), 15 fail(s), and 0 exception(s).
[ View ]

Wrong patch, correct interdiff. My apologies.

Status:Needs review» Needs work

The last submitted patch, node-1987778-73.patch, failed testing.

@@ -19,6 +19,23 @@ node_delete_confirm:
+  requirements:
+    _access_node_revision: 'view'
+    node: \d+
+    node_revision: \d+
...
+  requirements:
+    _permission: 'access content'
+    node: \d+

This either needs _access_mode: 'ALL' or should wait for #2063401: Replace the default _access_checks(access mode) with ALL instead of ANY

Why? The regex checks are not handled by the access control system. They're part of the UrlMatcher and would result in a 404, not 403.

Okay, well it seemed to be an easy explanation by those fails. I leave it to someone else to debug then.

Status:Needs work» Needs review
StatusFileSize
new5.14 KB
new22.83 KB
FAILED: [[SimpleTest]]: [MySQL] 58,198 pass(es), 57 fail(s), and 1 exception(s).
[ View ]

Here's some fixes.

Status:Needs review» Needs work

The last submitted patch, node-1987778-78.patch, failed testing.

Issue tags:+Needs reroll

It needs reroll.

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

Please also clean \Drupal\comment\Form\DeleteForm in next reroll.

Status:Needs work» Needs review
StatusFileSize
new6.19 KB
PASSED: [[SimpleTest]]: [MySQL] 58,707 pass(es).
[ View ]

This is pretty much a rewrite after #2089635: Convert non-test non-form page callbacks to routes and controllers went in.

It only focusses on node_show() and node_page_view() and doesn't touch breadcrumbs or anything else. I moved the methods in \Drupal\node\Controller\NodeView back into \Drupal\node\Controller\NodeController as they were pretty closely tied together.

Issue tags:-Needs reroll

Doesn't need a reroll anymore

  1. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
    @@ -32,11 +34,34 @@ public function add(EntityInterface $node_type) {
    -    return node_show($node_revision, TRUE);
    +++ /dev/null
    @@ -1,35 +0,0 @@
    -    return node_page_view($node);

    Is there a reason why these two functions are not removed in this issue?

  2. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
    @@ -47,4 +72,81 @@ public function revisionOverview(NodeInterface $node) {
    +            'data' => 'window.addEventListener("load",function(){Drupal.history.markAsRead(' . $node->id() . ');},false);',

    Wow I would have not thought that this went throw an actual review given that inline JS seems really wrong.

StatusFileSize
new6.46 KB
new9.7 KB
FAILED: [[SimpleTest]]: [MySQL] 58,435 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Good points, I think maybe that inline js should be tackled in another issue. It definitely seems crappy to me :)

Looking and the buildPage method; it seems like node should not be caring about this, and history module should be altering the node build array instead.

Also, removed those functions.

Status:Needs review» Needs work

The last submitted patch, 1987778-86.patch, failed testing.

+++ b/core/modules/history/history.module
@@ -129,6 +130,30 @@ function history_cron() {
/**
+ * Implements hook_node_view_alter().
+ */
+function history_node_view_alter(&$build, EntityInterface $node, EntityDisplay $display) {
+  // Update the history table, stating that this user viewed this node.

hook_node_view_alter() can be invoked from places other than hitting /node/{node}; I think it's called for any teaser or other view mode, too. This is going to end up over-reporting a user's hits.

Yeah, like any view using teasers of a node.

It could at least check the view mode (for 'full'), or check the route name?

StatusFileSize
new696 bytes
new9.74 KB
FAILED: [[SimpleTest]]: [MySQL] 58,413 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Ha, you guys are totally right. I didn't think about that, but then I didn't really spend a lot of time on that patch :p

Status:Needs work» Reviewed & tested by the community

I prefer to use the display mode only as you could see the full display in various places which then should count it.

It would be great to add a @todo to remove this uglyness.

Status:Reviewed & tested by the community» Needs review

Ups,

Status:Needs review» Needs work

The last submitted patch, 1987778-90.patch, failed testing.

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
@@ -32,11 +34,34 @@ public function add(EntityInterface $node_type) {
+    $page = $this->buildPage($node);

You removed build page, but we're still calling it here.

Status:Needs work» Needs review
StatusFileSize
new1.74 KB
new10.11 KB
PASSED: [[SimpleTest]]: [MySQL] 58,824 pass(es).
[ View ]

Rerolled and added that todo.

Kim, you're totally right, that would be why the revision tests are failing I guess :) I added back in the buildPage() method, sorry for removing that...

#85 & #86: The inline JS got through extensive reviews at #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user — please read that issue before jumping to conclusions. That being said, obviously I agree that it's ugly. But at least now node views are render cacheable ;) Until we have something like a #post_render_cache callback, this is the only way we can fix it. See #2073535: Node history markers require an HTTP request .

Thanks for the heads up Wim. I'm not going to read that issue though, I will just take your word for it.. I believe you on these matters :)

+++ b/core/modules/history/history.module
@@ -129,6 +130,31 @@ function history_cron() {
+        // @todo Move this functionality into a js file.

I guess we should remove the @todo then.

Looking through, I didn't see anything else to complain about. Yay for code finally working! :-)

StatusFileSize
new742 bytes
new10.05 KB
PASSED: [[SimpleTest]]: [MySQL] 58,465 pass(es).
[ View ]

Totally.

Status:Needs review» Reviewed & tested by the community

Great, let's try to continue on the other few ones.

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
@@ -55,4 +80,60 @@ public function revisionOverview(NodeInterface $node) {
+      drupal_add_html_head_link(array(
+        'rel' => $rel,
+        'href' => $this->urlGenerator()->generateFromPath($uri['path'], $uri['options']),
+      ), TRUE);

Couldn't this use #attached?

Otherwise seems OK.

Status:Reviewed & tested by the community» Needs review

Status:Needs review» Reviewed & tested by the community

No? We could change drupal_process_attached to support more than just 'library', 'css', or 'js', but I think that's out of scope.

Status:Reviewed & tested by the community» Needs review

It already does:

// Add additional types of attachments specified in the render() structure.
  // Libraries, JavaScript and CSS have been added already, as they require
  // special handling.
  foreach ($elements['#attached'] as $callback => $options) {
    if (function_exists($callback)) {
      foreach ($elements['#attached'][$callback] as $args) {
        call_user_func_array($callback, $args);
      }
    }
  }

StatusFileSize
new2.71 KB
new11.48 KB
PASSED: [[SimpleTest]]: [MySQL] 58,957 pass(es).
[ View ]

Let's do that and test that it works.

Status:Needs review» Reviewed & tested by the community

Looks good. Back to RTBC.

Status:Reviewed & tested by the community» Fixed

Thanks! Committed/pushed to 8.x.

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes

Add note to committers for commit credits