Updated: Comment #60

Problem/Motivation

The node access storage is hardwired to SQL. Bad.

Proposed resolution

Create a NodeGrantStorageControllerInterface and implement it for SQL in NodeGrantStorageController.

Remaining tasks

Reviews are needed.

User interface changes

None.

API changes

node_access_acquire_grants() no longer writes to the database. Call node_access_write_grants() instead.

CommentFileSizeAuthor
#72 65-67-interdiff.txt2.08 KBalexpott
#72 1921426_67.patch38.5 KBalexpott
#70 63-65-interdiff.txt32.2 KBalexpott
#70 1921426_65.patch38.49 KBalexpott
#68 1921426_65.patch37.15 KBchx
#68 interdiff.txt6.18 KBchx
#66 1921426_65.patch36.82 KBchx
#66 interdiff.txt5.95 KBchx
#63 1921426_63.patch38.67 KBchx
#63 interdiff.txt10.11 KBchx
#61 1921426_61.patch38.59 KBchx
#61 interdiff.txt7.61 KBchx
#60 1921426_60.patch36.81 KBchx
#58 node_acccss-1921426-58.patch36.04 KBdawehner
#58 interdiff.txt760 bytesdawehner
#53 1921426_53.patch36.04 KBchx
#53 interdiff.txt4.23 KBchx
#51 1921426_51.patch34.43 KBchx
#51 interdiff.txt2.27 KBchx
#50 node_access-1921426-50.patch33.17 KBdawehner
#50 interdiff.txt3.39 KBdawehner
#47 node_access-1921426-47.patch32.82 KBdawehner
#47 interdiff.txt13.33 KBdawehner
#40 1921426-node-access-dic.patch25.4 KBagentrickard
#37 node-1921426-34.patch25.42 KBchx
#37 interdiff.txt562 byteschx
#34 node-1921426-34.patch25.42 KBdawehner
#34 interdiff.txt1.04 KBdawehner
#31 node-1921426-31.patch25.44 KBdawehner
#31 interdiff.txt1.89 KBdawehner
#29 interdiff.txt6.17 KBdawehner
#29 drupal-1921426-29.patch25.16 KBdawehner
#27 drupal-1921426-27.patch24.49 KBdawehner
#27 interdiff.txt648 bytesdawehner
#26 drupal-1921426-26.patch24.46 KBdawehner
#26 interdiff.txt5.24 KBdawehner
#24 node-1921426-24.patch22.07 KBdawehner
#20 node-access-dic_20.patch20.47 KBagentrickard
#13 node-access-dic_13.patch19.46 KBagentrickard
#9 node-access-dic.patch17.01 KBmarcingy
#2 node-access-dic.patch16.83 KBmarcingy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

Real life is likely going to mean that it will be some time next week before I get the first patch up but kinda of thinking about stuff in the background in between kickboxing training and work.

marcingy’s picture

Status: Active » Needs review
FileSize
16.83 KB

Here is a patch, tests pass locally.

Status: Needs review » Needs work

The last submitted patch, node-access-dic.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review

Does not fail locally so retesting

marcingy’s picture

#2: node-access-dic.patch queued for re-testing.

chx’s picture

#2: node-access-dic.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, node-access-dic.patch, failed testing.

moshe weitzman’s picture

I had proposed that node access become a module that could be disabled and swapped and so on. Any reason not to do this? Modules that implement node access like OG would depend on it.

marcingy’s picture

Status: Needs work » Needs review
FileSize
17.01 KB

Rerolled to keep up with head changes

agentrickard’s picture

I'm with Moshe -- I'd love to see this spun out into an optional module.

I'd also like to see #1825984: Separate concerns for node access "acquire" and "write" actions addressed, likely as part of this patch.

marcingy’s picture

Assigned: marcingy » Unassigned

I don't have the desire to move this into a module I will be honest, I have a desire to see it as a pluggable solution but nothing else. So unassigning from me as I am not going to take this forward any more and there is no agreement on approach so even re-rolling seems pointless at moment.

agentrickard’s picture

That seems a little abrupt. We're just discussing _preferences_.

agentrickard’s picture

FileSize
19.46 KB

Here's a re-roll that separates node_access_acquire_grants() from node_access_write_grants(), which makes much more sense to me.

I'm with @marcingy here. Let's get this change in and then see if we can move to another module.

agentrickard’s picture

Sigh. Note that the DIC patch in general, and my last change specifically reverts #237634: Rename node_access_write_grants() to _node_access_write_grants() and discourage its use. I still think this one is more proper.

Status: Needs review » Needs work

The last submitted patch, node-access-dic_13.patch, failed testing.

agentrickard’s picture

Status: Needs work » Needs review

