Skip to content

Commit 83b7009

Browse files
KocalKocal
committed
bug #48 [Asset] Reset asset state when an exception is handled (Kocal)
This PR was merged into the main branch. Discussion ---------- [Asset] Reset asset state when an exception is handled | Q | A | -------------- | --- | Bug fix? | yes | New feature? | no | Deprecations? | no | Documentation? | no | License | MIT When a template renders an entry's tags and then throws, Symfony's `ErrorListener` renders the error page in a sub-request during `kernel.exception`. Until now RepriseBundle reset its per-request asset state (the entrypoints lookup's deduplication set and the tag renderer's "HMR client already injected" flag) only on `FINISH_REQUEST`, which fires *after* that error page has already been rendered. The result: the error page received only whatever deduplicated remainder of the tags was left over from the failed request, so `<script>` and `<link>` tags were missing, and in dev mode the HMR client was never injected. The fix makes the reset listener also subscribe to `kernel.exception` at default priority (0), which runs ahead of `ErrorListener`'s -128. Both services are reset before `ErrorListener` takes over, so the error page gets the full tag set. This mirrors the approach taken by WebpackEncoreBundle's `ExceptionListener`. Covered by unit tests, including one that asserts the reset fires before the -128 error render, and a functional test for the full flow. Commits ------- 6191ce3 [Asset] Reset asset state when an exception is handled
2 parents a70a55d + 6191ce3 commit 83b7009

5 files changed

Lines changed: 147 additions & 10 deletions

File tree

‎src/EventListener/ResetAssetsEventListener.php‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,16 @@
1212
namespace Symfony\Reprise\EventListener;
1313

1414
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
15+
use Symfony\Component\HttpKernel\Event\ExceptionEvent;
1516
use Symfony\Component\HttpKernel\Event\FinishRequestEvent;
1617
use Symfony\Component\HttpKernel\KernelEvents;
1718
use Symfony\Reprise\Asset\EntrypointsLookupInterface;
19+
use Symfony\Reprise\Asset\TagRenderer;
1820

1921
/**
20-
* Clears the lookup's per-request deduplication state once the main request finishes,
21-
* so a long-running worker (FrankenPHP, RoadRunner, ...) starts each request afresh.
22+
* Clears the per-request asset state (lookup dedup + renderer client flag) once the main request finishes,
23+
* so a long-running worker (FrankenPHP, RoadRunner, ...) starts afresh, and on an exception before
24+
* ErrorListener renders the error page, so that page still gets the full tag set.
2225
*
2326
* @author Hugo Alliaume <hugo@alliau.me>
2427
*
@@ -28,21 +31,36 @@ final class ResetAssetsEventListener implements EventSubscriberInterface
2831
{
2932
public function __construct(
3033
private readonly EntrypointsLookupInterface $entrypointsLookup,
34+
private readonly TagRenderer $tagRenderer,
3135
) {
3236
}
3337

3438
public function onFinishRequest(FinishRequestEvent $event): void
3539
{
3640
if ($event->isMainRequest()) {
37-
$this->entrypointsLookup->reset();
41+
$this->reset();
3842
}
3943
}
4044

45+
public function onException(ExceptionEvent $event): void
46+
{
47+
$this->reset();
48+
}
49+
4150
/**
4251
* @return array<string, string>
4352
*/
4453
public static function getSubscribedEvents(): array
4554
{
46-
return [KernelEvents::FINISH_REQUEST => 'onFinishRequest'];
55+
return [
56+
KernelEvents::FINISH_REQUEST => 'onFinishRequest',
57+
KernelEvents::EXCEPTION => 'onException',
58+
];
59+
}
60+
61+
private function reset(): void
62+
{
63+
$this->entrypointsLookup->reset();
64+
$this->tagRenderer->reset();
4765
}
4866
}

