[Followup] Move node access storage to DIC

#2: node-access-dic.patch queued for re-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.

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.

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

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.

That fail seems unrelated to this patch. Odd.

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

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

There we go.

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

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

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

There we go.

Good catch by jibran.

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.

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

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

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

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

Sure, I guess this can be directly RTBC again.

Awesome, thanks @dawehner!

Fixed a space.

Candidate for the trivial patch change of the month!

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

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.

.

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

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

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.

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

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

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

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.

The interdiff is against #50.

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

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.

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

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?

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.

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

Phew, good thing this is D8.

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.

Looks great. My concerns are addressed.

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

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.

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

On it.

https://drupal.org/node/2041015

That looks good.

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.

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.

Updated issue summary.

Does anyone understand the status of this issue?

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