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.

CommentFileSizeAuthor
#105 node_show-1987778-105.patch11.48 KBdawehner
#105 interdiff.txt2.71 KBdawehner
#99 1987778-99.patch10.05 KBdamiankloip
#99 interdiff-1987778-99.txt742 bytesdamiankloip
#95 1987778-95.patch10.11 KBdamiankloip
#95 interdiff-1987778-95.txt1.74 KBdamiankloip
#90 1987778-90.patch9.74 KBdamiankloip
#90 interdiff-1987778-90.txt696 bytesdamiankloip
#86 1987778-86.patch9.7 KBdamiankloip
#86 interdiff-1987778-86.txt6.46 KBdamiankloip
#83 1987778-node-show-85.patch6.19 KBkim.pepper
#78 node-1987778-78.patch22.83 KBtim.plunkett
#78 interdiff.txt5.14 KBtim.plunkett
#73 node-1987778-73.patch19.43 KBtim.plunkett
#71 interdiff.txt2.84 KBtim.plunkett
#71 custom-blocks-2062439-17.patch18.39 KBtim.plunkett
#68 1987778-node-controller.patch20.5 KBCrell
#68 interdiff.txt4.61 KBCrell
#65 1987778-node-controller.patch21.68 KBCrell
#65 interdiff.txt775 bytesCrell
#62 1987778-node-controller.patch21.65 KBCrell
#62 interdiff.txt15.97 KBCrell
#57 1987778-node-show-controller-57.patch11.74 KBdokumori
#54 convert-node-show-1987778-54.patch12.67 KBjessebeach
#48 1987778-node-show-48.patch10.55 KBjessebeach
#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
#41 1987778-diff-36-41.txt1.33 KBvijaycs85
#36 1987778-node-show-36.patch8.62 KBvijaycs85
#36 1987778-diff-29-36.txt895 bytesvijaycs85
#32 1987778-32.patch10.4 KBjibran
#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
#25 node-1987778-25.patch8.6 KBtim.plunkett
#25 interdiff.txt7.13 KBtim.plunkett
#21 node_show_controller-1987778-21.patch7.67 KBmonan
#18 node_show_controller-1987778-18.patch7.63 KBmonan
#13 node_show_controller-1987778-13.patch8.25 KBmonan
#10 node_show_controller-1987778-10.patch7.11 KBmonan
#3 node_show_controller-1987778-3.patch6.96 KBmonan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

monan’s picture

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

monan’s picture

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

monan’s picture

Status: Active » Needs review
FileSize
6.96 KB

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.

monan’s picture

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?

xtfer’s picture

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

xtfer’s picture

Status: Needs review » Needs work
rbayliss’s picture

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.

xtfer’s picture

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

Yes, I mean that. Thanks.

monan’s picture

Status: Needs work » Needs review
FileSize
7.11 KB

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!

xtfer’s picture

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.

monan’s picture

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

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

monan’s picture

Status: Needs work » Needs review
FileSize
8.25 KB

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.

dawehner’s picture

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

xtfer’s picture

Status: Needs review » Needs work
monan’s picture

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!

rbayliss’s picture

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


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);
} 
monan’s picture

Status: Needs work » Needs review
FileSize
7.63 KB

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.

rbayliss’s picture

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')

monan’s picture

Status: Needs work » Needs review
FileSize
7.67 KB

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.

dawehner’s picture

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

xtfer’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
7.13 KB
8.6 KB

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.

tim.plunkett’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.6 KB
750 bytes
36.67 KB
183.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

jibran’s picture

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

LOL @dawehner, best review ever.

jibran’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.8 KB
10.4 KB

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

dawehner’s picture

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

dawehner’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
895 bytes
8.62 KB

dawehner++

dawehner’s picture

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

jibran’s picture

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

vijaycs85’s picture

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

tim.plunkett’s picture

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

vijaycs85’s picture

ok, here is update...

jibran’s picture

jibran’s picture

Status: Needs review » Reviewed & tested by the community
dawehner’s picture

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.

jessebeach’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review
scor’s picture

rerolling after #2039199 was committed.

jessebeach’s picture

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

jessebeach’s picture

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.

jessebeach’s picture

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.

jessebeach’s picture

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)
jessebeach’s picture

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.

tim.plunkett’s picture

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.

dokumori’s picture

Status: Needs work » Needs review
Issue tags: +LONDON_2013_JULY
FileSize
11.74 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion, +LONDON_2013_JULY
tim.plunkett’s picture

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

Berdir’s picture

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.

Crell’s picture

Assigned: monan » Crell
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
15.97 KB
21.65 KB

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.

Crell’s picture

Title: Convert node_show() to a new style controller » Convert 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.

Crell’s picture

Status: Needs work » Needs review
FileSize
775 bytes
21.68 KB

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

tim.plunkett’s picture

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

Crell’s picture

Status: Needs work » Needs review
FileSize
4.61 KB
20.5 KB

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.

Crell’s picture

Assigned: Crell » Unassigned

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.39 KB
2.84 KB

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

Crell’s picture

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*

tim.plunkett’s picture

FileSize
19.43 KB

Wrong patch, correct interdiff. My apologies.

Status: Needs review » Needs work

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

tim.plunkett’s picture

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

Crell’s picture

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.

tim.plunkett’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.14 KB
22.83 KB

Here's some fixes.

Status: Needs review » Needs work

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

jibran’s picture

Issue tags: +Needs reroll

It needs reroll.

xjm’s picture

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.

jibran’s picture

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
6.19 KB

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.

kim.pepper’s picture

Issue tags: -Needs reroll

Doesn't need a reroll anymore

dawehner’s picture

  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.

damiankloip’s picture

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.

Crell’s picture

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

tim.plunkett’s picture

Yeah, like any view using teasers of a node.

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

damiankloip’s picture

FileSize
696 bytes
9.74 KB

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

dawehner’s picture

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.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review

Ups,

Status: Needs review » Needs work

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

kim.pepper’s picture

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

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.74 KB
10.11 KB

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

Wim Leers’s picture

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

damiankloip’s picture

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 :)

Crell’s picture

+++ 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! :-)

damiankloip’s picture

FileSize
742 bytes
10.05 KB

Totally.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

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

catch’s picture

Status: Reviewed & tested by the community » Needs review
tim.plunkett’s picture

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.

catch’s picture

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);
      }
    }
  }
dawehner’s picture

FileSize
2.71 KB
11.48 KB

Let's do that and test that it works.

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed/pushed to 8.x.

andypost’s picture

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

Anonymous’s picture

Issue summary: View changes

Add note to committers for commit credits

andypost’s picture