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.

Files: 
CommentFileSizeAuthor
#72 65-67-interdiff.txt2.08 KBalexpott
#72 1921426_67.patch38.5 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 56,783 pass(es).
[ View ]
#70 63-65-interdiff.txt32.2 KBalexpott
#70 1921426_65.patch38.49 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 56,830 pass(es).
[ View ]
#68 1921426_65.patch37.15 KBchx
PASSED: [[SimpleTest]]: [MySQL] 56,817 pass(es).
[ View ]
#68 interdiff.txt6.18 KBchx
#66 1921426_65.patch36.82 KBchx
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#66 interdiff.txt5.95 KBchx
#63 1921426_63.patch38.67 KBchx
PASSED: [[SimpleTest]]: [MySQL] 56,943 pass(es).
[ View ]
#63 interdiff.txt10.11 KBchx
#61 1921426_61.patch38.59 KBchx
PASSED: [[SimpleTest]]: [MySQL] 57,156 pass(es).
[ View ]
#61 interdiff.txt7.61 KBchx
#60 1921426_60.patch36.81 KBchx
PASSED: [[SimpleTest]]: [MySQL] 56,760 pass(es).
[ View ]
#58 node_acccss-1921426-58.patch36.04 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,203 pass(es), 1,270 fail(s), and 616 exception(s).
[ View ]
#58 interdiff.txt760 bytesdawehner
#53 1921426_53.patch36.04 KBchx
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/lib/Drupal/node/NodeGrantStorageController.php.
[ View ]
#53 interdiff.txt4.23 KBchx
#51 1921426_51.patch34.43 KBchx
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#51 interdiff.txt2.27 KBchx
#50 node_access-1921426-50.patch33.17 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,448 pass(es).
[ View ]
#50 interdiff.txt3.39 KBdawehner
#47 node_access-1921426-47.patch32.82 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,791 pass(es).
[ View ]
#47 interdiff.txt13.33 KBdawehner
#40 1921426-node-access-dic.patch25.4 KBagentrickard
PASSED: [[SimpleTest]]: [MySQL] 56,714 pass(es).
[ View ]
#37 node-1921426-34.patch25.42 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,652 pass(es).
[ View ]
#37 interdiff.txt562 byteschx
#34 node-1921426-34.patch25.42 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,835 pass(es).
[ View ]
#34 interdiff.txt1.04 KBdawehner
#31 node-1921426-31.patch25.44 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,974 pass(es).
[ View ]
#31 interdiff.txt1.89 KBdawehner
#29 interdiff.txt6.17 KBdawehner
#29 drupal-1921426-29.patch25.16 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,811 pass(es).
[ View ]
#27 drupal-1921426-27.patch24.49 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,955 pass(es).
[ View ]
#27 interdiff.txt648 bytesdawehner
#26 drupal-1921426-26.patch24.46 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,495 pass(es).
[ View ]
#26 interdiff.txt5.24 KBdawehner
#24 node-1921426-24.patch22.07 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,836 pass(es).
[ View ]
#20 node-access-dic_20.patch20.47 KBagentrickard
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node-access-dic_20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 node-access-dic_13.patch19.46 KBagentrickard
FAILED: [[SimpleTest]]: [MySQL] 54,666 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#9 node-access-dic.patch17.01 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 54,558 pass(es).
[ View ]
#2 node-access-dic.patch16.83 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node-access-dic.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

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.

Status:Active» Needs review
StatusFileSize
new16.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node-access-dic.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here is a patch, tests pass locally.

Status:Needs review» Needs work

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

Status:Needs work» Needs review

Does not fail locally so retesting

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

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

Status:Needs review» Needs work

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

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.

Status:Needs work» Needs review
StatusFileSize
new17.01 KB
PASSED: [[SimpleTest]]: [MySQL] 54,558 pass(es).
[ View ]

Rerolled to keep up with head changes

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.

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.

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

StatusFileSize
new19.46 KB
FAILED: [[SimpleTest]]: [MySQL] 54,666 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review

That fail seems unrelated to this patch. Odd.

#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.

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

Status:Needs work» Needs review
StatusFileSize
new20.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node-access-dic_20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

