AccessibleInterface should have nothing to do with TypedData. That was a bizarre decision.
Similarly, AccessInterface is closely tied to routes, and has its own set of constants that are very helpful to understanding the difference between FALSE and NULL.So let's please not add a 3rd system. Entity's usage of AccessibleInterface == the entity access controller...
From tim.plunkett in 1839516#95
Entity access just builds upon the typed data Accessibleinterface + have the controllers to implement it. It's not like it is a third system?
From fago in 1839516#96
AccessInterface is for routes. These are API hooks.I know that it is for routes, though the interface is not part of the routing namespace, just saying ... and this might add some clarity
From Xano and dawehner in 2057377#25
The access check interfaces and classes are in the wrong place, which causes confusion about what they should be used for. Moving them to the routing namespace will prevent this, but the ternary constants should remain, so they can be used for other code as well.
Dependent issues
- #2112023: Remove hook_node_access() in favor of hook_ENTITY_TYPE_access()
- Future issues that will be able to convert boolean access control to use ternary return values in a well-documented way.
Issues that touch the same files
Comment | File | Size | Author |
---|---|---|---|
#36 | access_check-2095125-35.patch | 1.64 KB | dawehner |
#34 | revert.patch | 11.07 KB | dawehner |
#23 | drupal_2095125_23.patch | 11.06 KB | Xano |
#19 | drupal_2095125_19.patch | 11.21 KB | Xano |
#14 | drupal_2095125_14.patch | 12.8 KB | Xano |
Comments
Comment #1
XanoComment #2
dawehnerAdds some tags which are important here.
Even you could argue that this belongs to the routing system itself this though is used also in places which are "just" related with routing like Local Tasks/Actions/Menu Links etc.
This will potentially break a lot of existing code in contrib, so the question has to be raised, whether it is worth to do it.
In general though I would agree that this belongs to the routing subsystem.
If we move the classes we should also move the tests as the test folder structure mirrors the one from the actual code.
Comment #3
XanoMoved the tests too.
Comment #4
tim.plunkettAt Prague we were just discussing reusing this interface elsewhere, or at least the ALLOW/DENY/KILL. So while this patch does improve the namespaces to match reality *right now*, it might be a little preemptive.
Comment #5
dawehner-- ignore --
Comment #6
dawehner-- ignore --
Comment #7
Xano.
Comment #8
Xano@tim.plunkett and I decided to merge #2095233: Move \Drupal\TypedData\AccessibleInterface to \Drupal\Access\AccessibleInterface into this one.
Comment #9
tim.plunkettLet's skip these moves in this issue. Just the interface restructuring for now
Comment #10
XanoComment #12
Xano#10: drupal_2095125_10.patch queued for re-testing.
Comment #14
XanoJust a re-roll.
Comment #16
XanoComment #17
Xano#14: drupal_2095125_14.patch queued for re-testing.
Comment #19
XanoRe-roll.
Comment #20
Crell CreditAttribution: Crell commentedIt looks like some documentation got lost here. Was that deliberate?
Sorry, no. :-)
Comment #21
XanoYes, because we want to support any operation and not just CRUD.
Hihi. I forgot that code sprint joke was still in there.
Comment #22
Crell CreditAttribution: Crell commentedOK, let's fix the inside joke (sorry, fago) and then I think this is good to go.
Comment #23
XanoRe-roll and I re-aliased the interface to GenericAccessInterface.
Comment #25
Xano#23: drupal_2095125_23.patch queued for re-testing.
Re-testing as I cannot reproduce the failures locally.
Comment #26
Crell CreditAttribution: Crell commentedNow it's ready. :-)
Comment #27
Xano#23: drupal_2095125_23.patch queued for re-testing.
Comment #28
Xano#23: drupal_2095125_23.patch queued for re-testing.
Comment #29
XanoAs a follow-up, we should convert NODE_ACCESS_ALLOW and sibling constants to use these newly available constants as well. This will not likely be hard to do, as most cases that use the node-specific constants also have a Node entity available, which will contain the new constants after this patch.
Comment #30
XanoSee #2112023: Remove hook_node_access() in favor of hook_ENTITY_TYPE_access().
Comment #30.0
Xano.
Comment #31
alexpottThis is an API change and should tagged as such. The changes are very minor - any contrib should just be able to replace a use statement eg...
Comment #32
alexpottDiscussed with @catch on IRC he said:
Committed 5085cb3 and pushed to 8.x. Thanks!
Change notices need updating- I found the following two but there might be more - https://drupal.org/node/1928882, https://drupal.org/node/2083471
Comment #33
dawehnerIn the little land of route access we do now have three interfaces
this means that every access checker have to implement two interfaces in order to work. This is 200% WTF especially because noone will gonna understand why for such a simple thing.
Comment #34
dawehnerThe patch can still be easily reverted.
Comment #35
tim.plunkettCrell, dawehner, and I all completely missed the implication of this change on routing access checkers. This needs more discussion.
RTBC for revert
Comment #36
dawehnerAlternative we let the first two interfaces extend the last one.
Comment #37
XanoI missed it as well, and I believe the patch from #36 takes care of the issue.
As a follow-up, we can use #2096023: Move route access checking code from \Drupal\Core\Access to \Drupal\Core\Routing\Access to move the route access checking interfaces to a better place, so the naming confusion will be taken care of.
Comment #38
webchickOk, since we have an alpha4 coming out tomorrow, which we hope will be a stable thing for module developers to work against, fast-tracking #36 to take care of that.
Committed and pushed to 8.x. Thanks!
Back to change notice. A fast turnaround on that would be great, so module devs know about this change.
Comment #38.0
webchickAdded dependent and related issues.
Comment #39
xjmTagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release
The final patch for this issue was committed on October 18, 2013, meaning that the change record updates have been outstanding for more than three months.
Comment #40
dawehnerIt does not make sense to change-notice this until #2096023: Move route access checking code from \Drupal\Core\Access to \Drupal\Core\Routing\Access is in.
Comment #41
xjmAlright, so safe to just mark this fixed then?