Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#17 interdiff.txt1.8 KBdawehner
#17 access-2310705-17.patch59.07 KBdawehner
#16 access-2310705-16.patch59.1 KBtim.plunkett
#16 interdiff.txt992 bytestim.plunkett
#14 access-2310705-14.patch59.1 KBtim.plunkett
#14 interdiff.txt60.47 KBtim.plunkett
#6 interdiff-3-6.txt884 bytesl0ke
#6 access-manager-constants-any-all-2310705-6.patch13.22 KBl0ke
#3 interdiff-1-3.txt513 bytesl0ke
#3 access-manager-constants-any-all-2310705-3.patch12.94 KBl0ke
#1 access-manager-constants-any-all-2310705-1.patch12.73 KBl0ke

Comments

l0ke’s picture

l0ke
Primary language English
Location Ukraine
commented
Status: Active » Needs review
StatusFileSize
new access-manager-constants-any-all-2310705-1.patch12.73 KB

Added the constants for 'ANY'/'ALL' access modes, replaced entries.

Status: Needs review » Needs work

The last submitted patch, 1: access-manager-constants-any-all-2310705-1.patch, failed testing.

l0ke’s picture

l0ke
Primary language English
Location Ukraine
commented
StatusFileSize
new access-manager-constants-any-all-2310705-3.patch12.94 KB
new interdiff-1-3.txt513 bytes

Add missing use Drupal\Core\Access\AccessManager; to Drupal\Tests\Core\Access\CsrfAccessCheckTest.

l0ke’s picture

l0ke
Primary language English
Location Ukraine
commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
commented

Nice work so far! Let's expand the documentation a bit to have something more helpful.

  1. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -32,6 +32,16 @@ class AccessManager implements ContainerAwareInterface {
   /**
+   * All access checks should pass.
+   */
+  const ACCESS_MODE_ALL = 'ALL';

    Let's describe that all access checkers have to return AccessInterface::ALLOW 

  2. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -32,6 +32,16 @@ class AccessManager implements ContainerAwareInterface {
+
+  /**
+   * Any of access checks should pass.
+   */
+  const ACCESS_MODE_ANY = 'ANY';

    Let's describe that at least one access checker has to return AccessInterface::ALLOW and none should return AccessInterface::KILL

l0ke’s picture

l0ke
Primary language English
Location Ukraine
commented
StatusFileSize
new access-manager-constants-any-all-2310705-6.patch13.22 KB
new interdiff-3-6.txt884 bytes

@dawehner, thank you for quick response. Changing the documentation considering your notes.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Nice!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Darn, it'd be nice if we had AccessManagerInterface before making this change everywhere :\

dawehner’s picture

dawehner
Primary language German
commented

I disagree with that comment. A constant on a class is not worse than on an interface

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I agree with Tim - let's introduce the interface and just put the constants on it. Since AccessManager is a service it can be swapped out - it is great if the constants are enforced by the interface and not the class.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Assigned: Unassigned » tim.plunkett

Sorry, for causing it to be unRTBC i'll do fix it

dawehner’s picture

dawehner
Primary language German
commented
Assigned: tim.plunkett » Unassigned
Status: Needs work » Reviewed & tested by the community

There was an intention when I created the issue: #2302093: add an access manager interface

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Yeah but this could introduce a bare bones Interface to have the constants and that issue could flesh it out. Otherwise we just going to change all the same lines again.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt60.47 KB
new access-2310705-14.patch59.1 KB

Sorry @dawehner.

UGH double crosspost. Sorry for the confusion and mess.

Status: Needs review » Needs work

The last submitted patch, 14: access-2310705-14.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt992 bytes
new access-2310705-16.patch59.1 KB

Amusingly enough, doctrine also has 17 usages of "Inteface".

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new access-2310705-17.patch59.07 KB
new interdiff.txt1.8 KB

Some small details.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 4a45c16 and pushed to 8.x. Thanks!

  • alexpott committed 4a45c16 on 8.x 
    Issue #2310705 by lokeoke, tim.plunkett, dawehner: AccessManager should...

Status: Fixed » Closed (fixed)

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

m1r1k’s picture

m1r1k commented
Issue tags: +#ams2014contest