Views needs a access check class, which allows you to check access by role. It's certainly not really helpful
for code-only routes, though if you configure for example a View people really often uses role based access, primarily
because you don't have an UI for adding new permissions, just for adding roles.

Files: 
CommentFileSizeAuthor
#61 drupal-1956896-61.patch12.68 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,421 pass(es).
[ View ]
#61 interdiff.txt1.99 KBdawehner
#60 drupal-1956896-60.patch11.89 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,361 pass(es).
[ View ]
#60 interdiff.txt7.17 KBdawehner
#55 drupal-1956896-55.patch7.01 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,443 pass(es).
[ View ]
#55 interdiff.txt550 bytesdawehner
#50 49-50-interdiff.txt395 bytesalexpott
#50 drupal-1956896-50.patch7.03 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,372 pass(es).
[ View ]
#49 48-49-interdiff.txt665 bytesalexpott
#49 drupal-1956896-49.patch7.03 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,460 pass(es).
[ View ]
#48 41-48-interdiff.txt2.99 KBalexpott
#48 drupal-1956896-48.patch6.93 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,396 pass(es).
[ View ]
#44 drupal-1956896-41.patch7.66 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,412 pass(es).
[ View ]
#43 result.png22.8 KBdawehner
#38 views-role-access-1956896-38.patch8.58 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 56,014 pass(es).
[ View ]
#38 views-role-access-1956896-38-interdiff.txt943 bytesklausi
#35 drupal-1956896-35.patch7.66 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 54,154 pass(es).
[ View ]
#27 interdiff.txt2.62 KBdawehner
#27 drupal-1956896-27.patch7.87 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1956896-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 drupal-1956896-20.patch7.61 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 54,091 pass(es).
[ View ]
#20 interdiff.txt3.09 KBdawehner
#17 drupal-1956896-17.patch7.47 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 53,964 pass(es).
[ View ]
#17 interdiff.txt740 bytesdawehner
#15 drupal-1956896-15.patch7.49 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 53,849 pass(es).
[ View ]
#15 interdiff.txt733 bytesdawehner
#13 drupal-1956896-13.patch7.48 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 53,865 pass(es).
[ View ]
#13 interdiff.txt1.3 KBdawehner
#11 drupal-1956896-11.patch7.18 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 53,869 pass(es).
[ View ]
#11 interdiff.txt1.2 KBdawehner
#9 interdiff.txt2.26 KBdawehner
#9 drupal-1956896-9.patch7.08 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 53,702 pass(es), 0 fail(s), and 15 exception(s).
[ View ]
#8 drupal-1956896-8.patch7.99 KBmsonnabaum
FAILED: [[SimpleTest]]: [MySQL] 53,749 pass(es), 0 fail(s), and 14 exception(s).
[ View ]
#3 drupal-1956896-2.patch7.87 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 53,719 pass(es).
[ View ]
#1 drupal-1956896-1.patch7.47 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 53,813 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new7.47 KB
PASSED: [[SimpleTest]]: [MySQL] 53,813 pass(es).
[ View ]

Here is a first patch with a test, though I would love to not use a WebTest for that.

Status:Needs review» Needs work

Nice work. I know you have a couple of todo things in there, but hey, reviews are always welcome right? :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouterRoleTest.phpundefined
@@ -0,0 +1,102 @@
+    // Setup one user with the first role, one with the second, one with both

Nice doc, I would just use 'Create' instead of 'Setup'?

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouterRoleTest.phpundefined
@@ -0,0 +1,102 @@
+    $all_uids[] = $account_1->id();
...
+    $all_uids[] = $account_2->id();
...
+    $all_uids[] = $account_12->id();
...
+    $all_uids[] = $account_none->id();

I get this is used below. I think it might be better to just do $all_uids = array_keys($accounts); after you've created the accounts?

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouterRoleTest.phpundefined
@@ -0,0 +1,102 @@
+    $account_12 = $this->drupalCreateUser();

I get this is account 1 and 2 kind of thing, but would just account 3 be better?

