Problem/Motivation

\Drupal\user\Plugin\views\access\Role::access() should return a Boolean but it returns an array. In access checking types can be combined and this might produce surprising results.

Discovered as part of #2742585: Deprecate dangerous assertTrue/False() compatibility overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests

Proposed resolution

Change code to conform to \Drupal\views\Plugin\views\access\AccessPluginBase::access()

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#2 3082287-2.patch1.55 KBalexpott
#2 3082287-2-test-only.patch943 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented
Status: Active » Needs review
FileSize
3082287-2-test-only.patch943 bytes
3082287-2.patch1.55 KB

The last submitted patch, 2: 3082287-2-test-only.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Needs work 
+++ b/core/modules/user/tests/src/Functional/Views/AccessRoleTest.php
@@ -50,8 +50,8 @@ public function testAccessRole() {
-    $this->assertFalse($executable->display_handler->access($this->webUser));
-    $this->assertTrue($executable->display_handler->access($this->normalUser));
+    $this->assertSame(FALSE, $executable->display_handler->access($this->webUser));
+    $this->assertSame(TRUE, $executable->display_handler->access($this->normalUser));

Do we need a @todo to revert this to assertTrue and assertFalse in #2742585: Deprecate dangerous assertTrue/False() compatibility overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests? Other than that, RTBC I'd say

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented
Status: Needs work » Needs review

@mondrake I don't think so. i think we need a follow-up to that issue to do a find and replace on all of the $this->assertSame(FALSE, and $this->assertSame(TRUE, in core. Not worth doing in that issue imo just more noise.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Reviewed & tested by the community

OK then

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Priority: Normal » Critical

Critical as it is holding the Critical parent

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented

RTBC +1

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented

I created follow-up for #5: #3082415: Replace assert(Not)Same/Identical() on booleans with assert(Not)True/False() in PHPUnit tests.

  • larowlan committed 6f29f98 on 8.8.x 
    Issue #3082287 by alexpott: \Drupal\user\Plugin\views\access\Role::...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 6f29f98 and pushed to 8.8.x. Thanks!

c/p to 8.7.x

Thanks folks

  • larowlan committed d789219 on 8.7.x 
    Issue #3082287 by alexpott: \Drupal\user\Plugin\views\access\Role::...

Status: Fixed » Closed (fixed)

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