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.
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.
Comment | File | Size | Author |
---|---|---|---|
#72 | 65-67-interdiff.txt | 2.08 KB | alexpott |
#72 | 1921426_67.patch | 38.5 KB | alexpott |
#70 | 63-65-interdiff.txt | 32.2 KB | alexpott |
#70 | 1921426_65.patch | 38.49 KB | alexpott |
#68 | 1921426_65.patch | 37.15 KB | chx |
Comments
Comment #1
marcingy CreditAttribution: marcingy commentedReal 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.
Comment #2
marcingy CreditAttribution: marcingy commentedHere is a patch, tests pass locally.
Comment #4
marcingy CreditAttribution: marcingy commentedDoes not fail locally so retesting
Comment #5
marcingy CreditAttribution: marcingy commented#2: node-access-dic.patch queued for re-testing.
Comment #6
chx CreditAttribution: chx commented#2: node-access-dic.patch queued for re-testing.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #9
marcingy CreditAttribution: marcingy commentedRerolled to keep up with head changes
Comment #10
agentrickardI'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.
Comment #11
marcingy CreditAttribution: marcingy commentedI 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.
Comment #12
agentrickardThat seems a little abrupt. We're just discussing _preferences_.
Comment #13
agentrickardHere'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.
Comment #14
agentrickardSigh. 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.
Comment #16
agentrickardThat fail seems unrelated to this patch. Odd.
Comment #17
xjm#13: node-access-dic_13.patch queued for re-testing.
Comment #19
agentrickardWell, I suspect that fail tells us that node_access_rebuild() didn't work properly. Fixing.
Comment #20
agentrickardThere we go.
Comment #21
andypost#20: node-access-dic_20.patch queued for re-testing.
Comment #22
fago#20: node-access-dic_20.patch queued for re-testing.
Comment #24
dawehnerLet's add an interface for all this public methods and rerole it.
Comment #25
dawehnerThere should be some more injection going on.
THis should better use Drupal::entityManager()->getAccessController('node')
Comment #26
dawehnerThere we go.
Comment #27
dawehnerGood catch by jibran.
Comment #28
jibranThanks for the reroll.
#25.2 is still pending.
EntityControllerInterface also NodeAccessControllerInterface
Typehint missing.
Same here for $tables and $account
Here too.
Comment #29
dawehnerThanks for the good review, here are fixes for everything.
Comment #30
jibranI am trying to improve my reviewing skills so please bare with me.
Not required.
NodeInterface namespace missing.
Should be NodeInterface
Comment #31
dawehnerThat's a good review. The point two, though is not valid. because it is not documentation.
Comment #32
jibranThank you @dawehner. I think it is ready to fly.
Comment #33
tstoecklerCan we have a quick re-roll for the missing indentation (and new-line, actually)?
Comment #34
dawehnerSure, I guess this can be directly RTBC again.
Comment #35
dawehnerComment #36
tstoecklerAwesome, thanks @dawehner!
Comment #37
chx CreditAttribution: chx commentedFixed a space.
Comment #38
dawehnerCandidate for the trivial patch change of the month!
Comment #39
alexpottNeeds a reroll...
Comment #40
agentrickardHere'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.
Comment #41
dawehnerComment #42
effulgentsia CreditAttribution: effulgentsia commented.
Comment #43
effulgentsia CreditAttribution: effulgentsia commented#42 just intended to remove the "Needs reroll" tag, not the other one.
Comment #44
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #45
alexpottI 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...
Comment #46
amateescu CreditAttribution: amateescu commentedI agree with #45. Also, a proper query-time (storage) entity access will allow us to remove a lot of fugly code from Entity reference.
Comment #47
dawehnerMoved to a grant storage controller.
Comment #48
tstoecklerThat should be ...GrantStorage... not ...GrandStorage...
Comment #49
chx CreditAttribution: chx commentedThat's quite a change, but I think we are OK.
I suspect this is incorrect. Or if it isn't , some comments are in order.
Comment #50
dawehnerOh right, so we need a new method...
Comment #51
chx CreditAttribution: chx commentedSniff, 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.
Comment #53
chx CreditAttribution: chx commentedThe interdiff is against #50.
Comment #55
amateescu CreditAttribution: amateescu commentedI 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..
Comment #56
dawehnerWell 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? )
Comment #57
amateescu CreditAttribution: amateescu commentedYes, a EntityQueryAccessControllerInterface is also fine because it ties it to EntityQuery, which is exactly what we want :)
Comment #58
dawehnerAs 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.
Comment #60
chx CreditAttribution: chx commentedComment #61
chx CreditAttribution: chx commentedMore stuff moved into a class, injected, stuff: node_access_write_grants and node_access_acquire_grants is dead.
Comment #63
chx CreditAttribution: chx commentedNext 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.)
Comment #64
msonnabaum CreditAttribution: msonnabaum commentedI 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?
Comment #66
chx CreditAttribution: chx commentedIf 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.
Comment #68
chx CreditAttribution: chx commentedSee I can't even roll a patch properly. Interdiff against #63.
Comment #69
msonnabaum CreditAttribution: msonnabaum commentedPhew, good thing this is D8.
Comment #70
alexpottPatch 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.
Comment #71
msonnabaum CreditAttribution: msonnabaum commentedLooks great. My concerns are addressed.
Comment #72
alexpottMinor docs improvements
Comment #73
catchSorry 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.
Comment #74
alexpott@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
Comment #75
catchThe 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.
Comment #76
alexpottPretty sure we need a change notice here...
Change notice material...
Comment #77
agentrickardOn it.
Comment #78
agentrickardhttps://drupal.org/node/2041015
Comment #79
chx CreditAttribution: chx commentedThat looks good.
Comment #80
PanchoDocs and change notice need to be corrected, after #61 completely removed node_access_write_grants() and node_access_acquire_grants():
Besides some code comments, the change notice refers to these nonexisting functions:
Comment #81
agentrickardWhich version of this patch was committed?!? The change notice was written from #72.
Comment #82
agentrickardUpdated 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.
Comment #83
xjmOh 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.
Comment #84
xjmComment #84.0
xjmUpdated issue summary.
Comment #85
agentrickardDoes anyone understand the status of this issue?
Comment #86
agentrickardPer xjm in IRC, it lloks like the outstanding concerns can be addressed in #1825984: Separate concerns for node access "acquire" and "write" actions.