There we go.

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

#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.

Status:Needs work» Needs review
StatusFileSize
new22.07 KB
PASSED: [[SimpleTest]]: [MySQL] 57,836 pass(es).
[ View ]

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

+++ 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')

StatusFileSize
new5.24 KB
new24.46 KB
PASSED: [[SimpleTest]]: [MySQL] 56,495 pass(es).
[ View ]

There we go.

StatusFileSize
new648 bytes
new24.49 KB
PASSED: [[SimpleTest]]: [MySQL] 57,955 pass(es).
[ View ]

Good catch by jibran.

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.

Status:Needs work» Needs review
StatusFileSize
new25.16 KB
PASSED: [[SimpleTest]]: [MySQL] 57,811 pass(es).
[ View ]
new6.17 KB

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

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

StatusFileSize
new1.89 KB
new25.44 KB
PASSED: [[SimpleTest]]: [MySQL] 57,974 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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

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)?

Status:Needs work» Needs review
StatusFileSize
new1.04 KB
new25.42 KB
PASSED: [[SimpleTest]]: [MySQL] 58,835 pass(es).
[ View ]

Sure, I guess this can be directly RTBC again.

Status:Needs review» Reviewed & tested by the community

Awesome, thanks @dawehner!

StatusFileSize
new562 bytes
new25.42 KB
PASSED: [[SimpleTest]]: [MySQL] 58,652 pass(es).
[ View ]

Fixed a space.

Candidate for the trivial patch change of the month!

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

Status:Needs work» Needs review
StatusFileSize
new25.4 KB
PASSED: [[SimpleTest]]: [MySQL] 56,714 pass(es).
[ View ]

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.

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

Issue tags:+Needs change record

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

Issue tags:+RTBC July 1

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

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...

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.

StatusFileSize
new13.33 KB
new32.82 KB
PASSED: [[SimpleTest]]: [MySQL] 56,791 pass(es).
[ View ]

Moved to a grant storage controller.

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

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

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.

Status:Needs work» Needs review
StatusFileSize
new3.39 KB
new33.17 KB
PASSED: [[SimpleTest]]: [MySQL] 56,448 pass(es).
[ View ]

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

StatusFileSize
new2.27 KB
new34.43 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new4.23 KB
new36.04 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/lib/Drupal/node/NodeGrantStorageController.php.
[ View ]

The interdiff is against #50.

Status:Needs review» Needs work

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

+++ 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..

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? )

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

Status:Needs work» Needs review
StatusFileSize
new760 bytes
new36.04 KB
FAILED: [[SimpleTest]]: [MySQL] 55,203 pass(es), 1,270 fail(s), and 616 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new36.81 KB
PASSED: [[SimpleTest]]: [MySQL] 56,760 pass(es).
[ View ]

StatusFileSize
new7.61 KB
new38.59 KB
PASSED: [[SimpleTest]]: [MySQL] 57,156 pass(es).
[ View ]

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

StatusFileSize
new10.11 KB
new38.67 KB
PASSED: [[SimpleTest]]: [MySQL] 56,943 pass(es).
[ View ]

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.)

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?

StatusFileSize
new5.95 KB
new36.82 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new6.18 KB
new37.15 KB
PASSED: [[SimpleTest]]: [MySQL] 56,817 pass(es).
[ View ]

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

Phew, good thing this is D8.

StatusFileSize
new38.49 KB
PASSED: [[SimpleTest]]: [MySQL] 56,830 pass(es).
[ View ]
new32.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.

Status:Needs review» Reviewed & tested by the community

Looks great. My concerns are addressed.

StatusFileSize
new38.5 KB
PASSED: [[SimpleTest]]: [MySQL] 56,783 pass(es).
[ View ]
new2.08 KB

Minor docs improvements

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.

@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

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.

Title:Move node access storage to DICChange 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...

Assigned:Unassigned» agentrickard

On it.

Status:Active» Needs review

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

That looks good.

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().

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

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.

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.

Priority:Critical» Normal

Issue summary:View changes

Updated issue summary.

Assigned:agentrickard» Unassigned
Issue summary:View changes

Does anyone understand the status of this issue?

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.