Merge pull request #476 from driskell/fix-entity-query-type-cache

fix: Fix unpredictable false positive/negative of missing entity access check in function scope due to cache key conflict

Author: mglaman

src/Type/EntityQuery/EntityQueryType.php

@@ -17,9 +17,20 @@ public function hasAccessCheck(): bool
 
     public function withAccessCheck(): self
     {
-        $type = clone $this;
+        // The constructor of ObjectType is under backward compatibility promise.
+        // @see https://phpstan.org/developing-extensions/backward-compatibility-promise
+        // @phpstan-ignore-next-line
+        $type = new static(
+            $this->getClassName(),
+            $this->getSubtractedType(),
+            $this->getClassReflection()
+        );
         $type->hasAccessCheck = true;
-
         return $type;
     }
+
+    protected function describeAdditionalCacheKey(): string
+    {
+        return $this->hasAccessCheck ? 'with-access-check' : 'without-access-check';
+    }
 }

tests/src/Rules/EntityQueryHasAccessCheckRuleTest.php

@@ -86,5 +86,15 @@ public function cases(): \Generator
             [__DIR__ . '/data/bug-437.php'],
             []
         ];*/
+
+        yield 'bug-475.php' => [
+            [__DIR__.'/data/bug-475.php'],
+            []
+        ];
+
+        yield 'bug-475b.php' => [
+            [__DIR__.'/data/bug-475b.php'],
+            []
+        ];
     }
 }

tests/src/Rules/data/bug-475.php

@@ -0,0 +1,21 @@
+<?php
+
+/**
+ * Cache key differs outside of class so this is separate reproduction
+ *
+ * @return void
+ */
+function bug475Caching(): void
+{
+    // Here condition() return type will be cached as one that has no access check
+    \Drupal::entityQuery('node')
+        ->condition('field_test', 'foo', '=')
+        ->accessCheck(FALSE)
+        ->execute();
+
+    // Cache return on condition() will also be no access check, even though caller did, unless caller type changes cache key
+    \Drupal::entityQuery('node')
+        ->accessCheck(FALSE)
+        ->condition('field_test', 'foo', '=')
+        ->execute();
+}

tests/src/Rules/data/bug-475b.php

@@ -0,0 +1,26 @@
+<?php
+
+namespace Bug475CacheKeyExample;
+
+/**
+ * Cache key differs if in class so this tests that too
+ *
+ * @return void
+ */
+class TestClass
+{
+    public function bug475Caching(): void
+    {
+        // Here condition() return type will be cached as one that has no access check
+        \Drupal::entityQuery('node')
+            ->condition('field_test', 'foo', '=')
+            ->accessCheck(FALSE)
+            ->execute();
+
+        // Cache return on condition() will also be no access check, even though caller did, unless caller type changes cache key
+        \Drupal::entityQuery('node')
+            ->accessCheck(FALSE)
+            ->condition('field_test', 'foo', '=')
+            ->execute();
+    }
+}