From 233632a6cdf0adba7a6ded53d7db3225c3dcd374 Mon Sep 17 00:00:00 2001 From: James Titcumb Date: Thu, 24 Aug 2023 07:59:50 +0100 Subject: [PATCH 1/4] Added reference to decorated lastCompiled property if it exists in ScoutViewEngineDecorator --- .../View/Engine/ScoutViewEngineDecorator.php | 22 +++++++++++++++ .../Engine/ScoutViewEngineDecoratorTest.php | 28 ++++++++++++++++++- 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/Laravel/View/Engine/ScoutViewEngineDecorator.php b/src/Laravel/View/Engine/ScoutViewEngineDecorator.php index 0d060460..284b4429 100644 --- a/src/Laravel/View/Engine/ScoutViewEngineDecorator.php +++ b/src/Laravel/View/Engine/ScoutViewEngineDecorator.php @@ -11,6 +11,7 @@ use function assert; use function method_exists; +use function property_exists; /** @noinspection ContractViolationInspection */ final class ScoutViewEngineDecorator implements Engine @@ -33,11 +34,32 @@ final class ScoutViewEngineDecorator implements Engine /** @var Factory */ private $viewFactory; + /** @var array|null */ + protected $lastCompiled; + public function __construct(Engine $engine, ScoutApmAgent $agent, Factory $viewFactory) { $this->engine = $engine; $this->agent = $agent; $this->viewFactory = $viewFactory; + + /** + * Unsure of which library or bit of code is trying to directly reflect on this protected property, but a + * customer reported a {@see \ReflectionException} when something was trying to reflect on `lastCompiled` + * (which was not a property of {@see ScoutViewEngineDecorator}, but it is a `protected` property in + * {@see CompilerEngine}. In order to satisfy the reflection, we can create a reference to the real + * {@see CompilerEngine::lastCompiled} property, so if someone mistakenly references our decorator directly, + * they should see the real value. + */ + if (! property_exists($engine, 'lastCompiled')) { + return; + } + + /** + * @psalm-suppress MixedAssignment + * @psalm-suppress NoInterfaceProperties + */ + $this->lastCompiled = &$engine->lastCompiled; } /** diff --git a/tests/Unit/Laravel/View/Engine/ScoutViewEngineDecoratorTest.php b/tests/Unit/Laravel/View/Engine/ScoutViewEngineDecoratorTest.php index 02ba4eb7..a920c807 100644 --- a/tests/Unit/Laravel/View/Engine/ScoutViewEngineDecoratorTest.php +++ b/tests/Unit/Laravel/View/Engine/ScoutViewEngineDecoratorTest.php @@ -16,7 +16,9 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use ReflectionClass; +use ReflectionException; use ReflectionMethod; +use ReflectionProperty; use Scoutapm\Laravel\View\Engine\ScoutViewEngineDecorator; use Scoutapm\ScoutApmAgent; use Spatie\LaravelIgnition\Views\BladeSourceMapCompiler; @@ -185,7 +187,7 @@ public function testSpatieLaravelIgnitionCompatibility(): void * * @link https://github.com/spatie/laravel-ignition/blob/d53075177ee0c710fbf588b8569f50435e1da054/src/Views/ViewExceptionMapper.php#L124-L130 * - * @noinspection PhpPossiblePolymorphicInvocationInspection PhpUndefinedFieldInspection + * @noinspection PhpPossiblePolymorphicInvocationInspection * @psalm-suppress NoInterfaceProperties */ $this->realEngine->lastCompiled = []; @@ -205,4 +207,28 @@ public function testSpatieLaravelIgnitionCompatibility(): void $vem = new ViewExceptionMapper($this->createMock(BladeSourceMapCompiler::class)); $vem->map(new ViewException('things (View: paththing)')); } + + /** @throws ReflectionException */ + public function testDecoratorLastCompiledPropertyReferencesCompilerEngineLastCompiledPropertyWhenUsingReflection(): void + { + /** + * @noinspection PhpPossiblePolymorphicInvocationInspection + * @psalm-suppress NoInterfaceProperties + */ + $this->realEngine->lastCompiled = ['a', 'b']; + + $this->viewEngineDecorator = new ScoutViewEngineDecorator($this->realEngine, $this->agent, $this->viewFactory); + + $prop = new ReflectionProperty($this->viewEngineDecorator, 'lastCompiled'); + $prop->setAccessible(true); + self::assertSame(['a', 'b'], $prop->getValue($this->viewEngineDecorator)); + + /** + * @noinspection PhpPossiblePolymorphicInvocationInspection + * @psalm-suppress NoInterfaceProperties + */ + $this->realEngine->lastCompiled = ['a', 'b', 'c']; + + self::assertSame(['a', 'b', 'c'], $prop->getValue($this->viewEngineDecorator)); + } } From 1307ce7dab60302b8eb199cf818c7738f4628d55 Mon Sep 17 00:00:00 2001 From: James Titcumb Date: Thu, 24 Aug 2023 09:32:54 +0100 Subject: [PATCH 2/4] Ensure protected/private lastCompiled can be accessed by ScoutViewEngineDecorator --- .../View/Engine/ScoutViewEngineDecorator.php | 11 +++++++++-- ...ngineImplementationWithGetCompilerMethod.php | 9 +++++++++ .../Engine/ScoutViewEngineDecoratorTest.php | 17 +++++------------ 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/Laravel/View/Engine/ScoutViewEngineDecorator.php b/src/Laravel/View/Engine/ScoutViewEngineDecorator.php index 284b4429..35bdc3a4 100644 --- a/src/Laravel/View/Engine/ScoutViewEngineDecorator.php +++ b/src/Laravel/View/Engine/ScoutViewEngineDecorator.php @@ -4,6 +4,7 @@ namespace Scoutapm\Laravel\View\Engine; +use Closure; use Illuminate\Contracts\View\Engine; use Illuminate\View\Compilers\CompilerInterface; use Illuminate\View\Factory; @@ -57,9 +58,15 @@ public function __construct(Engine $engine, ScoutApmAgent $agent, Factory $viewF /** * @psalm-suppress MixedAssignment - * @psalm-suppress NoInterfaceProperties + * @psalm-suppress PossiblyInvalidFunctionCall */ - $this->lastCompiled = &$engine->lastCompiled; + $this->lastCompiled = & Closure::bind( + function & () { + return $this->lastCompiled; + }, + $engine, + $engine + )->__invoke(); } /** diff --git a/tests/Unit/Laravel/View/Engine/EngineImplementationWithGetCompilerMethod.php b/tests/Unit/Laravel/View/Engine/EngineImplementationWithGetCompilerMethod.php index 1228f868..eae91820 100644 --- a/tests/Unit/Laravel/View/Engine/EngineImplementationWithGetCompilerMethod.php +++ b/tests/Unit/Laravel/View/Engine/EngineImplementationWithGetCompilerMethod.php @@ -9,12 +9,21 @@ class EngineImplementationWithGetCompilerMethod implements Engine { + /** @var list */ + protected $lastCompiled = []; + /** @inheritDoc */ public function get($path, array $data = []) { return ''; } + /** @param list $newValue */ + public function setLastCompiled(array $newValue): void + { + $this->lastCompiled = $newValue; + } + public function getCompiler(): CompilerInterface { return new class implements CompilerInterface { diff --git a/tests/Unit/Laravel/View/Engine/ScoutViewEngineDecoratorTest.php b/tests/Unit/Laravel/View/Engine/ScoutViewEngineDecoratorTest.php index a920c807..45a434d2 100644 --- a/tests/Unit/Laravel/View/Engine/ScoutViewEngineDecoratorTest.php +++ b/tests/Unit/Laravel/View/Engine/ScoutViewEngineDecoratorTest.php @@ -211,24 +211,17 @@ public function testSpatieLaravelIgnitionCompatibility(): void /** @throws ReflectionException */ public function testDecoratorLastCompiledPropertyReferencesCompilerEngineLastCompiledPropertyWhenUsingReflection(): void { - /** - * @noinspection PhpPossiblePolymorphicInvocationInspection - * @psalm-suppress NoInterfaceProperties - */ - $this->realEngine->lastCompiled = ['a', 'b']; + $realEngine = new EngineImplementationWithGetCompilerMethod(); + $realEngine->setLastCompiled(['a', 'b']); - $this->viewEngineDecorator = new ScoutViewEngineDecorator($this->realEngine, $this->agent, $this->viewFactory); + $this->viewEngineDecorator = new ScoutViewEngineDecorator($realEngine, $this->agent, $this->viewFactory); $prop = new ReflectionProperty($this->viewEngineDecorator, 'lastCompiled'); $prop->setAccessible(true); self::assertSame(['a', 'b'], $prop->getValue($this->viewEngineDecorator)); - /** - * @noinspection PhpPossiblePolymorphicInvocationInspection - * @psalm-suppress NoInterfaceProperties - */ - $this->realEngine->lastCompiled = ['a', 'b', 'c']; - + // Make sure the value can be changed at runtime, and the decorator's value is also changed + $realEngine->setLastCompiled(['a', 'b', 'c']); self::assertSame(['a', 'b', 'c'], $prop->getValue($this->viewEngineDecorator)); } } From 42e7df868250e1b5b2d00c93b0257f4776193256 Mon Sep 17 00:00:00 2001 From: James Titcumb Date: Thu, 24 Aug 2023 10:13:43 +0100 Subject: [PATCH 3/4] Updated testSpatieLaravelIgnitionCompatibility test as lastCompiled is now a real protected property on the test harness --- .../Engine/ScoutViewEngineDecoratorTest.php | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/tests/Unit/Laravel/View/Engine/ScoutViewEngineDecoratorTest.php b/tests/Unit/Laravel/View/Engine/ScoutViewEngineDecoratorTest.php index 45a434d2..75ef1334 100644 --- a/tests/Unit/Laravel/View/Engine/ScoutViewEngineDecoratorTest.php +++ b/tests/Unit/Laravel/View/Engine/ScoutViewEngineDecoratorTest.php @@ -173,25 +173,20 @@ public function testScoutViewEngineDecoratorImplementsAllPublicApiOfCompilerEngi } } + /** + * The `spatie/laravel-ignition` package depends on the engine having a property called `lastCompiled`, which + * only exists in the `\Illuminate\View\Engines\CompilerEngine` Blade Compiler. The implementation does sort of + * account for decoration, but it expects the property to be called `engine`. Therefore, in this test, we + * invoke the problematic consumer to ensure our decorated view engine conforms to this assumption. + * + * @link https://github.com/spatie/laravel-ignition/blob/d53075177ee0c710fbf588b8569f50435e1da054/src/Views/ViewExceptionMapper.php#L124-L130 + */ public function testSpatieLaravelIgnitionCompatibility(): void { if (! class_exists(ViewExceptionMapper::class)) { self::markTestSkipped('Test depends on `spatie/laravel-ignition`, but it is not installed'); } - /** - * The `spatie/laravel-ignition` package depends on the engine having a property called `lastCompiled`, which - * only exists in the `\Illuminate\View\Engines\CompilerEngine` Blade Compiler. The implementation does sort of - * account for decoration, but it expects the property to be called `engine`. Therefore, in this test, we - * invoke the problematic consumer to ensure our decorated view engine conforms to this assumption. - * - * @link https://github.com/spatie/laravel-ignition/blob/d53075177ee0c710fbf588b8569f50435e1da054/src/Views/ViewExceptionMapper.php#L124-L130 - * - * @noinspection PhpPossiblePolymorphicInvocationInspection - * @psalm-suppress NoInterfaceProperties - */ - $this->realEngine->lastCompiled = []; - $viewEngineResolver = new EngineResolver(); $viewEngineResolver->register('blade', function () { return $this->viewEngineDecorator; From 1382f817b1288c25ff54cb1008c10cda4a407ffa Mon Sep 17 00:00:00 2001 From: James Titcumb Date: Thu, 24 Aug 2023 10:27:21 +0100 Subject: [PATCH 4/4] Output logs for failed E2E test runs --- .github/workflows/continuous-integration.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index eae33797..7e66b2ac 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -526,6 +526,9 @@ jobs: wget --content-on-error -O error.html http://localhost:8000/e || true cat error.html ps -ax + - name: "Output logs on failure" + if: failure() + run: cat test-app/storage/logs/laravel.log - name: "Check logs for successful payload send" run: | cd test-app @@ -791,6 +794,9 @@ jobs: wget http://localhost:8000 cat index.html ps -ax + - name: "Output logs on failure" + if: failure() + run: cat test-app/storage/logs/lumen.log - name: "Check logs for successful payload send" run: | cd test-app