Issue #2938053 by Wim Leers, alexpott: AccessResult::orIf() fails to retain...

Issue #2938053 by Wim Leers, alexpott: AccessResult::orIf() fails to retain the reason if both operands are neutral or forbidden, but the first contains a reason and the second one does not

core/lib/Drupal/Core/Access/AccessResult.php

@@ -336,10 +336,10 @@ public function orIf(AccessResultInterface $other) {
         $merge_other = TRUE;
       }
 
-      if ($this->isForbidden() && $this instanceof AccessResultReasonInterface) {
+      if ($this->isForbidden() && $this instanceof AccessResultReasonInterface && !is_null($this->getReason())) {
         $result->setReason($this->getReason());
       }
-      elseif ($other->isForbidden() && $other instanceof AccessResultReasonInterface) {
+      elseif ($other->isForbidden() && $other instanceof AccessResultReasonInterface && !is_null($other->getReason())) {
         $result->setReason($other->getReason());
       }
     }
@@ -353,14 +353,13 @@ public function orIf(AccessResultInterface $other) {
       $result = static::neutral();
       if (!$this->isNeutral() || ($this->getCacheMaxAge() === 0 && $other->isNeutral()) || ($this->getCacheMaxAge() !== 0 && $other instanceof CacheableDependencyInterface && $other->getCacheMaxAge() !== 0)) {
         $merge_other = TRUE;
-        if ($other instanceof AccessResultReasonInterface) {
-          $result->setReason($other->getReason());
       }
-      }
-      else {
-        if ($this instanceof AccessResultReasonInterface) {
+
+      if ($this instanceof AccessResultReasonInterface && !is_null($this->getReason())) {
         $result->setReason($this->getReason());
       }
+      elseif ($other instanceof AccessResultReasonInterface && !is_null($other->getReason())) {
+        $result->setReason($other->getReason());
       }
     }
     $result->inheritCacheability($this);

core/tests/Drupal/Tests/Core/Access/AccessResultTest.php

@@ -269,8 +269,12 @@ public function testAndIf() {
    */
   public function testOrIf() {
     $neutral = AccessResult::neutral('neutral message');
+    $neutral_other = AccessResult::neutral('other neutral message');
+    $neutral_reasonless = AccessResult::neutral();
     $allowed = AccessResult::allowed();
     $forbidden = AccessResult::forbidden('forbidden message');
+    $forbidden_other = AccessResult::forbidden('other forbidden message');
+    $forbidden_reasonless = AccessResult::forbidden();
     $unused_access_result_due_to_lazy_evaluation = $this->getMock('\Drupal\Core\Access\AccessResultInterface');
     $unused_access_result_due_to_lazy_evaluation->expects($this->never())
       ->method($this->anything());
@@ -304,6 +308,18 @@ public function testOrIf() {
     $this->assertTrue($access->isNeutral());
     $this->assertEquals('neutral message', $access->getReason());
     $this->assertDefaultCacheability($access);
+    // Reason inheritance edge case: first reason is kept.
+    $access = $neutral->orIf($neutral_other);
+    $this->assertEquals('neutral message', $access->getReason());
+    $access = $neutral_other->orIf($neutral);
+    $this->assertEquals('other neutral message', $access->getReason());
+    // Reason inheritance edge case: one of the operands is reasonless.
+    $access = $neutral->orIf($neutral_reasonless);
+    $this->assertEquals('neutral message', $access->getReason());
+    $access = $neutral_reasonless->orIf($neutral);
+    $this->assertEquals('neutral message', $access->getReason());
+    $access = $neutral_reasonless->orIf($neutral_reasonless);
+    $this->assertNull($access->getReason());
 
     // NEUTRAL || ALLOWED === ALLOWED.
     $access = $neutral->orIf($allowed);
@@ -329,7 +345,7 @@ public function testOrIf() {
     $this->assertDefaultCacheability($access);
 
     // FORBIDDEN || NEUTRAL === FORBIDDEN.
-    $access = $forbidden->orIf($allowed);
+    $access = $forbidden->orIf($neutral);
     $this->assertFalse($access->isAllowed());
     $this->assertTrue($access->isForbidden());
     $this->assertFalse($access->isNeutral());
@@ -337,12 +353,24 @@ public function testOrIf() {
     $this->assertDefaultCacheability($access);
 
     // FORBIDDEN || FORBIDDEN === FORBIDDEN.
-    $access = $forbidden->orIf($allowed);
+    $access = $forbidden->orIf($forbidden);
     $this->assertFalse($access->isAllowed());
     $this->assertTrue($access->isForbidden());
     $this->assertFalse($access->isNeutral());
     $this->assertEquals('forbidden message', $access->getReason());
     $this->assertDefaultCacheability($access);
+    // Reason inheritance edge case: first reason is kept.
+    $access = $forbidden->orIf($forbidden_other);
+    $this->assertEquals('forbidden message', $access->getReason());
+    $access = $forbidden_other->orIf($forbidden);
+    $this->assertEquals('other forbidden message', $access->getReason());
+    // Reason inheritance edge case: one of the operands is reasonless.
+    $access = $forbidden->orIf($forbidden_reasonless);
+    $this->assertEquals('forbidden message', $access->getReason());
+    $access = $forbidden_reasonless->orIf($forbidden);
+    $this->assertEquals('forbidden message', $access->getReason());
+    $access = $forbidden_reasonless->orIf($forbidden_reasonless);
+    $this->assertNull($access->getReason());
 
     // FORBIDDEN || * === FORBIDDEN.
     $access = $forbidden->orIf($unused_access_result_due_to_lazy_evaluation);