hook_node_access() no longer fires for the 'create' operation

Well, hook_node_access() should just be executed in case you have a single node, not when you have a list of entries.

It's also not invoked for user 1 or roles that can Administer content.

However, it does appear that the 'create' operation is no longer invoked by hook_node_access().

Adding a die() statement here should cause \Drupal\node\Tests\NodeCreationTest to crash, but it does not.

Tracking through the code it looks like the work is handled by different hooks in D8.

From EntityAccessHandler.

    $access = array_merge(
      $this->moduleHandler()->invokeAll('entity_create_access', array($account, $context, $entity_bundle)),
      $this->moduleHandler()->invokeAll($this->entityTypeId . '_create_access', array($account, $context, $entity_bundle))
    );

Though I can't see where Node module invokes these, either, so I may just be misreading.

Here's the patch. Updating the issue summary with details.

@chx points out that 'existant' is not a word ;-p.

This is no longer a documentation-only patch. The mention of Drupal 8 in doxygen is very odd, ag ' \* .*Drupal 8'|grep -vi 'deprecated\|test' tells me this is simply not done elsewhere, the @see should be enough and https://www.drupal.org/node/1862420 should be amended to include the new hook and altogether made cleaner.

This is already documented on that page:

A default entity access controlhandler (Drupal\Core\Entity\EntityAccessControlHandler) is provided that takes care of a static cache for access checks and also invokes a hook named after the entity_type (hook_{$entity_type}_access(EntityInterface $operation, UserInterface $account, $langcode)). This follows the same pattern as hook_node_access() in 7.x. If no hook implementation returns something, checkAccess() is invoked.

Docblock update to remove the Drupal 8 reference.

Yes but the hook_ENTITY_TYPE_create_access hook is not mentioned AFAICS

I see. I think the above quote is a typo.

Updated docs page.

