Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.We're still suffering from a 10 year old confusion here...
#2473021: Deprecate NodeAccessControlHandlerInterface::writeGrants() for removal in Drupal 9.0.x has raised the confusion level again significantly — we currently have the following (merging NodeAccessControlHandlerInterface and NodeAccessControlHandler):
/**
* Gets the list of node access grants.
*
* This function is called to check the access grants for a node. It collects
* all node access grants for the node from hook_node_access_records()
* implementations, allows these grants to be altered via
* hook_node_access_records_alter() implementations, and returns the grants to
* the caller.
*
* @param \Drupal\node\NodeInterface $node
* The $node to acquire grants for.
*
* @return array $grants
* The access rules for the node.
*/
public function acquireGrants(NodeInterface $node) {
$grants = $this->moduleHandler->invokeAll('node_access_records', array($node));
// Let modules alter the grants.
$this->moduleHandler->alter('node_access_records', $grants, $node);
// If no grants are set and the node is published, then use the default grant.
if (empty($grants) && $node->isPublished()) {
$grants[] = array('realm' => 'all', 'gid' => 0, 'grant_view' => 1, 'grant_update' => 0, 'grant_delete' => 0);
}
return $grants;
}
/**
* Writes a list of grants to the database, deleting any previously saved ones.
*
* Modules that use node access can use this function when doing mass updates
* due to widespread permission changes.
*
* Note: Don't call this function directly from a contributed module. Call
* \Drupal\node\NodeAccessControlHandlerInterface::acquireGrants() instead.
*
* @param \Drupal\node\NodeInterface $node
* The node whose grants are being written.
* @param $delete
* (optional) If false, does not delete records. This is only for optimization
* purposes, and assumes the caller has already performed a mass delete of
* some form. Defaults to TRUE.
*
* @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
* Use \Drupal\node\NodeAccessControlHandlerInterface::acquireGrants().
*/
public function writeGrants(NodeInterface $node, $delete = TRUE) {
$grants = $this->acquireGrants($node);
$this->grantStorage->write($node, $grants, NULL, $delete);
}
acquireGrants() just returns the grant records, but it doesn't do anything in the database. Calling it "instead" does not work!
writeGrants() calls acquireGrants() and then writes them to the database. It is the one that everyone needs to call, even though it's deprecated and its docblock says "Don't call this function". Core (and probably everyone else) works around this contradiction by just copying the two lines in writeGrants(), which is obviously stupid.
acquireGrants(), OTOH, is only useful if you're going to write those grant records, which is precisely what writeGrants() does. There's one exception: Devel Node Access would like to call acquireGrants() in order to verify that what's in the node_access table is actually correct.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | interdiff-2782175-8-11.txt | 570 bytes | kalistos |
| #11 | drupal-node_access_acquire_and_write_grants-2782175-11.patch | 5.36 KB | kalistos |
| #2 | acquireAndWriteGrants-2782175-2.diff | 5.02 KB | salvis |
Comments
Comment #2
salvisMuch of the confusion comes from less than ideal naming of those methods which goes back to D5. I propose to keep the deprecated status of
writeGrants()and introduce a new method calledacquireAndWriteGrants().And I would like to keep
acquireGrants()as a separate method for the benefit of contribs that want to find out what records we should have in the NA table — without writing to the table!Comment #3
markdorisonacquireGrants()with a method name that is more clear in that it is just retrieving the grants.retrieveGrants()? Not sure of our level of tolerance for these types of changes.'false' should be capitalized (FALSE). Line also wraps beyond 80 characters.
Comment #4
salvisThank you for your review!
2.
retrieveGrants()sounds an awful lot likereadGrants()orloadGrants(), as if we were just "retrieving" them from the database or so. But there's a whole lot more going in during this call — I would vote for keeping theacquireGrants()name.3. I just copied that comment as it was, but I'm attaching a new patch with fixed capitalization and line lengths. The same comment appears also in NodeGrantDatabaseStorageInterface.php and this patch fixes it there, too, plus another comment line that is too long.
Comment #5
markdorisonComment #6
alexpottUnrelated changes. Should be a separate issue.
We need to know why this is here. Just removing it without that consideration being noted on the issue doesn't make sense. The fact this is already deprecated means that someone has already considered that necessary....
It came from node_access_write_grants() in D7 but actually \Drupal\node\NodeAccessControlHandlerInterface::writeGrants is doing what node_access_acquire_grants() did in D7. Nice.
I think we should do this quite differently. We should undeprecate \Drupal\node\NodeAccessControlHandlerInterface::writeGrants and not introduce a new method. We should remove the warning not to use and revert much of #2473021: Deprecate NodeAccessControlHandlerInterface::writeGrants() for removal in Drupal 9.0.x. We should file a follow-up to rename \Drupal\node\NodeAccessControlHandlerInterface::acquireGrants and deprecate that method. And lastly we should file a followup to discuss the architecture of NodeAccessControlHandler / node.grant_storage. The equivalent to D7's node_access_write_grants() is actually on the node.grant_storage service and now we're recommending people call it all the time (precisely the opposite advice lol).
Comment #7
alexpotttbh I'm not sure that the NodeAccessControlHandler should have all the methods to write and acquire grants. In my mind we should have a NodeGrants service that has all the logic pertaining to the effects of hook_node_access_records() and hook_node_grants().
Comment #8
salvisThank you for reviewing. I'm attaching a revised patch without the "unrelated changes". I had put them there because that comment is an exact copy of the other one.
The reason why it's here is that I asked to have it put there in D7 in #237634: Rename node_access_write_grants() to _node_access_write_grants() and discourage its use. That issue also has the reasoning. In short, the corresponding function was called node_access_write_grants() at the time and it would just delete the existing grant records and write the provided set of new ones. If a node access module called that function, it was unable to coexist with any other node access modules, breaking them!
Unfortunately, what was committed five years ago was not perfect, which has perpetuated the confusion. I'm trying to clean it up now. Since the current writeGrants() method faithfully calls acquireGrants(), it does not have this problem anymore, and it is the method that everyone should call, rather than duplicating the two calls inside.
Yes, that's why I propose to keep it deprecated and introduce the new acquireAndWriteGrants() method, kind of just renaming writeGrants(). This will leave us in a safe and comprehensible state.
With the introduction of hook_node_access_records_alter() the whole mechanism was changed anyway, and I don't see why we should have to carry over parts of the old procedural idiom into the D8 class.
Comment #11
kalistos CreditAttribution: kalistos at Adyax commentedError fixing.
Comment #20
LendudeThis seems aimed at making an improvement to DX, but nothing is broken as such. So reclassifying as a task. If somebody feels something is broken and it should be a bug, please add a test-only patch that demonstrates the failure.