Deprecate node_access_view_all_nodes()

Looks nice, but how this internal cache will be cleared? I guess it needs extra tests

1. +++ b/core/modules/node/src/NodeAccessControlHandler.php
   @@ -192,4 +199,29 @@ public function checkAllGrants(AccountInterface $account) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function viewAllNodes(AccountInterface $account) {
   +    $account_id = $account->id();
   

   I think adding extra API's to EntityAccessControlHandlers is a bit problematic, but we have quite a few grant related methods there already, so it makes sense I suppose.

2. +++ b/core/modules/node/tests/src/Unit/Access/NodeAccessTest.php
   @@ -0,0 +1,54 @@
   +   * @expectedDeprecation node_access_view_all_nodes() is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use \Drupal::entityTypeManager()->getAccessControlHandler("node")->viewAllNodes($account). See https://www.drupal.org/node/3038909.
   +   * @see node_access_view_all_nodes();
   +   */
   +  public function testNodeAccessViewAllNodesDeprecation() {
   +    $container = new ContainerBuilder();
   +    $current_user = $this->prophesize(AccountProxyInterface::class);
   +    $container->set('current_user', $current_user->reveal());
   +    $entity_type_manager = $this->prophesize(EntityTypeManagerInterface::class);
   +    $node_access_control_handler = $this->prophesize(NodeAccessControlHandlerInterface::class);
   +    $entity_type_manager->getAccessControlHandler('node')->willReturn($node_access_control_handler->reveal());
   +    $container->set('entity_type.manager', $entity_type_manager->reveal());
   +    \Drupal::setContainer($container);
   +
   +    require_once $this->root . '/core/modules/node/node.module';
   +    node_access_view_all_nodes();
   +  }
   

   We don't do unit tests for functions, as loading stuff then pollutes other unit tests.

   I'd suggest to simply make this a kernel test, should be much simpler?

Rerolled patch for 9.1.x
Moved test methods to core/modules/node/tests/src/Kernel/NodeAccessTest.php
Please review!

Needs another reroll, also needs the deprecation updating.

Re-roll patch created, Also updated deprecations. Please have a look.

Fixed format issue, Please have a look.

Fixed fail deprecation tests, Please have a look.

Added some comments.

For the unresolved threads on the MR.

Test will fail until #3514197: ModuleHandler::resetImplementations should reset all properties with hook implementations is fixed.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Can the CR also be updated that drupal_static_reset() with the node_grant key is deprecated.

Done

LGTM

One comment on the MR.

Converted to use a memory cache

Think we missed the 11.2 boat can those be updated to 11.3, reviewed again and closed the rest of the threads as answered.

If you are another contributor eager to jump in, please allow the previous poster(s) at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

Thanks! All feedback appears to be addressed

Adding issue credit.

As per @acbramley, I'm concerned that the focus of this issue is about the use of drupal_static and not the node access API. I think it is great to refactor out usages of drupal static - but this needs to be done with an eye to the resulting API. We have the following node access related functions left in node.module and we should have a plan for them:

• node_access_grants
• node_access_view_all_nodes
• node_access_needs_rebuild
• node_access_rebuild
• _node_access_rebuild_batch_operation
• _node_access_rebuild_batch_finished

Are there issues for the other methods? It feels as though they split down into two groups:

• node_access_grants and node_access_view_all_nodes
• The *rebuild* ones

Are there existing issues? I think not looking at #3532197: [meta] Deprecate node.module functions.

I think we need to agree the API shape before doing this issue and once we've done that we should focus the issues on doing that and as a by product remove drupal_static usage where we can.

I think that converting node_access_grants and node_access_view_all_nodes to a single service makes sense. I also think the service should be marked as @internal because it is API glue and if you want to affect this API you sure be implementing the hooks and not doing anything to this service.

Added #3533299: Deprecate node access rebuild functions and updated #2473041: Restructure node access grant behavior into the node access handler

I think node_access_grants could live on the service that this provides, maybe with a new method name as per the IS in that issue to more accurately describe what it does (and so it's not so confusing with hook_node_grants)

What do you think @alexpott?

@acbramley yes I think this issue could create a service for both of these methods in node.module. I've been trying to think if any of the existing node access services could be locations and I think I agree with the earlier posts - a separate service is a good idea. I considered node.grant_storage but I think this is not about storage and the new service needs to be injected into that one. I also considered cache_context.user.node_grants - or maybe making a new service that is also a cache context but that felt overly complex to. Therefore a new service with two public methods for each function we're replacing and marked @internal because it is not meant to be extended - the extension points are the node grant hooks.

How's that?

The other method should be moved in the other issue

@acbramley I think we should widen the scope to include the other function here. Doing it piecemeal results in a less complete API for no real gain.

Deprecated node_access_grants as well and added the note to the @internal tag.

I think a good next step would be to flesh out #2473041: Restructure node access grant behavior into the node access handler. The first step should probably be to remove all/most of the grants functions from NodeAccessControlHandler, especially the ones that just proxy to the grant storage service. Like there's no reason hasViewAllNodesGrant or NodeRequirements::runtime needs to call a function on the access control handler...

I also found some interesting stuff like NodeHooks1::nodeAccess which implements hook_ENTITY_TYPE_access for its own entity type 🤷‍♂️ There's no wonder why barely anyone understands this system fully (including myself)

So after refactoring node_access_view_all_nodes() into \Drupal\node\NodeGrantDatabaseStorage::checkAll() - see https://git.drupalcode.org/project/drupal/-/merge_requests/12586 - I think this issue can return to it's original intent - because there is no real API change. And then we can use #2473041: Restructure node access grant behavior into the node access handler to decide how to deal with node_access_grants() as part of an issue that is completely API focussed.

@acbramley sorry it is taken a while to see through the API trees here but I think that the new MR points to a better place :)

One thing that is "interesting" is that node_access_view_all_nodes() is documented to return a bool. ATM in 11.x it only does this if there are no hook_node_grants implementations. Otherwise it returns a 0 or 1 integer because that's what Drupal::entityTypeManager()->getAccessControlHandler('node')->checkAllGrants($account) returns. With the new MR it will always return a 0 or 1. Boo to us and our lax return types. I think we should do a separate issue and CR to change \Drupal\node\NodeAccessControlHandlerInterface::checkAllGrants() and \Drupal\node\NodeGrantDatabaseStorageInterface::checkAll() to return a bool as this is a TRUE / FALSE access check and an integer is confusing.

Added some notes on both MRs, I don't really think I like either now lol

@acbramley I've responded to your points on the new MR.

I guess we need someone else to weigh in, I don't feel this is much cleaner than my MR but it's hard when this whole system is a bowl of spaghetti in the first place.

I'd appreciate if you could add any notes to #2473041: Restructure node access grant behavior into the node access handler on what you'd like to achieve there too since we've flip flopped a couple of times.

Node access subsystem maintainer here.

I haven't had a chance to review the MR in detail yet (will try to do so soon) but I strongly, strongly advise against making any API or behavior changes in this issue. The functionality of this and all the procedural functions in the node access system has a lot of edgecase behaviors that are fiddly and changing them even slightly can break access control behavior on sites with complicated implementations. We don't only have to worry about individual contrib modules; we have to worry about sites with multiple access control modules, or custom code that implements custom access control and/or integrates multiple access control modules, which is very common.

The refactoring and eventual deprecation of the whole API is much more feasible now that D7 is EOL, but for scope management's sake and for the sake of not having surprise behavior changes or access bypasses on some weird edgecase sites, none of it should happen in this issue. This issue should only deprecate and move the code so we can get it out of the .module file. We have an opportunity for many followups!

Thanks!

This is back to just node_access_view_all_nodes

Adding #2199001: Statically cache node access grants as related issue, don't think it's relevant to this issue but it would be relevant to further refactoring to try to get rid of the circular/semi-circular dependencies.

Followups followups ➡️ 😇

Updated issue summary reflect the approach.

@xjm I think we have all the follow-ups in place already. I found an existing issue for the return types - #2863737: Change the return type of NodeGrantDatabaseStorageInterface::checkAll() and NodeAccessControlHandler::checkAllGrants() to a bool and the deprecation / moving about of node.module code has plenty of issues #2473041: Restructure node access grant behavior into the node access handler and #3532197: [meta] Deprecate node.module functions for example. Is there something else we need to put in place?

Rebased, fixed a minor issue in the deprecation message and updated the CR a bit. I think this is good to go now. Agree that the follow ups we have in #63 cover plenty.

The main API change is going from returning TRUE when there are no grant hook implementations to returning 1 (to match the return signature of checkAll). That will then change in 2863737 so I think that's fine for now. The deprecated function still returns what it used to.

Leaving in NR since this is tagged for subsystem maintainer review which is not me.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased again.

Re:

Leaving in NR since this is tagged for subsystem maintainer review which is not me.

If it's blocked on subsystem maintainers for an extended period in general it can also be escalated to framework managers, although in this case since a subsytem maintainer is also a release manager someone RTBCing it with the tag on is fine as well. Meanwhile though I'm also going to ping the other active maintainer for the subsystem since I still don't have time to look at this this week.

From the node_access subsystem perspective, this seems fine.

From a module developer perspective, it's also fine, as I consider node_access_view_all_nodes() to be an internal function and don't call it directly. Even if I did, there is an OO replacement I can use, and this code directs me to that.

Untagging from #70; thanks Ken!

Note for committers, as I mentioned in Slack: The one thing here that needs very careful attention from a reviewer is the test coverage, to ensure the coverage is exactly identical with the new API. The fact that the test passes is not sufficient by itself.

Oh, belatedly confirming #63 -- @alexpott and I already discussed that in Slack last week but it looks like our consensus that the followups are covered did not make it back onto the issue. So we are all good. Thanks!

And updated credits.

Great, this is RTBC then

This looks good to me. Committed/pushed to 11.x, thanks!