That fail seems unrelated to this patch. Odd.

xjm’s picture

#13: node-access-dic_13.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, node-access-dic_13.patch, failed testing.

agentrickard’s picture

Well, I suspect that fail tells us that node_access_rebuild() didn't work properly. Fixing.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
20.47 KB

There we go.

andypost’s picture

#20: node-access-dic_20.patch queued for re-testing.

fago’s picture

#20: node-access-dic_20.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, node-access-dic_20.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
22.07 KB

Let's add an interface for all this public methods and rerole it.

dawehner’s picture

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -126,4 +127,158 @@ protected function accessGrants(EntityInterface $node, $operation, $langcode, Ac
+   $query = db_select('node_access');
...
+      $query = db_delete('node_access')->condition('nid', $node->nid);
...
+    if (!empty($grants) && count(module_implements('node_grants'))) {

There should be some more injection going on.

+++ b/core/modules/node/node.moduleundefined
@@ -2666,27 +2666,7 @@ function node_access_view_all_nodes($account = NULL) {
+    $access[$account->uid] = entity_access_controller('node')->nodeAccessCheckAll($account);

@@ -2839,76 +2766,32 @@ function node_access_acquire_grants(EntityInterface $node, $delete = TRUE) {
+  entity_access_controller('node')->nodeAccessWriteGrants($node, $grants, NULL, $delete);

@@ -2966,7 +2849,8 @@ function node_access_needs_rebuild($rebuild = NULL) {
+  $controller = entity_access_controller('node');

THis should better use Drupal::entityManager()->getAccessController('node')

dawehner’s picture

FileSize
5.24 KB
24.46 KB

There we go.

dawehner’s picture

FileSize
648 bytes
24.49 KB

Good catch by jibran.

jibran’s picture

Status: Needs review » Needs work

Thanks for the reroll.
#25.2 is still pending.

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -7,16 +7,59 @@
+class NodeAccessController extends EntityAccessController implements EntityControllerInterface {

EntityControllerInterface also NodeAccessControllerInterface

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -126,4 +169,158 @@ protected function accessGrants(EntityInterface $node, $operation, $langcode, Ac
+  public function nodeAccessCheckAll($account) {

Typehint missing.

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -126,4 +169,158 @@ protected function accessGrants(EntityInterface $node, $operation, $langcode, Ac
+  public function nodeAccessAlter($query, $tables, $op, $account, $base_table) {

Same here for $tables and $account

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -126,4 +169,158 @@ protected function accessGrants(EntityInterface $node, $operation, $langcode, Ac
+  public function nodeAccessWriteGrants($node, $grants, $realm = NULL, $delete = TRUE) {

Here too.

dawehner’s picture

Status: Needs work » Needs review
FileSize
25.16 KB
6.17 KB

Thanks for the good review, here are fixes for everything.

jibran’s picture

I am trying to improve my reviewing skills so please bare with me.

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -126,4 +169,162 @@ protected function accessGrants(EntityInterface $node, $operation, $langcode, Ac
+   * @param NodeInterface $node
+   * @param array $grants
+   * @param null $realm
+   * @param bool $delete

Not required.

+++ b/core/modules/node/lib/Drupal/node/NodeAccessControllerInterface.phpundefined
@@ -0,0 +1,98 @@
+  public function nodeAccessWriteGrants(NodeInterface $node, array $grants, $realm = NULL, $delete = TRUE);

NodeInterface namespace missing.

+++ b/core/modules/node/node.moduleundefined
@@ -2834,76 +2761,32 @@ function node_access_acquire_grants(EntityInterface $node, $delete = TRUE) {
  * @param \Drupal\Core\Entity\EntityInterface $node
...
+function node_access_write_grants(EntityInterface $node, $delete = TRUE) {

Should be NodeInterface

dawehner’s picture

FileSize
1.89 KB
25.44 KB

That's a good review. The point two, though is not valid. because it is not documentation.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @dawehner. I think it is ready to fly.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/node/lib/Drupal/node/NodeAccessControllerInterface.php
@@ -0,0 +1,98 @@
+   * @return mixed
+  @see node_access_write_grants()

Can we have a quick re-roll for the missing indentation (and new-line, actually)?

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
25.42 KB

Sure, I guess this can be directly RTBC again.

dawehner’s picture

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

Awesome, thanks @dawehner!

chx’s picture

FileSize
562 bytes
25.42 KB

Fixed a space.

dawehner’s picture

Candidate for the trivial patch change of the month!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll...

curl https://drupal.org/files/node-1921426-34_0.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 26034  100 26034    0     0  14912      0  0:00:01  0:00:01 --:--:-- 23950
error: patch failed: core/modules/node/node.module:2751
error: core/modules/node/node.module: patch does not apply
agentrickard’s picture

Status: Needs work » Needs review
FileSize
25.4 KB

Here's a re-roll that should test green.

Note to devs: node_access_acquire_grants() no longer writes to the database. Call node_access_write_grants() instead.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record
effulgentsia’s picture

effulgentsia’s picture

Issue tags: +Needs change record

#42 just intended to remove the "Needs reroll" tag, not the other one.

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

I think we should be creating a NodeAccessStorageController rather than implementing in the existing NodeAccessController... and I think we should refactor all the current code in the NodeAccessController that touches the db directly into the new NodeAccessStorageController class. So when it comes time to swap out the database touching functions we only have to implement the minimum. Separation of concerns and all...

Setting back to needs review to get the thoughts of others...

amateescu’s picture

I agree with #45. Also, a proper query-time (storage) entity access will allow us to remove a lot of fugly code from Entity reference.

dawehner’s picture

FileSize
13.33 KB
32.82 KB

Moved to a grant storage controller.

tstoeckler’s picture

+++ b/core/modules/node/lib/Drupal/node/NodeGrandStorageControllerInterface.php
@@ -0,0 +1,120 @@
+interface NodeGrandStorageControllerInterface {

That should be ...GrantStorage... not ...GrandStorage...

chx’s picture

Status: Needs review » Needs work
-  protected function getController($entity_type, $controller_type) {
+  public function getController($entity_type, $controller_type) {

That's quite a change, but I think we are OK.

-    $grant_count = db_query('SELECT COUNT(*) FROM {node_access}')->fetchField();
+    $grant_count = Drupal::entityQuery('node')->count()->execute();

I suspect this is incorrect. Or if it isn't , some comments are in order.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.39 KB
33.17 KB

Oh right, so we need a new method...

chx’s picture

FileSize
2.27 KB
34.43 KB

Sniff, db_or / db_and in an otherwise well-injected class. How sad.

Edit: if you find an uncanny resemblance between the new method on the database and the one on the entity query that is quite deliberate.

Status: Needs review » Needs work

The last submitted patch, 1921426_51.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
4.23 KB
36.04 KB

The interdiff is against #50.

Status: Needs review » Needs work

The last submitted patch, 1921426_53.patch, failed testing.

amateescu’s picture

+++ b/core/modules/node/lib/Drupal/node/NodeGrantStorageControllerInterface.php
@@ -0,0 +1,128 @@
+/**
+ * Provides an interface for node access controllers.
+ */
+interface NodeGrantStorageControllerInterface {

I think the description is missing a 'storage' between 'node' and 'acess', and the name of the interface ties it to a grants-like system.

In my comment from #46 I was thinking more of something that could allow us to move these kind of things outside of Entity reference:

http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

Something like EntityStorageAccessControllerInterface I guess..

dawehner’s picture

Well on the other hand we also want to make clear that the difference between this type of access and the normal access is the fact that it is based on grants and can be used to change the query so maybe something like EntityStorageQueryAccessControllerInterface? (maybe skip the storage? )

amateescu’s picture

Yes, a EntityQueryAccessControllerInterface is also fine because it ties it to EntityQuery, which is exactly what we want :)

dawehner’s picture

Status: Needs work » Needs review
FileSize
760 bytes
36.04 KB

As long it is flexible enough to also cover non-EQ queries as well. Just for example for stuff like search api getting the information about the grants but not storing it is also helpful.

But seriously are you sure this works in this patch? We would need to think about a basic API which works on each entity type. I would suggest to defer this discussion into a follow up.

Status: Needs review » Needs work

The last submitted patch, node_acccss-1921426-58.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
36.81 KB
chx’s picture

FileSize
7.61 KB
38.59 KB

More stuff moved into a class, injected, stuff: node_access_write_grants and node_access_acquire_grants is dead.

chx’s picture

FileSize
10.11 KB
38.67 KB

Next iteration: fixed a copypaste doxygen and renamed methods on NodeGrantStorageControllerInterface not to include node, access and grant cos that's superflous per msonnabaum's request. (I run Drupal\node\Tests\NodeAccessBaseTableTest now before every post, it's reasonable to expect the patch passing if that one is passing for these sorts of changes.)

msonnabaum’s picture

I like the name changes.

It feels very odd to me however, that we'd be adding another entity controller just for grant storage. Maybe that could just be a service that the access controller pulls in?

chx’s picture

FileSize
5.95 KB
36.82 KB

If this would be Drupal 7, I would be arguing with #64 based both on process (we are already past API freeze so a certain urgency is necessary), technical (it makes sense to have a controller etc) and community (this is what a core committer asked for) grounds.

But this is Drupal 8 so we can only thank the OOP Masters for sharing their wisdom with us lowly struggling-to-get-out-of-the-old-procedural-ways coders. After all, the urgency can be fixed by me skipping some sleep, I am almost certainly wrong on everything technical and a core committer's word is not set in stone. I hope you can accept my apologies for ever questioning your word both here and IRC. Please find the patch attached and let me know if there's anything I can do. Perhaps change the service name dots to underscores? Just let me know. I will try to get this patch to completion as much as my limited abilities allow.

Status: Needs review » Needs work

The last submitted patch, 1921426_65.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
6.18 KB
37.15 KB

See I can't even roll a patch properly. Interdiff against #63.

msonnabaum’s picture

Phew, good thing this is D8.

alexpott’s picture

FileSize
38.49 KB
32.2 KB

Patch attached move NodeGrantStorageController to a service and changes the class name to NodeGrantDatabaseStorage. Plus it delegates most of the calls from the procedural layer through NodeAccessController to NodeGrantDatabaseStorage.

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

Looks great. My concerns are addressed.

alexpott’s picture

FileSize
38.5 KB
2.08 KB

Minor docs improvements

catch’s picture

Sorry to re-open that discussion, but why make the grant storage a service? Nothing else is going to use that, so it feels like bloating the container. Having it as another entity controller kept it restricted to Nodes.

alexpott’s picture

@catch because it makes the node annotation somewhat special - these annotation are supposed to be general metadata which tells the EntityManager how to manage the entity - the changes to the EntityManager kind of show why that approach is a bit weird - it changed EntityManager::getController() from public to protected... all the other controllers have public access methods eg. getAccessController()... so even here we are polluted a generalised space.

That's why I agree with and worked on the approach (suggested by @msonnabaum in IRC) in #70. Do we know the impact of "container bloat" as fas as I know services on the container are lazy loaded so the I would have thought the impact was minimal

catch’s picture

Status: Reviewed & tested by the community » Fixed

The services are lazy loaded but the container itself isn't.

What concerns me more is putting services on the container that are single-use, given it's likely people will want to inspect the container to see what's available. iirc there's a way to mark services private, but we're not using that yet.

I'm more concerned about that for things like routes which could balloon it, than things which are really one-offs like node access though, so let's leave it. Committed/pushed to 8.x.

alexpott’s picture

Title: Move node access storage to DIC » Change notice: Move node access storage to DIC
Priority: Normal » Critical
Status: Fixed » Active

Pretty sure we need a change notice here...

+++ b/core/modules/node/node.moduleundefined
@@ -2447,159 +2427,8 @@ function node_query_node_access_alter(AlterableInterface $query) {
-function node_access_acquire_grants(NodeInterface $node, $delete = TRUE) {
...
-function _node_access_write_grants(EntityInterface $node, $grants, $realm = NULL, $delete = TRUE) {

Change notice material...

agentrickard’s picture

Assigned: Unassigned » agentrickard

On it.

agentrickard’s picture

Status: Active » Needs review
chx’s picture

Title: Change notice: Move node access storage to DIC » Move node access storage to DIC
Priority: Critical » Normal
Status: Needs review » Fixed

That looks good.

Pancho’s picture

Title: Move node access storage to DIC » [Followup] Move node access storage to DIC
Priority: Normal » Critical
Status: Fixed » Active

Docs and change notice need to be corrected, after #61 completely removed node_access_write_grants() and node_access_acquire_grants():

More stuff moved into a class, injected, stuff: node_access_write_grants and node_access_acquire_grants is dead.

Besides some code comments, the change notice refers to these nonexisting functions:

node_access_acquire_grants() has been changed to only gather and report node access grants. To write grants to the database, you must now call node_access_write_grants().

agentrickard’s picture

Which version of this patch was committed?!? The change notice was written from #72.

agentrickard’s picture

Updated the change notice, but the committed patch replaces one sin with another.

writeGrants() now assumes that you want to acquireGrants() using the standard method. Which means we still have one method doing two things.

xjm’s picture

Issue tags: -Needs change record

Oh man, this totally was not on my radar.

Unless there is a critical regression, can we close this issue and open a separate one?

It sounds like the current change notice is correct for what's in HEAD, so untagging.

xjm’s picture

Priority: Critical » Normal
xjm’s picture

Issue summary: View changes

Updated issue summary.

agentrickard’s picture

Assigned: agentrickard » Unassigned
Issue summary: View changes

Does anyone understand the status of this issue?

agentrickard’s picture

Status: Active » Closed (fixed)

Per xjm in IRC, it lloks like the outstanding concerns can be addressed in #1825984: Separate concerns for node access "acquire" and "write" actions.