IBX-11747: Fixed exception loop when rendering HTTP 405 responses - #1996
IBX-11747: Fixed exception loop when rendering HTTP 405 responses#1996Sztig wants to merge 2 commits into
Conversation
|
alongosz
changed the title
| if (!$event->isMainRequest()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
@Sztig One thing that is unclear for me - does this mean that the other responses carried the same issue? Because while you've changed this for 405, it's gonna apply to all now.
There was a problem hiding this comment.
Yes, this is not just 405 error but I figured I should fix it here, I should elaborate what actually is happening here. The first exception is an actual relevant exception - 405, and since this is not a covered case, default template would also try to render the menu for the user when rendering the exception view - this also fails as a subrequest.
Its being thrown at symfony/http-kernel/Fragment/InlineFragmentRenderer, $this->dispatcher->dispatch($event, KernelEvents::EXCEPTION); dispatches the event, it lands in AdminExceptionListener repeating the process.
Symfony comments in the code mentions that this dispatch happens only to trigger the logging, the response that comes back should be ignored ergo the decision to stop the subrequest logging on our side in AdminExceptionListener - https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php#L82C1-L83C55
Please let me know what you think as maybe this is not the optimal place to fix it
| /** @var \Twig\Environment|\PHPUnit\Framework\MockObject\MockObject */ | ||
| private $twig; | ||
|
|
||
| /** @var \Ibexa\Contracts\AdminUi\Notification\NotificationHandlerInterface|\PHPUnit\Framework\MockObject\Stub */ | ||
| private $notificationHandler; | ||
|
|
||
| /** @var \Symfony\WebpackEncoreBundle\Asset\TagRenderer|\PHPUnit\Framework\MockObject\MockObject */ | ||
| private $encoreTagRenderer; | ||
|
|
||
| /** @var \Symfony\WebpackEncoreBundle\Asset\EntrypointLookupCollectionInterface|\PHPUnit\Framework\MockObject\MockObject */ | ||
| private $entrypointLookupCollection; | ||
|
|
||
| /** @var \Ibexa\AdminUi\EventListener\AdminExceptionListener */ | ||
| private $listener; |
There was a problem hiding this comment.
Please use in PHPDoc intersection instead of union and strict-type the "main" object type.
| /** | ||
| * @return iterable<string, array{0: string, 1: int, 2: string}> | ||
| */ | ||
| public function provideNoOpConditions(): iterable |
There was a problem hiding this comment.
| public function provideNoOpConditions(): iterable | |
| public static function provideNoOpConditions(): iterable |
| /** | ||
| * @return iterable<string, array{0: \Throwable, 1: int, 2: string, 3: array<string, string>}> | ||
| */ | ||
| public function provideHttpExceptionsWithDedicatedErrorPages(): iterable |
There was a problem hiding this comment.
| public function provideHttpExceptionsWithDedicatedErrorPages(): iterable | |
| public static function provideHttpExceptionsWithDedicatedErrorPages(): iterable |
| use Twig\Environment; | ||
| use Twig\Error\RuntimeError; | ||
|
|
||
| class AdminExceptionListenerTest extends TestCase |
There was a problem hiding this comment.
| class AdminExceptionListenerTest extends TestCase | |
| final class AdminExceptionListenerTest extends TestCase |
| int $requestType, | ||
| string $siteaccessName | ||
| ): void { | ||
| $this->twig->expects($this->never())->method('render'); |
There was a problem hiding this comment.
Use https://github.com/ibexa/internal-ai/pull/3 for PHPUnit guidelines. This will help us avoid recurring review comments:
| $this->twig->expects($this->never())->method('render'); | |
| $this->twig->expects(self::never())->method('render'); |
| $event = $this->createExceptionEvent(new NotFoundHttpException(), $requestType, $siteaccessName); | ||
| $this->createListener($environment)->onKernelException($event); | ||
|
|
||
| $this->assertNull($event->getResponse()); |
There was a problem hiding this comment.
| $this->assertNull($event->getResponse()); | |
| self::assertNull($event->getResponse()); |
| array $expectedHeaders | ||
| ): void { | ||
| $this->twig | ||
| ->expects($this->once()) |
| $this->assertInstanceOf(Response::class, $response); | ||
| $this->assertSame($expectedStatusCode, $response->getStatusCode()); | ||
| $this->assertSame('rendered_error_page_content', $response->getContent()); |
| $this->assertSame('rendered_error_page_content', $response->getContent()); | ||
|
|
||
| foreach ($expectedHeaders as $name => $value) { | ||
| $this->assertSame($value, $response->headers->get($name)); |
| $entrypointLookup->expects($this->once())->method('reset'); | ||
|
|
||
| $this->entrypointLookupCollection | ||
| ->expects($this->once()) |
| $this->encoreTagRenderer->expects($this->once())->method('reset'); | ||
|
|
||
| $this->twig | ||
| ->expects($this->once()) |
| $this->assertInstanceOf(Response::class, $response); | ||
| $this->assertSame(Response::HTTP_INTERNAL_SERVER_ERROR, $response->getStatusCode()); |
7 participants
Description:
I'm preventing the loping by only processing the error from the main request
I'm also handling the 405 code as a separate case with its own template
This listener had no test coverage previously so I have added that as well, covered both the fix and the expected behavior outside this specific case.
For QA:
Documentation: