Replace node_access() by $entity->access()

Yes, i'm interested if you can provide me an example or a change notice of how to do this change. I don't really know what i'm doing. :)

Sure, sorry for not providing that already.

Basically, look for calls like "node_access($node, 'view')" and replace them with "$node->access('view')". There will quite a few of those and some might be more complicated. Just start with what you can do and we'll look into the more complicated cases later.

See http://drupal.org/node/1862420 for the change notice.

a babystep to see if i'm doing things right.

Forgot a parentheses the last patch will fail.

Yes, that looks good ( I had the argument order switched in my example, sorry). Now we just need a lot more of those :)

Patch includes a bunch of low hanging fruit and simple instances. More complex cases to come. Have at it bot.

@fubhy @Berdir: This maybe a silly question but where can i find the $node->access method definition? I did a couple greps and google's and didn't have any luck. I'm probably missing something really simple. I'm trying to see what arguments it accepts.

You can see the definition here: http://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21TypedData%...

The implementation(s) are linked there as well.

What we are currently missing and will be required for some of the conversions here is the language code, which we should probably add in a separate issue.

Or maybe instead of adding the language code, we'll do something like $entity->getTranslation('de')->access() but that too, might need to be correctly implemented.

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityFieldAccessCheck.phpundefined
@@ -44,7 +44,7 @@ public function accessEditEntityField(EntityInterface $entity, $field_name) {
     $entity_type = $entity->entityType();
     // @todo Generalize to all entity types once http://drupal.org/node/1862750
     // is done.
-    return ($entity_type == 'node' && node_access('update', $entity) && field_access('edit', $field_name, $entity_type, $entity));
+    return ($entity_type == 'node' && $node->access('update') && field_access('edit', $field_name, $entity_type, $entity));

Here you can actually remove the $entity_type check as ->access() works for all entities, unlike node_access(). Note that the $entity_type variable is still required as it's passed to field_access()

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceAutocomplete.phpundefined
@@ -68,7 +68,8 @@ public function getMatches($field, $instance, $entity_type, $entity_id = '', $pr
       // @todo: Improve when we have entity_access().
-      $entity_access = $target_type == 'node' ? node_access('view', $entity) : TRUE;
+      // @todo revisit later
+      $entity_access = $target_type == 'node' ? $node->access('view', $entity) : TRUE;
       if (!$entity || !$entity_access) {

You should probably ignore this one as it is already being converted in another issue.

+++ b/core/modules/file/file.api.phpundefined
@@ -223,6 +223,7 @@ function hook_file_delete(Drupal\file\File $file) {
   if ($entity->entityType() == 'node') {
+    // @todo revisit later
     return node_access('view', $entity);

Same here, I'm removing the whole hook now that we have $entity->access() in a different issue.

+++ b/core/modules/forum/forum.moduleundefined
@@ -175,6 +175,7 @@ function forum_menu_local_tasks(&$data, $router_item, $root_path) {
+        // @todo revisit later
         if (node_access('create', $type)) {

This one is a bit tricky because we don't have a node yet. So what you need to do is $node = entity_create('node', array('type' => $type)) and then call access on $node. Which is a bit unfortunate here as we are looping over possibly quite a few types and entity_create() is quite costly. But node_access() is already doing that internally, so it doesn't matter.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
index a2b8b63..e9f7bec 100644
--- a/core/modules/node/lib/Drupal/node/Plugin/views/argument_validator/Node.php

--- a/core/modules/node/lib/Drupal/node/Plugin/views/argument_validator/Node.php
+++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument_validator/Node.phpundefined

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument_validator/Node.phpundefined
+++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument_validator/Node.phpundefined
@@ -94,7 +94,7 @@ function validate_argument($argument) {

@@ -94,7 +94,7 @@ function validate_argument($argument) {
         }
 
         if (!empty($this->options['access'])) {
-          if (!node_access($this->options['access_op'], $node)) {
+          if (!$node->access($this->options['access_op'])) {

Looks like this one could maybe be generalized into an entity argument access validator or something like that but that's a separate issue, this is good enough for now.

+++ b/core/modules/node/node.moduleundefined
@@ -3542,6 +3542,7 @@ function node_modules_disabled($modules) {
 function node_file_download_access($field, EntityInterface $entity, File $file) {
   if ($entity->entityType() == 'node') {
+    // @todo revisit later
     return node_access('view', $entity);

Same as above, just remove the @todo and ignore for now.

+++ b/core/modules/node/node.pages.incundefined
@@ -43,6 +43,7 @@ function node_add_page() {
+    // @todo Revisit later
     if (node_hook($type->type, 'form') && node_access('create', $type->type)) {

As in the other example, entity_create() first. Which here, is really quite bad. @fubhy, ideas?

Looks like it didn't like my changes in the book.module file. Maybe i'll revert that and a couple other instances and see where i'm at. But i have to do that a bit later.

Thanks for the feedback @berdir and @fuhby it's been most helpful. I'll get those changes in the next patch. Probably separately. I'm going to divide and conquer and see which ones are causing the errors and hopefully get back to green.

That looks like a Node NG issue and not something that's wrong in your patch.

What's happening is that before, we passed a decorated node object that still allows old-style access to something like $node->uid but now it goes through the decorator and passed the NodeNG object into the access controller and it seems we have a case where we call something like $node->uid or $node->status and got an integer before and now a Field object. I'll look into it.

This should take care of those fails, or most of them and allow you to get back to this if you're interested (or someone else!). Didn't fix most of my review above except removing the conflicting part in EntityReferenceAutocomplete.php that didn't apply anymore.

Sounds good. I'll get back at it once we get back to green or if you can help me figure out how to get it back to green?

This should fix the book tests, the views ones were a HEAD failure unrelated to this patch.

implemented all the feedback from #13. Let me know if i missed anything. Let's hope this goes green!

Ran a grep to find all the other instances:

% grep -nr "node_access(" .                                                                                                    

./core/modules/book/lib/Drupal/book/Plugin/block/block/BookNavigationBlock.php:84:          $book['access'] = node_access('view', $book_node);
./core/modules/book/book.module:103:      if ((user_access('add content to books') || user_access('administer book outlines')) && node_access('create', $child_type) && $node->status == 1 && $node->book['depth'] < MENU_MAX_DEPTH) {
./core/modules/file/file.api.php:226:    return node_access('view', $entity);
./core/modules/node/node.api.php:647:function hook_node_access($node, $op, $account, $langcode) {
./core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.php:13: * Verifies node_access() functionality for multiple languages.
./core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.php:53:   * Tests node_access() with multiple node languages and no private nodes.
./core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.php:95:    // Reset the node access cache and turn on our test node_access() code.
./core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.php:110:   * Tests node_access() with multiple node languages and private nodes.
./core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.php:154:    // Reset the node access cache and turn on our test node_access() code.
./core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareCombinationTest.php:163:   * Tests node_access() and node access queries with multiple node languages.
./core/modules/node/lib/Drupal/node/Tests/NodeTestBase.php:35:   * Asserts that node_access() correctly grants or denies access.
./core/modules/node/lib/Drupal/node/Tests/NodeTestBase.php:53:        'node_access() returns @result with operation %op, language code %langcode.',
./core/modules/node/lib/Drupal/node/Tests/NodeTestBase.php:60:      $this->assertEqual($result, node_access($op, $node, $account, $langcode), $msg);
./core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareTest.php:126:   * Tests node_access() and node access queries with multiple node languages.
./core/modules/node/lib/Drupal/node/NodeTranslationController.php:22:    return node_access($op, $entity);
./core/modules/node/tests/modules/node_access_test/node_access_test.module:241: * Implements hook_node_access().
./core/modules/node/tests/modules/node_access_test/node_access_test.module:243:function node_access_test_node_access($node, $op, $account, $langcode) {
./core/modules/node/node.module:57: * Modules should return this value from hook_node_access() to allow access to a
./core/modules/node/node.module:65: * Modules should return this value from hook_node_access() to deny access to a
./core/modules/node/node.module:73: * Modules should return this value from hook_node_access() to indicate no
./core/modules/node/node.module:1620:      $access[$cid] = node_access($op, node_load($node->nid), $account, $langcode) && ($node->isDefaultRevision() || node_access($op, $node, $account, $langcode));
./core/modules/node/node.module:1638:    if (node_hook($type->type, 'form') && node_access('create', $type->type)) {
./core/modules/node/node.module:2465: * In determining access rights for a node, node_access() first checks whether
./core/modules/node/node.module:2469: * Next, all implementations of hook_node_access() will be called. Each
./core/modules/node/node.module:2486: * the process above is followed except that hook_node_access() is not called on
./core/modules/node/node.module:2493: * Note: Even a single module returning NODE_ACCESS_DENY from hook_node_access()
./core/modules/node/node.module:2529:function node_access($op, $node, $account = NULL, $langcode = NULL) {
./core/modules/node/node.module:2563: * Implements hook_node_access().
./core/modules/node/node.module:2565:function node_node_access($node, $op, $account) {
./core/modules/node/node.module:2640: * at once, or decide to apply some other hook_node_access() implementation to
./core/modules/node/node.module:2903: * node_access() can use this function when doing mass updates due to widespread
./core/modules/node/node.module:3561:    return node_access('view', $entity);
./core/modules/node/node.pages.inc:47:    if (node_hook($type->type, 'form') && node_access('create', $type->type)) {
./core/modules/translation/translation.module:121: * Implements hook_node_access().
./core/modules/translation/translation.module:123:function translation_node_access($node, $op, $account, $langcode) {
./core/modules/translation/translation.module:151:  return node_access('view', $node, $account) && (user_access('translate all content', $account) || ($node->uid == $account->uid && user_access('translate own content', $account)));
./core/modules/translation/translation.pages.inc:49:      if (node_access('update', $translation_node)) {

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityFieldAccessCheck.phpundefined
@@ -44,7 +44,7 @@ public function accessEditEntityField(EntityInterface $entity, $field_name) {
     $entity_type = $entity->entityType();
     // @todo Generalize to all entity types once http://drupal.org/node/1862750
     // is done.
-    return ($entity_type == 'node' && $node->access('update') && field_access('edit', $field_name, $entity_type, $entity));
+    return field_access('edit', $field_name, $entity_type, $entity));

I know that WimLeers is working on this part, I would just ignore it. You would still need to do a $entity->access('update') if you want to keep it. but it's probably just going to conflict.

Didn't know which direction to go. Might need some clarification from Berdir.

Patch fixes php error and restores previous return statement as suggested in #23

Anyone able to provide me feedback? Or has this been postponed?

#25: 1947880-deprecate-node-access-25.patch queued for re-testing.

The changes so far look good but we want to remove node_access() completely. Multiple of the remaining cases that I mentioned have already been removed, so most if not all of the remaining cases can now be converted.

Note that your grep contains a fair amount of hook_node_access() references, those should stay. Grepping for " node_access(" should give you better results.

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -260,7 +260,7 @@ public function access($operation = 'view', \Drupal\user\Plugin\Core\Entity\User
-      ->$method($this, LANGUAGE_DEFAULT, $account);
+      ->$method($this, $this->language()->langcode, $account);

It would be cool to have a test for that, as it's sort of security relevant.

Been a while. I'm going to take this task back. If you'd like to jump back in james please let me know.

Reroll caused a couple conflicts. Didn't feel super confident doing this. So i thought i'd breakdown the merge conflicts:

core/lib/Drupal/Core/Entity/Entity.php

<<<<<<< HEAD
      ->access($this, $operation, Language::LANGCODE_DEFAULT, $account);
=======
      ->$method($this, $this->language()->langcode, $account);
>>>>>>> Reroll

I went with head on this one.

core/modules/edit/lib/Drupal/edit/Access/EditEntityFieldAccessCheck.php

<<<<<<< HEAD
    return $entity->access('update') && field_access('edit', $field_name, $entity->entityType(), $entity);
=======
    $entity_type = $entity->entityType();
    // @todo Generalize to all entity types once http://drupal.org/node/1862750
    // is done.
    return field_access('edit', $field_name, $entity_type, $entity));
>>>>>>> Reroll

More on core/modules/edit/lib/Drupal/edit/Access/EditEntityFieldAccessCheck.php

<<<<<<< HEAD
    return $entity->access('update') && field_access('edit', $field_name, $entity->entityType(), $entity);
=======
    $entity_type = $entity->entityType();
    // @todo Generalize to all entity types once http://drupal.org/node/1862750
    // is done.
    return field_access('edit', $field_name, $entity_type, $entity));
>>>>>>> Reroll
  }

  /**
   * Validates and upcasts request attributes.
   */
  protected function validateAndUpcastRequestAttributes(Request $request) {
    // Load the entity.
    if (!is_object($entity = $request->attributes->get('entity'))) {
      $entity_id = $entity;
      $entity_type = $request->attributes->get('entity_type');
      if (!$entity_type || !entity_get_info($entity_type)) {
        throw new NotFoundHttpException();
      }
      $entity = entity_load($entity_type, $entity_id);
      if (!$entity) {
        throw new NotFoundHttpException();
      }
      $request->attributes->set('entity', $entity);
    }

    // Validate the field name and language.
    $field_name = $request->attributes->get('field_name');
    if (!$field_name || !field_info_instance($entity->entityType(), $field_name, $entity->bundle())) {
      throw new NotFoundHttpException();
    }
    $langcode = $request->attributes->get('langcode');
    if (!$langcode || (field_valid_language($langcode) !== $langcode)) {
      throw new NotFoundHttpException();
    }
  }

}

Went with head there too

Ended up with head on all of them. I just assumed that those references have been removed.

Attached is the patch after being rerolled.

I was going to add changes. But we should see if the patch still works or if i messed up the reroll.

Going to try the reroll again. Instead this time i just reverted my changes on the merge conflicted files.

Here's a reroll that should apply for now.

Unfortunately it's not already possible to remove the node_access function due to existing access callback definitions in node_menu(). This has to wait until the node_menu function will be cleaned up. I addressed this in a follow up issue #2019677: Remove node_access.

+++ b/core/modules/book/book.moduleundefined
@@ -124,7 +124,8 @@ function book_node_view_link(EntityInterface $node, $view_mode) {
+      $node = entity_create('node', array('type' => $child_type));
+      if ((user_access('add content to books') || user_access('administer book outlines')) && $node->access('create', $child_type) && $node->status == 1 && $node->book['depth'] < MENU_MAX_DEPTH) {

This replaces the already existing $node variable. You need to use something like $child_node instead.

I was not able to fix the issues with the node access test, fixed the book related failing though here's what i have for now.

Got it now

This blocks any routes that want to properly use the NodeAccessController as requirements.

The innards of node_access() need to be moved into NodeAccessController.

Just a usual rerole. plach thought that langcode would not be really needed so we maybe just drop the passing?

Not passing in the language code breaks the node access language tests :)

I will get back to this after #1810370: Entity Translation API improvements landed, we'll see how this changes things in regards to the language.

#1810370: Entity Translation API improvements landed, rerolled patch.

See also #1979094: Separate create access operation entity access controllers to avoid costly EntityNG instantiation, which improves the performance for create access checks.

Both issues are "only" major, but I think node_access() can be considered @deprecated in favor of the new entity access API anyway and as long as we don't actually remove it, this is not an API change anyway. So doesn't need to get in before api freeze.

Okay, this should get us closer.

Hah, I must have hit paste while saving the file :)

Fixed some silly mistakes.

So now it's failing only for speficic language when the op is 'view'...

Not sure why.

Language stuff is tricky. See #1953892: Move language node access logic into NodeAccessController about that snippet. According to @plach, we might be able to just remove it, use the current entity language and if you want to check access in a different language, use $entity->getTranslation('de')->access('view'). I thought we did that already at some point?

Straight reroll. Fixing #60 doesn't appear to be a Novice task, so removing that tag.

This apparently blocks #1938318: Convert book_remove_form to a new-style Form object so bumping to critical.

^

Re-roll and also actually removing the function.

Looks like the other issue unblocked itself with a hack, which I'm trying to remove again in #2072945: Remove the $langcode parameter in EntityAccessControllerInterface::access() and friends, but more than happy to remove this anyway, we should really use our API's. Not sure if it's still critical but I don't think we should keep node specific language stuff here.

Updating the title, since we are not keeping node_access().

#65: node-1947880-65.patch queued for re-testing.

Re-roll.

I think I know what's wrong. Trying something out.

NodeTestBase::assertNodeAccess() was still assuming the magic lived in node_access().
But langcode cannot be passed to Node::access().

#73: node-access-1947880-73.patch queued for re-testing.

#73: node-access-1947880-73.patch queued for re-testing.

1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessTest.php
   @@ -36,13 +36,13 @@ function testNodeAccess() {
   +    $this->assertNodeAccess(array('create' => TRUE), $node2, $web_user2);
   

   Create access should not be run through $accessController->access() like that.

2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTestBase.php
   @@ -51,14 +59,17 @@ function setUp() {
   +        '$node->access() returns @result with operation %op, language code %langcode.',
   

   The test case does not run the check through $node->access(), but through the access controller directly.

Yay that it's green, but not sure how useful it is if we convert here to the API's that we're trying to remove in #2072945: Remove the $langcode parameter in EntityAccessControllerInterface::access() and friends

Disregard that, it's not yet clear if that will even happen like that, the $langcode parameter might still remain.

So I'm fine moving on here. Will check if the patch still applies.

79: drupal_1947880_79.patch queued for re-testing.

Tagging for reroll.

Re-roll. I also removed one last code comment reference to node_access().

Let's do.

+++ b/core/modules/forum/forum.module
@@ -160,7 +160,7 @@ function forum_menu_local_tasks(&$data, $route_name) {
+      if (\Drupal::entityManager()->getAccessController('node')->createAccess($type)) {

Do we have an issue for making these calls a bit less verbose? This isn't just about access checks so doesn't affect this patch.

Otherwise looks great. Committed/pushed to 8.x, thanks!

This issue mostly just implements the existing new API and is already referenced in https://drupal.org/node/1862420. Add a sentence about the removed function.