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

Issues that touch the same files

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Assigned: Xano » Unassigned
Status: Active » Needs review
FileSize
40.7 KB
dawehner’s picture

Issue tags: +WSCCI, +Stalking Crell

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

+++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
index aa058bb..71fc5ed 100644
--- a/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php

--- a/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php
+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php
index ab02aec..936a614 100644
--- a/core/tests/Drupal/Tests/Core/Access/DefaultAccessCheckTest.php

--- a/core/tests/Drupal/Tests/Core/Access/DefaultAccessCheckTest.php
+++ b/core/tests/Drupal/Tests/Core/Access/DefaultAccessCheckTest.php

If we move the classes we should also move the tests as the test folder structure mirrors the one from the actual code.

Xano’s picture

FileSize
41.84 KB

Moved the tests too.

tim.plunkett’s picture

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

dawehner’s picture

-- ignore --

dawehner’s picture

-- ignore --

Xano’s picture

.

Xano’s picture

Title: Move \Drupal\Core\Access to \Drupal\Core\Routing\Access » Use access constants in every access control context
FileSize
49.49 KB
6.91 KB
tim.plunkett’s picture

+++ b/core/core.services.yml
@@ -382,12 +382,12 @@ services:
-    class: Drupal\Core\Access\CsrfTokenGenerator
+    class: Drupal\Core\Routing\Access\CsrfTokenGenerator
...
-    class: Drupal\Core\Access\AccessManager
+    class: Drupal\Core\Routing\Access\AccessManager

@@ -398,7 +398,7 @@ services:
-    class: Drupal\Core\Access\DefaultAccessCheck
+    class: Drupal\Core\Routing\Access\DefaultAccessCheck

Let's skip these moves in this issue. Just the interface restructuring for now

Xano’s picture

FileSize
12.89 KB

Status: Needs review » Needs work
Issue tags: -DX (Developer Experience), -WSCCI, -Stalking Crell

The last submitted patch, drupal_2095125_10.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review

#10: drupal_2095125_10.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +DX (Developer Experience), +WSCCI, +Stalking Crell

The last submitted patch, drupal_2095125_10.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
12.8 KB

Just a re-roll.

Status: Needs review » Needs work

The last submitted patch, drupal_2095125_14.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
Xano’s picture

#14: drupal_2095125_14.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +DX (Developer Experience), +WSCCI, +Stalking Crell

The last submitted patch, drupal_2095125_14.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
11.21 KB

Re-roll.

Crell’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Access/AccessibleInterface.php
    @@ -0,0 +1,31 @@
    +  /**
    +   * Checks data value access.
    +   *
    +   * @param string $operation
    +   *   The operation to be performed.
    +   * @param \Drupal\Core\Session\AccountInterface $account
    +   *   (optional) The user for which to check access, or NULL to check access
    +   *   for the current user. Defaults to NULL.
    +   *
    

    It looks like some documentation got lost here. Was that deliberate?

  2. +++ b/core/lib/Drupal/Core/Routing/Access/AccessInterface.php
    @@ -0,0 +1,32 @@
    +interface AccessInterface extends FagoLikesThisInterface {
    

    Sorry, no. :-)

Xano’s picture

It looks like some documentation got lost here. Was that deliberate?

Yes, because we want to support any operation and not just CRUD.

Sorry, no. :-)

Hihi. I forgot that code sprint joke was still in there.

Crell’s picture

OK, let's fix the inside joke (sorry, fago) and then I think this is good to go.

Xano’s picture

Status: Needs work » Needs review
FileSize
11.06 KB

Re-roll and I re-aliased the interface to GenericAccessInterface.

Status: Needs review » Needs work
Issue tags: -DX (Developer Experience), -WSCCI, -Stalking Crell

The last submitted patch, drupal_2095125_23.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
Issue tags: +DX (Developer Experience), +WSCCI, +Stalking Crell

#23: drupal_2095125_23.patch queued for re-testing.

Re-testing as I cannot reproduce the failures locally.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Now it's ready. :-)

Xano’s picture

#23: drupal_2095125_23.patch queued for re-testing.

Xano’s picture

#23: drupal_2095125_23.patch queued for re-testing.

Xano’s picture

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

Xano’s picture

Xano’s picture

Issue summary: View changes

.

alexpott’s picture

Issue tags: +API change

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

-use Drupal\Core\TypedData\AccessibleInterface;
+use Drupal\Core\Access\AccessibleInterface;
alexpott’s picture

Title: Use access constants in every access control context » Change notice: Use access constants in every access control context
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record, +Approved API change

Discussed with @catch on IRC he said:

Committed 5085cb3 and pushed to 8.x. Thanks!

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

dawehner’s picture

In the little land of route access we do now have three interfaces

  • Drupal\Core\Access\StaticAccessInterface providing appliesTo()
  • Drupal\Core\Access\AccessInterface providing applies()
  • Drupal\Core\Routing\Access\AccessCheckinterface provides access()

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.

dawehner’s picture

FileSize
11.07 KB

The patch can still be easily reverted.

tim.plunkett’s picture

Status: Active » Reviewed & tested by the community

Crell, dawehner, and I all completely missed the implication of this change on routing access checkers. This needs more discussion.
RTBC for revert

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.64 KB

Alternative we let the first two interfaces extend the last one.

Xano’s picture

Status: Needs review » Reviewed & tested by the community

Crell, dawehner, and I all completely missed the implication of this change on routing access checkers.

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

webchick’s picture

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

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

webchick’s picture

Issue summary: View changes

Added dependent and related issues.

xjm’s picture

Issue tags: +Missing change record

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

dawehner’s picture

xjm’s picture

Title: Change notice: Use access constants in every access control context » Use access constants in every access control context
Status: Active » Fixed
Issue tags: -Needs change record, -Missing change record

Alright, so safe to just mark this fixed then?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.