+++ b/core/modules/node/node.module
@@ -904,8 +907,6 @@ function node_node_access(NodeInterface $node, $op, $account) {
-    case 'create':
-      return AccessResult::allowedIfHasPermission($account, 'create ' . $type . ' content');
 

While I don't think it will ever happen in practice, someone could theoretically call node_node_access() directly with $op = 'create'.

This makes me think we should split this hunk (and only this hunk) out to an 8.1.x follow-up.

Why? It's a hook implementation, you are absolutely not supposed to call those.

Yes, you're not supposed to do lots of things.

We decided to commit things that break code doing things it should not for minor releases, unless it's necessary to fix a bug.

That way, if you have a site that has a module that does lots of horrible things, it breaks once every six months (or if we're lucky, just once when 8.1.x comes out then never again when the module is fixed), not every month.

This patch is a fairly extreme example, but the dead code can be removed in 8.1.x as early as tomorrow, so it doesn't hurt much to leave it dead for another 4.5 months in 8.0.x, then the entire branch will be lopped off.

Then in the unlikely case some custom module somewhere is re-animating that dead code, it'll blow up in April instead of January.

Shouldn't we at least push the documentation portion of this patch? This was a huge point of confusion for me when working on module ports.

Well I'd commit the whole thing if the hunk pointed out in #12 was split to an 8.1.x follow-up...

Revised per #16.

Unfortunately 8.1.0 is out now, so with the same argument, we need to update the @todo to 8.2 now?

Or we switch to 8.2.x, do it and backport without that part.

If we're going to touch node_node_access() and removing one case is OK then I would even consider to remove the hook entirely and move that remaining logic into checkAccess() where everything else is checked.

Also not sure this is a (major) bug, there is no functional problem here, just wrong documentation and dead code?

Thanks @Berdir.

+++ b/core/modules/node/node.module
@@ -906,6 +909,7 @@ function node_node_access(NodeInterface $node, $op, $account) {
+    // @TODO: The 'create' operation is deprecated and will be removed in 8.1.

Let's actually just remove the "and will be removed in 8.1" from this @todo. It should be formatted like:

// @todo The 'create' option is deprecated. https://www.drupal.org/node/followup

Then we can commit the docs fix to both branches now and consider what @Berdir suggests in a followup. The test case suggested in the summary also sounds like a good followup.

I think we should also add a CR for this to communicate that this was previously changed.

The case I would see for this to still be major is if the result of following the documentation would be information disclosure when the hook didn't fire. Is there a scenario where that would happen? E.g. if some other implementation allowed create access, but your implementation was trying to override and forbid it.

Created placeholder for the followup. #2730439: Follow up: hook_node_access() no longer fires for the 'create' operation

+++ b/core/modules/node/node.module
@@ -907,6 +910,7 @@ function node_node_access(NodeInterface $node, $op, $account) {
 
   switch ($op) {
+    // @todo The 'create' option is deprecated. https://www.drupal.org/node/2730439
     case 'create':
       return AccessResult::allowedIfHasPermission($account, 'create ' . $type . ' content');

We discussed that at the entity field issue triage meeting in Dublin.

It is not deprecated, it does not exist. And we do not support calling hooks directly, so we can just remove this.

the only thing we didn't agree on was if it can be removed in a patch or a minor release.

Updating patch for 8.2.x branch.
Removing comment as mentioned by @berdir

We also did agree in Dublin that this issue is major, because by documenting access control that does not actually work it could even lead to information disclosure for what appears to be "supported" code.

I don't care if it should be a patch or a minor release, doesn't matter, better to get it into 8.7 than waiting another 2 years :)

Marked #2825524: hook_node_access() is not called for 'create', contradicting its documentation as a duplicate.

This patch still needs a CR and unfortunately it does not apply :(

i think this hook work only for "anonymous" role

if any role have permission "Bypass content access control" , this hook not call

No, this works for everyone (except create access), might there might be caching involved.

Edit: Yes, the bypass permission bypasses everything including this hook. That's not the same as only for anonymous users, though :)

@alexpott: Not sure what you want to do in an CR. We have a CR for entity access that also documents that create access is separate since 2012 :) https://www.drupal.org/node/1862420.

@Berdir I was just making sure that we were not avoiding taking #21 in account. It's great that a CR already exists.

+++ b/core/modules/node/node.api.php
@@ -328,15 +331,14 @@ function hook_node_grants_alter(&$grants, \Drupal\Core\Session\AccountInterface
   switch ($op) {
-    case 'create':
-      return AccessResult::allowedIfHasPermission($account, 'create ' . $type . ' content');
-

I would add a

// Note create access is handled by hook_ENTITY_TYPE_create_access().

above switch ($op) because it helps explain why create is not in the list without having to read the methods docblock.

I think @alexpott meant "Node" instead of "Note". Note could also be an option, but then it would need to be "Note that...".

Also, make sure you provide an interdiff, that makes it easier to review the difference: https://www.drupal.org/documentation/git/interdiff

I meant Note - the that is superfluous but it can be added so // Note that create access is handled by hook_ENTITY_TYPE_create_access(). is great.

Ok, needs a reroll anyway.

#3061217: Clarify if hook_node_access() $node argument can or cannot be a string is a duplicate of this but it also contains an important fix to hook_node_access() docs that's not here.

Also I ponder if we should be maintaining hook_node_access() docs in node.api and not pointing to the docs in entity.api.php.

Here is update with changes according to #44

I don't care if it should be a patch or a minor release, doesn't matter, better to get it into 8.7 than waiting another 2 years :)
@berdir (28 November 2018 at 21:35)

and here we are... =]

So my colleague, @boobaa, hast just bumped into this dead code related to the "create" operation in node_node_access(). He actually spotted the missing break after the "create" case in the switch, long story short, we unnecessary spent time figuring out that how this bug is not discovered by any core tests...

How can we help to get this fix in core?

I deliberated about the changes to node_node_access and whether they'd have any impact. Some places in contrib are calling this directly!!! I'm guessing never with an op of create because it is totally broken. Also there are places in contrib that say "copying from node_node_access" which is a very good reason to fix this broken code. I also agree with @Berdir who has pointed out that that node_node_access shouldn't really exist but that can be done in a task follow-up.

@catch did ask for that hunk to be broken out before BUT in the interim things have got even worse and the $access from the create is not even being used. Atm if you call node_node_access with an op of create it will return you the access result for edit permissions. That is totally wrong.

I'm going to commit this to 9.1.x but I believe it should be backported to 8.9.x as this code has gone from wrong to dangerous.

Committed aa8d81c and pushed to 9.1.x. Thanks!

diff --git a/core/modules/node/node.api.php b/core/modules/node/node.api.php
index fb446bbd4e..8461d91139 100644
--- a/core/modules/node/node.api.php
+++ b/core/modules/node/node.api.php
@@ -8,7 +8,6 @@
 use Drupal\node\NodeInterface;
 use Drupal\Component\Utility\Html;
 use Drupal\Component\Utility\Xss;
-use Drupal\Core\Access\AccessResult;
 
 /**
  * @addtogroup hooks

Removed unused use on commit.

Discussed with @catch and we agreed to backport this to 9.1.x given the changes since the last rtbc.