Skip to content

Commit

Permalink
FIX Don't swallow relevant BadMethodCallExceptions from ViewLayerData
Browse files Browse the repository at this point in the history
  • Loading branch information
GuySartorelli committed Nov 28, 2024
1 parent c651007 commit ea1ab5b
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 3 deletions.
33 changes: 31 additions & 2 deletions src/View/ViewLayerData.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
37 changes: 36 additions & 1 deletion tests/php/View/ViewLayerDataTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
}
14 changes: 14 additions & 0 deletions tests/php/View/ViewLayerDataTest/TestBadMethodCallObject.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?php

namespace SilverStripe\View\Tests\ViewLayerDataTest;

use BadMethodCallException;
use SilverStripe\Dev\TestOnly;

class TestBadMethodCallObject implements TestOnly
{
public function __call(string $name, array $arguments): void
{
throw new BadMethodCallException('This exception should be caught by ViewLayerData');
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

namespace SilverStripe\View\Tests\ViewLayerDataTest;

use BadMethodCallException;
use SilverStripe\Dev\TestOnly;
use SilverStripe\Model\ModelData;

class TestBadMethodCallObjectSubclass extends TestBadMethodCallObject implements TestOnly
{
public function __call(string $name, array $arguments): void
{
if ($name === 'directMethod') {
throw new BadMethodCallException('This exception should be caught by ViewLayerData');
}
if ($name === 'realMethod') {
$this->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');
}
}

0 comments on commit ea1ab5b

Please sign in to comment.