+++ b/core/modules/system/tests/modules/router_test/lib/Drupal/router_test/TestControllers.phpundefined
@@ -46,4 +46,8 @@ public function test8() {
+  public function testRole() {

Needs a docblock.

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.phpundefined
@@ -0,0 +1,47 @@
+   * Implements AccessCheckInterface::applies().

We are still using the full namespaces?

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.phpundefined
@@ -0,0 +1,47 @@
+    $roles = array_keys($account->roles);

Not sure if we are bothering, but should we use $account->get('roles')?

Status:Needs work» Needs review
StatusFileSize
new7.87 KB
PASSED: [[SimpleTest]]: [MySQL] 53,719 pass(es).
[ View ]

Wow this was a long route ;)

Additional this is so full of hacks, so I'm wondering whether it's worth to do now, but with #1620010: Move LANGUAGE constants to the Language class this hacks in bootstrap.inc aren't required anymore.

Tried to actually get a working kernel + access manager etc, but then I realized all what's needed is to use the RoleAccessCheck() class directly.

Status:Needs review» Needs work

I didn't checked the review in #2, sorry

I'm waiting for WSCCI to tell whether the hack in bootstrap.inc is okay.

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.php
@@ -0,0 +1,151 @@
+  /**
+   * Tests role requirements on routes.
+   */
+  public function testRoleAccess() {

PHPUnit has annotations that can let you inject all of this setup code, including getTestRouteCollection(), from other methods. That would keep the test cleaner, IMO.

We could also wait until #1825332: Introduce an AccountInterface to represent the current user is in, all this removes the requirement for creating User Entities.

StatusFileSize
new7.99 KB
FAILED: [[SimpleTest]]: [MySQL] 53,749 pass(es), 0 fail(s), and 14 exception(s).
[ View ]

Here's a version that uses @dataProvider like Crell suggested. It's not a *huge* win here because the setup is still a bit complex, but it does make the assertion method much simpler and we could probably use more examples of this in core.

I also removed the bootstrap.inc include. That's verboten in UnitTestCase. Instead, I just defined the one language constant that User needs.

Status:Needs work» Needs review
StatusFileSize
new7.08 KB
FAILED: [[SimpleTest]]: [MySQL] 53,702 pass(es), 0 fail(s), and 15 exception(s).
[ View ]
new2.26 KB

Oh right this works also. Then please let's also remove this bootstrap.inc hack.

Status:Needs review» Needs work

The last submitted patch, drupal-1956896-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.2 KB
new7.18 KB
PASSED: [[SimpleTest]]: [MySQL] 53,869 pass(es).
[ View ]

This exceptions happen if you execute these tests via simpletest.

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.php
@@ -0,0 +1,157 @@
+    // Setup expected values, so which path can be access by which user.

Wouldn't this be "say which path"? In which case it should be preceeded by a semi-colon, not comma.

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.php
@@ -0,0 +1,157 @@
+  public function accountsToDeny($grant_users) {

Needs docblock.

Other than those nits, this is looking good.

StatusFileSize
new1.3 KB
new7.48 KB
PASSED: [[SimpleTest]]: [MySQL] 53,865 pass(es).
[ View ]

Fixed the documentation.

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.php
@@ -106,7 +106,7 @@ public function testRoleAccessProvider() {
+    // Setup expected values; so which path can be access by which user.

This still doesn't make sense grammatically. "Setup expected values; specify which paths can be accessed by which user." would be what you're looking for, I think.

StatusFileSize
new733 bytes
new7.49 KB
PASSED: [[SimpleTest]]: [MySQL] 53,849 pass(es).
[ View ]

Respect, this has been exactly 80 chars :)

Looking pretty good.

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.phpundefined
@@ -0,0 +1,47 @@
+    $roles = array_keys($account->roles);

Looks like we don't really need to use a $role variable, can just be in the array_diff?

StatusFileSize
new740 bytes
new7.47 KB
PASSED: [[SimpleTest]]: [MySQL] 53,964 pass(es).
[ View ]

Yeah we could do it. No idea which of these two alternatives are easier to read.

Issue tags:+WSCCI

Adding the tag.

Status:Needs review» Needs work

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.phpundefined
@@ -0,0 +1,46 @@
+   * Implements AccessCheckInterface::applies().
...
+   * Implements AccessCheckInterface::access().

Either use the new {@inheritdoc} or the full \Drupal\... part, but not this :)

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.phpundefined
@@ -0,0 +1,46 @@
+    $diff = array_diff(array_filter($rids), array_keys($account->roles));
+    if (empty($diff)) {
+      return TRUE;
+    }
+    else {
+      return NULL;

If a user doesn't have a role, shouldn't they be denied access? NULL means "no opinion", that doesn't seem to be correct here. So replace with return empty($diff);?

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.phpundefined
@@ -0,0 +1,167 @@
+        '_controller' => '\Drupal\router_test\TestControllers::test1'
+      ),
+      array(
+        '_role' => 'role_test_1',
+      )
+    ));
+    $route_collection->add('role_test_2', new Route('/role_test_2',
+      array(
+        '_controller' => '\Drupal\router_test\TestControllers::test1'
+      ),
+      array(
+        '_role' => 'role_test_2',
+      )
+    ));
+    $route_collection->add('role_test_3', new Route('/role_test_3',
+      array(
+        '_controller' => '\Drupal\router_test\TestControllers::test1'
+      ),
+      array(
+        '_role' => 'role_test_1, role_test_2',
+      )
+    ));