‎src/RepriseBundle.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ public function loadExtension(array $config, ContainerConfigurator $container, C
103103
$services->alias(EntrypointsLookupInterface::class, 'reprise.entrypoints_lookup');
104104

105105
$services->set('reprise.reset_assets_listener', ResetAssetsEventListener::class)
106-
->args([service('reprise.entrypoints_lookup')])
106+
->args([service('reprise.entrypoints_lookup'), service('reprise.tag_renderer')])
107107
->tag('kernel.event_subscriber')
108108
;
109109

‎tests/EventListener/ResetAssetsEventListenerTest.php‎

Lines changed: 78 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,43 +12,116 @@
1212
namespace Symfony\Reprise\Tests\EventListener;
1313

1414
use PHPUnit\Framework\TestCase;
15+
use Symfony\Component\Asset\Packages;
16+
use Symfony\Component\Asset\PathPackage;
17+
use Symfony\Component\Asset\VersionStrategy\EmptyVersionStrategy;
18+
use Symfony\Component\EventDispatcher\EventDispatcher;
1519
use Symfony\Component\HttpFoundation\Request;
20+
use Symfony\Component\HttpKernel\Event\ExceptionEvent;
1621
use Symfony\Component\HttpKernel\Event\FinishRequestEvent;
1722
use Symfony\Component\HttpKernel\HttpKernelInterface;
23+
use Symfony\Component\HttpKernel\KernelEvents;
1824
use Symfony\Reprise\Asset\EntrypointsLookup;
25+
use Symfony\Reprise\Asset\EntrypointsLookupInterface;
26+
use Symfony\Reprise\Asset\TagRenderer;
1927
use Symfony\Reprise\EventListener\ResetAssetsEventListener;
2028

2129
final class ResetAssetsEventListenerTest extends TestCase
2230
{
23-
private function lookup(): EntrypointsLookup
31+
private function lookup(string $fixture = 'build'): EntrypointsLookup
2432
{
25-
return new EntrypointsLookup(__DIR__.'/../fixtures/build/entrypoints.json');
33+
return new EntrypointsLookup(__DIR__.'/../fixtures/'.$fixture.'/entrypoints.json');
34+
}
35+
36+
private function rendererFor(EntrypointsLookupInterface $lookup): TagRenderer
37+
{
38+
return new TagRenderer($lookup, new Packages(new PathPackage('/', new EmptyVersionStrategy())));
2639
}
2740

2841
private function finishRequest(int $requestType): FinishRequestEvent
2942
{
3043
return new FinishRequestEvent($this->createStub(HttpKernelInterface::class), Request::create('/'), $requestType);
3144
}
3245

46+
private function exceptionEvent(int $requestType): ExceptionEvent
47+
{
48+
return new ExceptionEvent($this->createStub(HttpKernelInterface::class), Request::create('/'), $requestType, new \RuntimeException('boom'));
49+
}
50+
3351
public function testResetsDeduplicationWhenTheMainRequestFinishes()
3452
{
3553
$lookup = $this->lookup();
3654
$lookup->getPreloadFiles('app'); // marks the shared chunk as already returned
3755

38-
new ResetAssetsEventListener($lookup)->onFinishRequest($this->finishRequest(HttpKernelInterface::MAIN_REQUEST));
56+
new ResetAssetsEventListener($lookup, $this->rendererFor($lookup))->onFinishRequest($this->finishRequest(HttpKernelInterface::MAIN_REQUEST));
3957

4058
// After the reset the shared chunk is offered again to the next request.
4159
$this->assertSame(['build/shared-e5f6.js'], $lookup->getPreloadFiles('admin'));
4260
}
4361

44-
public function testIgnoresSubRequests()
62+
public function testIgnoresSubRequestsOnFinishRequest()
4563
{
4664
$lookup = $this->lookup();
4765
$lookup->getPreloadFiles('app');
4866

49-
new ResetAssetsEventListener($lookup)->onFinishRequest($this->finishRequest(HttpKernelInterface::SUB_REQUEST));
67+
new ResetAssetsEventListener($lookup, $this->rendererFor($lookup))->onFinishRequest($this->finishRequest(HttpKernelInterface::SUB_REQUEST));
5068

5169
// A sub-request finishing must NOT reset -- the shared chunk stays deduplicated.
5270
$this->assertSame([], $lookup->getPreloadFiles('admin'));
5371
}
72+
73+
public function testResetsDeduplicationWhenAnExceptionIsHandled()
74+
{
75+
$lookup = $this->lookup();
76+
$lookup->getPreloadFiles('app');
77+
78+
new ResetAssetsEventListener($lookup, $this->rendererFor($lookup))->onException($this->exceptionEvent(HttpKernelInterface::MAIN_REQUEST));
79+
80+
$this->assertSame(['build/shared-e5f6.js'], $lookup->getPreloadFiles('admin'));
81+
}
82+
83+
public function testResetsDeduplicationEvenForSubRequestExceptions()
84+
{
85+
// An exception at any level abandons the render, so even a sub-request one resets (unlike FINISH_REQUEST).
86+
$lookup = $this->lookup();
87+
$lookup->getPreloadFiles('app');
88+
89+
new ResetAssetsEventListener($lookup, $this->rendererFor($lookup))->onException($this->exceptionEvent(HttpKernelInterface::SUB_REQUEST));
90+
91+
$this->assertSame(['build/shared-e5f6.js'], $lookup->getPreloadFiles('admin'));
92+
}
93+
94+
public function testResetsTheRendererWhenAnExceptionIsHandled()
95+
{
96+
$lookup = $this->lookup('dev');
97+
$renderer = $this->rendererFor($lookup);
98+
$renderer->renderScriptTags('app');
99+
$this->assertStringNotContainsString('@vite/client', $renderer->renderScriptTags('app'), 'sanity: not re-injected within the same request');
100+
101+
new ResetAssetsEventListener($lookup, $renderer)->onException($this->exceptionEvent(HttpKernelInterface::MAIN_REQUEST));
102+
103+
$this->assertStringContainsString('@vite/client', $renderer->renderScriptTags('app'));
104+
}
105+
106+
public function testResetsBeforeTheErrorPageIsRendered()
107+
{
108+
$lookup = $this->lookup();
109+
$renderer = $this->rendererFor($lookup);
110+
111+
$dispatcher = new EventDispatcher();
112+
$dispatcher->addSubscriber(new ResetAssetsEventListener($lookup, $renderer));
113+
114+
// Symfony's ErrorListener renders the error page at priority -128; capture what it would emit.
115+
$errorPageTags = null;
116+
$dispatcher->addListener(KernelEvents::EXCEPTION, static function () use (&$errorPageTags, $renderer) {
117+
$errorPageTags = $renderer->renderScriptTags('app');
118+
}, -128);
119+
120+
$full = $renderer->renderScriptTags('app');
121+
$dispatcher->dispatch($this->exceptionEvent(HttpKernelInterface::MAIN_REQUEST), KernelEvents::EXCEPTION);
122+
123+
// Reprise's reset (default priority 0) runs before -128, so the error page gets the full tag set.
124+
$this->assertSame($full, $errorPageTags);
125+
$this->assertStringContainsString('build/app-a1b2.js', $errorPageTags);
126+
}
54127
}

‎tests/Functional/ExceptionResetTest.php‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Reprise\Tests\Functional;
13+
14+
use PHPUnit\Framework\TestCase;
15+
use Symfony\Component\HttpFoundation\Request;
16+
use Symfony\Component\HttpKernel\Event\ExceptionEvent;
17+
use Symfony\Component\HttpKernel\HttpKernelInterface;
18+
use Symfony\Reprise\Asset\TagRenderer;
19+
use Symfony\Reprise\EventListener\ResetAssetsEventListener;
20+
use Symfony\Reprise\Tests\Kernel\FunctionalAppKernel;
21+
22+
final class ExceptionResetTest extends TestCase
23+
{
24+
public function testTheErrorPageGetsTheFullTagsAfterAPartialRenderThrows()
25+
{
26+
$kernel = new FunctionalAppKernel(__DIR__.'/../fixtures/build');
27+
$kernel->boot();
28+
$container = $kernel->getContainer();
29+
30+
/** @var TagRenderer $renderer */
31+
$renderer = $container->get('reprise.tag_renderer');
32+
/** @var ResetAssetsEventListener $listener */
33+
$listener = $container->get('reprise.reset_assets_listener');
34+
35+
// Stand in for a controller that renders the entry, then throws.
36+
$full = $renderer->renderScriptTags('app');
37+
$this->assertStringContainsString('build/app-a1b2.js', $full);
38+
$this->assertSame('', $renderer->renderScriptTags('app'), 'sanity: the deduplicated re-render is empty');
39+
40+
// The container-wired listener shares that lookup, so its reset clears the dedup set before the error page.
41+
$listener->onException(new ExceptionEvent($kernel, Request::create('/'), HttpKernelInterface::MAIN_REQUEST, new \RuntimeException('boom')));
42+
43+
$this->assertSame($full, $renderer->renderScriptTags('app'));
44+
}
45+
}

‎tests/Kernel/FunctionalAppKernel.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ public function process(ContainerBuilder $container): void
7474
{
7575
$container->getAlias(EntrypointsLookupInterface::class)->setPublic(true);
7676
$container->getDefinition('reprise.tag_renderer')->setPublic(true);
77+
$container->getDefinition('reprise.reset_assets_listener')->setPublic(true);
7778

7879
if ($container->hasDefinition('reprise.entrypoints_cache_warmer')) {
7980
$container->getDefinition('reprise.entrypoints_cache_warmer')->setPublic(true);

0 commit comments

Comments
 (0)