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

Files: 
CommentFileSizeAuthor
#36 access_check-2095125-35.patch1.64 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,040 pass(es).
[ View ]
#34 revert.patch11.07 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,056 pass(es).
[ View ]
#23 drupal_2095125_23.patch11.06 KBXano
PASSED: [[SimpleTest]]: [MySQL] 59,276 pass(es).
[ View ]
#19 drupal_2095125_19.patch11.21 KBXano
PASSED: [[SimpleTest]]: [MySQL] 58,784 pass(es).
[ View ]
#14 drupal_2095125_14.patch12.8 KBXano
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2095125_14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 drupal_2095125_10.patch12.89 KBXano
FAILED: [[SimpleTest]]: [MySQL] 53,059 pass(es), 3,377 fail(s), and 515 exception(s).
[ View ]
#8 interdiff.txt6.91 KBXano
#8 drupal_2095125_8.patch49.49 KBXano
PASSED: [[SimpleTest]]: [MySQL] 58,659 pass(es).
[ View ]
#3 drupal_2095125_3.patch41.84 KBXano
PASSED: [[SimpleTest]]: [MySQL] 58,911 pass(es).
[ View ]
#1 drupal_2095125_1.patch40.7 KBXano
PASSED: [[SimpleTest]]: [MySQL] 58,541 pass(es).
[ View ]

Comments

Assigned:Xano» Unassigned
Status:Active» Needs review
StatusFileSize
new40.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,541 pass(es).
[ View ]

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.

StatusFileSize
new41.84 KB
PASSED: [[SimpleTest]]: [MySQL] 58,911 pass(es).
[ View ]

Moved the tests too.

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.

-- ignore --

-- ignore --

Title:Move \Drupal\Core\Access to \Drupal\Core\Routing\AccessUse access constants in every access control context
StatusFileSize
new49.49 KB
PASSED: [[SimpleTest]]: [MySQL] 58,659 pass(es).
[ View ]
new6.91 KB

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

StatusFileSize
new12.89 KB
FAILED: [[SimpleTest]]: [MySQL] 53,059 pass(es), 3,377 fail(s), and 515 exception(s).
[ View ]

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

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

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.

Status:Needs work» Needs review
StatusFileSize
new12.8 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2095125_14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Just a re-roll.

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

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

Status:Needs work» Needs review
StatusFileSize
new11.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,784 pass(es).
[ View ]

Re-roll.

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

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.

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

Status:Needs work» Needs review
StatusFileSize
new11.06 KB
PASSED: [[SimpleTest]]: [MySQL] 59,276 pass(es).
[ View ]

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.

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.

Status:Needs review» Reviewed & tested by the community

Now it's ready. :-)

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.

Issue summary:View changes

.

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;

Title:Use access constants in every access control contextChange 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

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.

StatusFileSize
new11.07 KB
PASSED: [[SimpleTest]]: [MySQL] 59,056 pass(es).
[ View ]

The patch can still be easily reverted.

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

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.64 KB
PASSED: [[SimpleTest]]: [MySQL] 59,040 pass(es).
[ View ]

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

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.

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.

Issue summary:View changes

Added dependent and related issues.

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.

Title:Change notice: Use access constants in every access control contextUse 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.