Missing some trailing commas here.

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.phpundefined
@@ -0,0 +1,167 @@
+  public function accountsToDeny($grant_users) {

Is making this public a PHPUnit thing, or can it be protected?

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.phpundefined
@@ -0,0 +1,167 @@
+//    return array_diff_assoc($this->accounts, $grant_users);

Delete this?

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.phpundefined
@@ -0,0 +1,167 @@
+   * @dataProvider testRoleAccessProvider

I don't know what this is. Missing param.

Status:Needs work» Needs review
StatusFileSize
new3.09 KB
new7.61 KB
PASSED: [[SimpleTest]]: [MySQL] 54,091 pass(es).
[ View ]

Thanks for your review!

If a user doesn't have a role, shouldn't they be denied access? NULL means "no opinion", that doesn't seem to be correct here. So replace with return empty($diff);?

Yeah I totally wasn't sure about that, so I went with the way PermissionAccessCheck works. http://api.drupal.org/api/drupal/core!modules!user!lib!Drupal!user!Acces...

I don't know what this is. Missing param.

That's pretty cool stuff: http://www.phpunit.de/manual/3.6/en/appendixes.annotations.html#appendix...

I disagree. Why should we force-deny if the role doesn't match? Most of the access checkers are TRUE/NULL, and don't return FALSE. Remember, if a checker returns false then NOTHING can grant access after that. FALSE is terminal.

Well, this comes back to the fact that access checkers aren't ANDed together. So if you want to say "This route must meet both of these access checkers", there is no way to do that. (that I can see).

True, but that's by design. Individual checkers shouldn't declare themselves the Final Authority(tm) without very good reason. I don't see a reason for generic role checks to be a special snowflake.

I agree with crell. If we return FALSE, you can't connect multiple one. I guess in the custom world there seems to be a lot of usecases for that.

Status:Needs review» Needs work

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php
@@ -0,0 +1,47 @@
+/**
+ * Determines access to routes based on roles defined via hook_permission().

wat? hook_permission() does not define roles?

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php
@@ -0,0 +1,47 @@
+    else {
+      return NULL;

That could use a comment why we return NULL instead of false. Yes, it is already documented elsewhere what NULL means, but I think we need to repeat it more often so that people get it.

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.php
@@ -0,0 +1,173 @@
+
+// Needed because the Entity class calls it.
+if (!defined('LANGUAGE_NOT_SPECIFIED')) {
+  define('LANGUAGE_NOT_SPECIFIED', 'und');
+}

What does the entity class call? A constant cannot be called? In what case is this constant not available?

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.php
@@ -0,0 +1,173 @@
+      'description' => 'Function Tests for the routing role system.',

Should be "Test for the role based access checker in the routing system."

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.php
@@ -0,0 +1,173 @@
+  /**
+   * Provides data for the role access test.
+   */
+  public function testRoleAccessProvider() {

So this is not a test method. Then it should at least have a reference or @see to the actual test method that uses it. Is there no PHPUnit doc standard for that?

Dataprovider could have a prefix other than "test".

Status:Needs work» Needs review
StatusFileSize
new7.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1956896-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.62 KB

Thanks for the reviews!

What does the entity class call? A constant cannot be called? In what case is this constant not available?

... Everything needs to be autoloadable or not usuable at all. Added a todo and improved the comment.

Adapted all the other comments as well.

Is there an issue on that exception Incorrect integer value: '' for column 'line' at row 3: INSERT INTO {simpletest}..?

Title:Add role based access checker.Add role based access checker

Sorry.

@Olli
Not that I know of.

Status:Needs review» Reviewed & tested by the community

Looks RTBC to me now!

Status:Reviewed & tested by the community» Needs review

Issue tags:-WSCCI, -VDC

#27: drupal-1956896-27.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+WSCCI, +VDC

The last submitted patch, drupal-1956896-27.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.66 KB
PASSED: [[SimpleTest]]: [MySQL] 54,154 pass(es).
[ View ]

No autocompletion in the bundle anymore :(

From the testbot log:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'line' at row 3: INSERT INTO {simpletest} (test_id, test_class, status, message, message_group, function, line, file) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7), (:db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13, :db_insert_placeholder_14, :db_insert_placeholder_15), (:db_insert_placeholder_16, :db_insert_placeholder_17, :db_insert_placeholder_18, :db_insert_placeholder_19, :db_insert_placeholder_20, :db_insert_placeholder_21, :db_insert_placeholder_22, :db_insert_placeholder_23); Array
(
    [:db_insert_placeholder_0] => 825
    [:db_insert_placeholder_1] => Drupal\Tests\Component\PhpStorage\FileStorageTest
    [:db_insert_placeholder_2] => pass
    [:db_insert_placeholder_3] =>
    [:db_insert_placeholder_4] => Other
    [:db_insert_placeholder_5] => Drupal\Tests\Component\PhpStorage\FileStorageTest->testCRUD()
    [:db_insert_placeholder_6] => 42
    [:db_insert_placeholder_7] => /var/lib/drupaltestbot/sites/default/files/checkout/core/tests/Drupal/Tests/Component/PhpStorage/FileStorageTest.php
    [:db_insert_placeholder_8] => 825
    [:db_insert_placeholder_9] => Drupal\Tests\Component\PhpStorage\FileStorageTest
    [:db_insert_placeholder_10] => pass
    [:db_insert_placeholder_11] =>
    [:db_insert_placeholder_12] => Other
    [:db_insert_placeholder_13] => Drupal\Tests\Component\PhpStorage\FileStorageTest->testReadOnly()
    [:db_insert_placeholder_14] => 51
    [:db_insert_placeholder_15] => /var/lib/drupaltestbot/sites/default/files/checkout/core/tests/Drupal/Tests/Component/PhpStorage/FileStorageTest.php
    [:db_insert_placeholder_16] => 825
    [:db_insert_placeholder_17] =>
    [:db_insert_placeholder_18] => pass
    [:db_insert_placeholder_19] =>
    [:db_insert_placeholder_20] => Other
    [:db_insert_placeholder_21] => ->Drupal\Tests\Core\Route\RouterRoleTest::testRoleAccess()
    [:db_insert_placeholder_22] =>
    [:db_insert_placeholder_23] =>
)
in Drupal\Core\Database\Connection->query() (line 554 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php).].

I have no idea what it's complaining about...

That sounds like those special PHP exceptions/warnings that don't have a line number, for example http://stackoverflow.com/questions/9076899/notice-unknown-on-line-0-how-...

StatusFileSize
new943 bytes
new8.58 KB
PASSED: [[SimpleTest]]: [MySQL] 56,014 pass(es).
[ View ]

Making simpletest more robust to see the actual error.

Issue tags:-WSCCI, -VDC

#38: views-role-access-1956896-38.patch queued for re-testing.

Issue tags:+WSCCI, +VDC

#38: views-role-access-1956896-38.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

I'm not sure if that first hunk really belongs, but this whole thing looks pretty great!

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new22.8 KB

Uploading a recent version + a screenshot from the simpletest page.

StatusFileSize
new7.66 KB
PASSED: [[SimpleTest]]: [MySQL] 55,412 pass(es).
[ View ]

urgs.

Status:Needs review» Reviewed & tested by the community

Is the first hook related to the broken phpunit reporting that got fixed in #1969406: PHPUnit Test asserting logging broken?

@Berdir
Yes, the patch from klausi has been just for debugging.

#44: drupal-1956896-41.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs review
Issue tags:+LONDON_2013_APRIL
StatusFileSize
new6.93 KB
PASSED: [[SimpleTest]]: [MySQL] 55,396 pass(es).
[ View ]
new2.99 KB

I don't think data providers should be providing data to the test through a class property... looks weird to me... about this?

StatusFileSize
new7.03 KB
PASSED: [[SimpleTest]]: [MySQL] 55,460 pass(es).
[ View ]
new665 bytes

Adding missing @param :(

StatusFileSize
new7.03 KB
PASSED: [[SimpleTest]]: [MySQL] 55,372 pass(es).
[ View ]
new395 bytes

@dawehner noticed a spurious blank line

I personally think this patch is now much easier to read with the changes of alex.

Status:Needs review» Reviewed & tested by the community

Nice changes! Still RTBC.

Nice example but without usage. Maybe better introduce it's usage within patch?

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php
@@ -0,0 +1,47 @@
+    // If there is no allowed role, return NULL to give other checks a chance.
+    else {
+      return NULL;
+    }

this could be removed, 'else' not needed

Thanks for your review.

Nice example but without usage. Maybe better introduce it's usage within patch?

The problem is that I doubt that there is any usage in core beside views and maybe SCOTCH. You know, as a module developer I think it's bad practice to rely on roles, but it's a useful feature elsewhere, so I can't find an actual usecase.

Regarding the else bit, I think it's helpful to explain why NULL is returned. This also matches the documentation of PermissionAccessCheck.

StatusFileSize
new550 bytes
new7.01 KB
PASSED: [[SimpleTest]]: [MySQL] 55,443 pass(es).
[ View ]

Removed the else

Thanx a lot , RTBC as it was

Can we document on the checker class that it is an "all" check, not an "any" check (vis, you can specify multiple roles, but you then need all of them)? I had to read the code 4 times to determine that it was an "all" check... and even then I'm not 100% certain of that. :-)

Probably just something in the class's docblock is sufficient.

It's indeed a good question whether we want to have both behavior? all and any. Does someone has an oppinion about that?

Well doesn't the answer to #58 depend on what @andypost has been asking for... use cases :)

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new7.17 KB
new11.89 KB
PASSED: [[SimpleTest]]: [MySQL] 55,361 pass(es).
[ View ]

I renamed the test file to match it with the classname. I think that's an improvement.

Additional I implemented some logic, how to connect with OR and AND.

StatusFileSize
new1.99 KB
new12.68 KB
PASSED: [[SimpleTest]]: [MySQL] 55,421 pass(es).
[ View ]

Let's extend the test coverage to test for non-trimmed values.

Alex asked why #60 changed phpunit_error.xml.

When #1963022: Problem with PHPUnit test results when using @dataProvider was written I used this issue to generate a proper phpunit_error.xml. On that time I had named the PHPUnit Test "RouteRoleTest",
so that also landed in the phpunit_error.xml file. Note: the RouteRoleTest did not get added by that patch, just the example phpunit_error.xml.

On #60 of this issue I realized: Let's just make the test class named as parallel as possible to the actual access checker,
so there is now RoleAccessCheck and RoleAccessCheckTest. Just for sanity, I also changed phpunit_error.xml to point to the actual existing file.
Nothing fancy at the end.

Status:Needs review» Reviewed & tested by the community

In that case, this looks great! (That was the only part I wasn't sure of).

@dawehner++

Title:Add role based access checkerChange notice: Add role based access checker
Category:feature» task
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record, +Needs followup

So according to #1800998-75: Use route system instead of hook_menu() in Views, this is a blocker for REST stuff, which I think makes this more of a task than a feature.

Seems sensible though, and something other modules outside of views could make good use of. I'm a little concerned about adding more $GLOBALS['user'] to our code, but I guess if that could be refactored out here, it would. Is this blocked on #1825332: Introduce an AccountInterface to represent the current user or..?

When I reviewed earlier with Alex, Jess, and Moshe, Jess had a concern about whether or not "," and "+" were clear enough for OR or AND. Let's spin off a follow-up to discuss whether we can keep that as-is or need to change it. Personally, I like it because it's what taxonomy does/used to do when combining multiple terms into a single URL.

Committed and pushed to 8.x. Thanks!

This will need a change notice to inform people of the new check.

Status:Active» Needs review

Thanks for finally getting this in!

I added a small bit to the more or less central route access change notice: http://drupal.org/node/1851490/revisions/view/2461630/2675054

Seems sensible though, and something other modules outside of views could make good use of. I'm a little concerned about adding more $GLOBALS['user'] to our code, but I guess if that could be refactored out here, it would. Is this blocked on #1825332: Introduce an AccountInterface / UserInterface interface for use in \Drupal\Core namespaces or..?

I don't 100% follow the plans to get rid of global user, but I guess the plan is to get the AccountInterface and place it on the request object at some point.
From there the access checkers would have access to it.

When I reviewed earlier with Alex, Jess, and Moshe, Jess had a concern about whether or not "," and "+" were clear enough for OR or AND. Let's spin off a follow-up to discuss whether we can keep that as-is or need to change it. Personally, I like it because it's what taxonomy does/used to do when combining multiple terms into a single URL.

Yeah indeed, this let's me always think a bit which one is which.

I opened an issue for that: #1986640: Support AND/OR conjunctions for permission checks

I would add a simple example. :)

Good idea.

Title:Change notice: Add role based access checkerAdd role based access checker
Status:Needs review» Fixed

Looks good.

The change notice is getting long and somewhat hard to scan, we might want to shorten it down and instead have a documentation page and just link to that to describe the different default access checkers. There' also the entity access one that has a separate change notice. But putting it in there means it is written down and someone who is better a structuring/writing documentation than us developers can do that :) That's what those checkboxes at the bottom are for...

Status:Fixed» Closed (fixed)

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

Priority:Normal» Major
Status:Closed (fixed)» Active

Whatever change notice was updated needs a reference to this issue.

Also please untag when the change notice is complete.

Status:Active» Fixed

Added

Issue tags:-Needs change record

Thanks! and removing the tag... :)

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