From 3f96de52a32e00e39f2d75e862fa44616e670007 Mon Sep 17 00:00:00 2001 From: Frank Ebbers Date: Fri, 12 May 2023 17:44:18 +0200 Subject: [PATCH 1/6] Added test cases. --- tests/src/Rules/RenderCallbackRuleTest.php | 11 ++++++- tests/src/Rules/data/bug-543.php | 36 ++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/tests/src/Rules/RenderCallbackRuleTest.php b/tests/src/Rules/RenderCallbackRuleTest.php index 14b966ad..01bce50e 100644 --- a/tests/src/Rules/RenderCallbackRuleTest.php +++ b/tests/src/Rules/RenderCallbackRuleTest.php @@ -161,7 +161,16 @@ public static function fileData(): \Generator yield [ __DIR__ . '/data/bug-543.php', - [] + [ + [ + "#access_callback callback array{static(Bug543\TestAccessClass), 'accessResultForbiddenNotInTrustedCallbacks'} at key '0' does not implement Drupal\Core\Security\TrustedCallbackInterface.", + 58 + ], + [ + "#access_callback callback array{static(Bug543\TestAccessClass), 'accessResultForbiddenNotInTrustedCallbacks'} at key '0' does not implement Drupal\Core\Security\TrustedCallbackInterface.", + 103 + ], + ], ]; } diff --git a/tests/src/Rules/data/bug-543.php b/tests/src/Rules/data/bug-543.php index 2eda8414..0c2eb833 100644 --- a/tests/src/Rules/data/bug-543.php +++ b/tests/src/Rules/data/bug-543.php @@ -21,10 +21,13 @@ public function providerAccessValues() { [TRUE], [AccessResult::forbidden()], [AccessResult::allowed()], + ['accessResultForbiddenNotInTrustedCallbacks'], ]; } /** + * Tests callbacks with the method names in a variable. + * * @dataProvider providerAccessValues */ public function testRenderWithAccessControllerResolved($access) { @@ -45,6 +48,10 @@ public function testRenderWithAccessControllerResolved($access) { case TRUE: $method = 'accessTrue'; break; + + case 'accessResultForbiddenNotInTrustedCallbacks': + $method = 'accessResultForbiddenNotInTrustedCallbacks'; + break; } $build = [ @@ -52,31 +59,56 @@ public function testRenderWithAccessControllerResolved($access) { ]; } + /** + * Tests callback with the actual method name. + */ public function bug543AccessResultAllowed(): void { $build = [ '#access_callback' => TestAccessClass::class . '::accessResultAllowed', ]; } + /** + * Tests callback with the actual method name. + */ public function bug543AccessResultForbidden(): void { $build = [ '#access_callback' => TestAccessClass::class . '::accessResultForbidden', ]; } + /** + * Tests callback with the actual method name. + */ public function bug543AccessFalse(): void { $build = [ '#access_callback' => TestAccessClass::class . '::accessFalse', ]; } + /** + * Tests callback with the actual method name. + */ public function bug543AccessTrue(): void { $build = [ '#access_callback' => TestAccessClass::class . '::accessTrue', ]; } + + /** + * Tests callback with the actual method name. + */ + public function bug543AccessResultForbiddenNotInTrustedCallbacks(): void { + $build = [ + '#access_callback' => TestAccessClass::class . '::accessResultForbiddenNotInTrustedCallbacks', + ]; + } + } +/** + * Test class with callbacks. + */ class TestAccessClass implements TrustedCallbackInterface { public static function accessTrue() { @@ -95,6 +127,10 @@ public static function accessResultForbidden() { return AccessResult::forbidden(); } + public static function accessResultForbiddenNotInTrustedCallbacks() { + return AccessResult::forbidden(); + } + /** * {@inheritdoc} */ From 594d352dd4373dc80174b06a581d5ae8f7bbec52 Mon Sep 17 00:00:00 2001 From: Matt Glaman Date: Wed, 28 Jun 2023 16:03:30 -0500 Subject: [PATCH 2/6] fetch methods from trustedCallbacks and inspect --- src/Rules/Drupal/RenderCallbackRule.php | 55 +++++++++++++++++++++- tests/src/Rules/RenderCallbackRuleTest.php | 10 ++-- 2 files changed, 60 insertions(+), 5 deletions(-) diff --git a/src/Rules/Drupal/RenderCallbackRule.php b/src/Rules/Drupal/RenderCallbackRule.php index 79d8121a..7e2f8afc 100644 --- a/src/Rules/Drupal/RenderCallbackRule.php +++ b/src/Rules/Drupal/RenderCallbackRule.php @@ -166,8 +166,28 @@ private function doProcessNode(Node\Expr $node, Scope $scope, string $keyChecked )->line($errorLine)->build(); } elseif (!$trustedCallbackType->isSuperTypeOf(new ObjectType($matches[1]))->yes()) { $errors[] = RuleErrorBuilder::message( - sprintf("%s callback class %s at key '%s' does not implement Drupal\Core\Security\TrustedCallbackInterface.", $keyChecked, $constantStringType->describe(VerbosityLevel::value()), $pos) + sprintf( + "%s callback class %s at key '%s' does not implement Drupal\Core\Security\TrustedCallbackInterface.", + $keyChecked, + $constantStringType->describe(VerbosityLevel::value()), + $pos + ) )->line($errorLine)->tip('Change record: https://www.drupal.org/node/2966725.')->build(); + } else { + $object = (new ObjectType($matches[1])); + if ($object->hasMethod('trustedCallbacks')->yes()) { + $allowedMethods = $this->getTrustedCallbackValues($object); + if (!in_array($matches[2], $allowedMethods, true)) { + $errors[] = RuleErrorBuilder::message( + sprintf( + "%s callback method '%s' is not present in 'trustedCallbacks' at key '%s'.", + $keyChecked, + $matches[2], + $pos + ) + )->line($errorLine)->tip('Change record: https://www.drupal.org/node/2966725.')->build(); + } + } } } } @@ -233,6 +253,21 @@ function (ClassReflection $reflection) use ($typeAndMethodName) { ) )->line($errorLine)->tip('Change record: https://www.drupal.org/node/2966725.')->build(); } + } elseif ($isTrustedCallbackInterfaceType) { + $object = $typeAndMethodName->getType(); + if ($object->hasMethod('trustedCallbacks')->yes()) { + $allowedMethods = $this->getTrustedCallbackValues($typeAndMethodName->getType()); + if (!in_array($typeAndMethodName->getMethod(), $allowedMethods, true)) { + $errors[] = RuleErrorBuilder::message( + sprintf( + "%s callback method '%s' is not present in 'trustedCallbacks' at key '%s'.", + $keyChecked, + $typeAndMethodName->getMethod(), + $pos + ) + )->line($errorLine)->tip('Change record: https://www.drupal.org/node/2966725.')->build(); + } + } } } } @@ -310,4 +345,22 @@ private function getType(Node\Expr $node, Scope $scope): Type } return $type; } + + /** + * @return string[] + */ + private function getTrustedCallbackValues(Type $type): array + { + $values = []; + foreach ($type->getObjectClassReflections() as $classReflection) { + if (!$classReflection->hasMethod('trustedCallbacks')) { + continue; + } + $values[] = $classReflection + ->getNativeReflection() + ->getMethod('trustedCallbacks') + ->invoke(null); + } + return array_merge(...$values); + } } diff --git a/tests/src/Rules/RenderCallbackRuleTest.php b/tests/src/Rules/RenderCallbackRuleTest.php index 4cb82e54..c5cc8b37 100644 --- a/tests/src/Rules/RenderCallbackRuleTest.php +++ b/tests/src/Rules/RenderCallbackRuleTest.php @@ -163,12 +163,14 @@ public static function fileData(): \Generator __DIR__ . '/data/bug-543.php', [ [ - "#access_callback callback array{static(Bug543\TestAccessClass), 'accessResultForbiddenNotInTrustedCallbacks'} at key '0' does not implement Drupal\Core\Security\TrustedCallbackInterface.", - 58 + "#access_callback callback method 'accessResultForbiddenNotInTrustedCallbacks' is not present in 'trustedCallbacks' at key '0'.", + 58, + "Change record: https://www.drupal.org/node/2966725." ], [ - "#access_callback callback array{static(Bug543\TestAccessClass), 'accessResultForbiddenNotInTrustedCallbacks'} at key '0' does not implement Drupal\Core\Security\TrustedCallbackInterface.", - 103 + "#access_callback callback method 'accessResultForbiddenNotInTrustedCallbacks' is not present in 'trustedCallbacks' at key '0'.", + 103, + "Change record: https://www.drupal.org/node/2966725." ], ], ]; From b849eae229000e8b75cb76fee235ff2acdc7577c Mon Sep 17 00:00:00 2001 From: Matt Glaman Date: Wed, 27 Sep 2023 15:03:42 -0500 Subject: [PATCH 3/6] load test classes with fixture test classes --- src/Drupal/DrupalAutoloader.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/Drupal/DrupalAutoloader.php b/src/Drupal/DrupalAutoloader.php index 699a97d6..a078eef6 100644 --- a/src/Drupal/DrupalAutoloader.php +++ b/src/Drupal/DrupalAutoloader.php @@ -94,6 +94,7 @@ public function register(Container $container): void $this->addThemeNamespaces(); $this->registerPs4Namespaces($this->namespaces); $this->loadLegacyIncludes(); + $this->loadTestFilesWithFixtureClasses(); foreach ($this->moduleData as $extension) { $this->loadExtension($extension); @@ -324,4 +325,18 @@ protected function camelize(string $id): string { return strtr(ucwords(strtr($id, ['_' => ' ', '.' => '_ ', '\\' => '_ '])), [' ' => '']); } + + private function loadTestFilesWithFixtureClasses() + { + $files = [ + $this->drupalRoot . '/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php', + $this->drupalRoot . '/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php', + $this->drupalRoot . '/core/tests/Drupal/Tests/Core/Render/RendererTest.php', + ]; + foreach ($files as $file) { + if (file_exists($file)) { + require_once $file; + } + } + } } From 09c6825fe31f3a9b713e4514b6929685a37c0797 Mon Sep 17 00:00:00 2001 From: Matt Glaman Date: Wed, 27 Sep 2023 15:11:59 -0500 Subject: [PATCH 4/6] fix return type --- src/Drupal/DrupalAutoloader.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Drupal/DrupalAutoloader.php b/src/Drupal/DrupalAutoloader.php index a078eef6..4fcf49f3 100644 --- a/src/Drupal/DrupalAutoloader.php +++ b/src/Drupal/DrupalAutoloader.php @@ -326,7 +326,7 @@ protected function camelize(string $id): string return strtr(ucwords(strtr($id, ['_' => ' ', '.' => '_ ', '\\' => '_ '])), [' ' => '']); } - private function loadTestFilesWithFixtureClasses() + private function loadTestFilesWithFixtureClasses(): void { $files = [ $this->drupalRoot . '/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php', From 30a70ef80a94050f3cc56c8f405832117be3f412 Mon Sep 17 00:00:00 2001 From: Matt Glaman Date: Wed, 27 Sep 2023 15:18:48 -0500 Subject: [PATCH 5/6] load test classes after class writer executes --- src/Drupal/DrupalAutoloader.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Drupal/DrupalAutoloader.php b/src/Drupal/DrupalAutoloader.php index 4fcf49f3..06df9e07 100644 --- a/src/Drupal/DrupalAutoloader.php +++ b/src/Drupal/DrupalAutoloader.php @@ -94,7 +94,6 @@ public function register(Container $container): void $this->addThemeNamespaces(); $this->registerPs4Namespaces($this->namespaces); $this->loadLegacyIncludes(); - $this->loadTestFilesWithFixtureClasses(); foreach ($this->moduleData as $extension) { $this->loadExtension($extension); @@ -197,6 +196,7 @@ class: Drupal\jsonapi\Routing\JsonApiParamEnhancer && class_exists('Drupal\TestTools\PhpUnitCompatibility\PhpUnit8\ClassWriter')) { \Drupal\TestTools\PhpUnitCompatibility\PhpUnit8\ClassWriter::mutateTestBase($this->autoloader); } + $this->loadTestFilesWithFixtureClasses(); $extension_map = $container->getByType(ExtensionMap::class); $extension_map->setExtensions($this->moduleData, $this->themeData, $profiles); From ae2fd21168620d1ee198f856cc09545a6861e459 Mon Sep 17 00:00:00 2001 From: Matt Glaman Date: Wed, 27 Sep 2023 15:30:49 -0500 Subject: [PATCH 6/6] do not load files add as a class map to autoload --- src/Drupal/DrupalAutoloader.php | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/src/Drupal/DrupalAutoloader.php b/src/Drupal/DrupalAutoloader.php index 06df9e07..617563fa 100644 --- a/src/Drupal/DrupalAutoloader.php +++ b/src/Drupal/DrupalAutoloader.php @@ -196,7 +196,6 @@ class: Drupal\jsonapi\Routing\JsonApiParamEnhancer && class_exists('Drupal\TestTools\PhpUnitCompatibility\PhpUnit8\ClassWriter')) { \Drupal\TestTools\PhpUnitCompatibility\PhpUnit8\ClassWriter::mutateTestBase($this->autoloader); } - $this->loadTestFilesWithFixtureClasses(); $extension_map = $container->getByType(ExtensionMap::class); $extension_map->setExtensions($this->moduleData, $this->themeData, $profiles); @@ -222,6 +221,14 @@ protected function addCoreTestNamespaces(): void $this->namespaces['Drupal\\TestSite'] = $core_tests_dir . '/TestSite'; $this->namespaces['Drupal\\TestTools'] = $core_tests_dir . '/TestTools'; $this->namespaces['Drupal\\Tests\\TestSuites'] = $this->drupalRoot . '/core/tests/TestSuites'; + + $classMap = [ + '\\Drupal\\Tests\\Core\\Render\\BubblingTest' => $this->drupalRoot . '/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php', + '\\Drupal\\Tests\\Core\\Render\\PlaceholdersTest' => $this->drupalRoot . '/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php', + '\\Drupal\\Tests\\Core\\Render\\TestAccessClass' => $this->drupalRoot . '/core/tests/Drupal/Tests/Core/Render/RendererTest.php', + '\\Drupal\\Tests\\Core\\Render\\TestCallables' => $this->drupalRoot . '/core/tests/Drupal/Tests/Core/Render/RendererTest.php', + ]; + $this->autoloader->addClassMap($classMap); } protected function addModuleNamespaces(): void @@ -325,18 +332,4 @@ protected function camelize(string $id): string { return strtr(ucwords(strtr($id, ['_' => ' ', '.' => '_ ', '\\' => '_ '])), [' ' => '']); } - - private function loadTestFilesWithFixtureClasses(): void - { - $files = [ - $this->drupalRoot . '/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php', - $this->drupalRoot . '/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php', - $this->drupalRoot . '/core/tests/Drupal/Tests/Core/Render/RendererTest.php', - ]; - foreach ($files as $file) { - if (file_exists($file)) { - require_once $file; - } - } - } }