From 89e8b363cd34499c700f21432643cfff7f164cc8 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Fri, 29 Nov 2024 11:52:15 +1300 Subject: [PATCH] FIX Don't swallow relevant BadMethodCallExceptions from ViewLayerData (#11482) --- src/Model/ModelDataCustomised.php | 4 +- src/View/ViewLayerData.php | 33 ++++++++++++++++- tests/php/View/ViewLayerDataTest.php | 37 ++++++++++++++++++- .../TestBadMethodCallObject.php | 14 +++++++ .../TestBadMethodCallObjectSubclass.php | 30 +++++++++++++++ 5 files changed, 113 insertions(+), 5 deletions(-) create mode 100644 tests/php/View/ViewLayerDataTest/TestBadMethodCallObject.php create mode 100644 tests/php/View/ViewLayerDataTest/TestBadMethodCallObjectSubclass.php diff --git a/src/Model/ModelDataCustomised.php b/src/Model/ModelDataCustomised.php index 8af4d6ebe4c..2aacbf39390 100644 --- a/src/Model/ModelDataCustomised.php +++ b/src/Model/ModelDataCustomised.php @@ -24,10 +24,10 @@ public function __construct(ModelData $originalObject, ModelData $customisedObje public function __call($method, $arguments) { if ($this->customised->hasMethod($method)) { - return call_user_func_array([$this->customised, $method], $arguments ?? []); + return $this->customised->$method(...$arguments); } - return call_user_func_array([$this->original, $method], $arguments ?? []); + return $this->original->$method(...$arguments); } public function __get(string $property): mixed diff --git a/src/View/ViewLayerData.php b/src/View/ViewLayerData.php index b593bff80c0..bb9380f7c16 100644 --- a/src/View/ViewLayerData.php +++ b/src/View/ViewLayerData.php @@ -244,12 +244,41 @@ private function callDataMethod(object $data, string $name, array $arguments, bo } catch (BadMethodCallException $e) { // Only throw the exception if we weren't relying on __call // It's common for __call to throw BadMethodCallException for methods that aren't "implemented" - // so we just want to return null in those cases. - if (!$hasDynamicMethods) { + // so we just want to return null in those cases - but only if it's a direct result of our method call. + if (!$hasDynamicMethods || $this->mustThrow($e->getTrace())) { throw $e; } } } return null; } + + private function mustThrow(array $trace): bool + { + $dataClass = get_class($this->data); + foreach ($trace as $data) { + $class = $data['class'] ?? ''; + $method = $data['function'] ?? ''; + if ($class === ViewLayerData::class) { + // If we hit ViewLayerData::callDataMethod() we've finished checking the relevant parts of the stack + if ($method === 'callDataMethod') { + break; + } + // If we're trying to call some other method on ViewLayerData and it causes problems, throw the exception. + return true; + } + // If we find a non __call method return true + // This means our method exists, but it tried to call something else which doesn't + if ($method !== '__call') { + return true; + } + // If we break class inheritance return true + if (!is_a($class, $dataClass, true) && !is_a($dataClass, $class, true)) { + return true; + } + } + // Hitting this means `callDataMethod()` only hit `__call()` methods inside the class hierarchy of our data, which + // means the method we were actually trying to call is missing and we can safely ignore the exception. + return false; + } } diff --git a/tests/php/View/ViewLayerDataTest.php b/tests/php/View/ViewLayerDataTest.php index 995c00b6f98..ca7a02ecfd8 100644 --- a/tests/php/View/ViewLayerDataTest.php +++ b/tests/php/View/ViewLayerDataTest.php @@ -5,7 +5,6 @@ use ArrayIterator; use BadMethodCallException; use Error; -use InvalidArgumentException; use PHPUnit\Framework\Attributes\DataProvider; use SilverStripe\Dev\SapphireTest; use SilverStripe\Model\ArrayData; @@ -20,6 +19,7 @@ use SilverStripe\View\Tests\ViewLayerDataTest\GetCountObject; use SilverStripe\View\Tests\ViewLayerDataTest\NonIterableObject; use SilverStripe\View\Tests\ViewLayerDataTest\StringableObject; +use SilverStripe\View\Tests\ViewLayerDataTest\TestBadMethodCallObjectSubclass; use SilverStripe\View\Tests\ViewLayerDataTest\TestFixture; use SilverStripe\View\Tests\ViewLayerDataTest\TestFixtureComplex; use SilverStripe\View\ViewLayerData; @@ -707,4 +707,39 @@ public function testSpecialNames(): void $this->assertSame('some other class', $viewLayerData->getRawDataValue('ClassName')); $this->assertSame('something else', $viewLayerData->getRawDataValue('Me')); } + + public static function provideCallDataMethodExceptionCatching(): array + { + return [ + 'exception thrown directly in __call()' => [ + 'method' => 'directMethod', + 'exceptionCaught' => true, + ], + 'exception thrown in __call() on superclass' => [ + 'method' => 'inheritedMethod', + 'exceptionCaught' => true, + ], + 'exception thrown outside __call()' => [ + 'method' => 'realMethod', + 'exceptionCaught' => false, + ], + 'exception thrown in __call() in a different class hierarchy' => [ + 'method' => 'anotherClass', + 'exceptionCaught' => false, + ], + ]; + } + + #[DataProvider('provideCallDataMethodExceptionCatching')] + public function testCallDataMethodExceptionCatching(string $method, bool $exceptionCaught): void + { + $data = new TestBadMethodCallObjectSubclass(); + $viewLayerData = new ViewLayerData($data); + if (!$exceptionCaught) { + $this->expectException(BadMethodCallException::class); + } + + $result = $viewLayerData->$method(); + $this->assertNull($result); + } } diff --git a/tests/php/View/ViewLayerDataTest/TestBadMethodCallObject.php b/tests/php/View/ViewLayerDataTest/TestBadMethodCallObject.php new file mode 100644 index 00000000000..54ec9809891 --- /dev/null +++ b/tests/php/View/ViewLayerDataTest/TestBadMethodCallObject.php @@ -0,0 +1,14 @@ +throwException(); + } + if ($name === 'anotherClass') { + $data = new ModelData(); + $data->thisMethodDoesntExist(); + } + parent::__call($name, $arguments); + } + + public function throwException(): void + { + throw new BadMethodCallException('This exception should NOT be caught by ViewLayerData'); + } +}