From 3fab2bfd16a65457eb9cc5d1cbd1a5f0f113b505 Mon Sep 17 00:00:00 2001 From: Tomas Vondracek Date: Mon, 17 Aug 2026 11:34:00 +0200 Subject: [PATCH] Propagate current locale to custom 404/403/500 error pages When no route matches (404/403), Symfony's LocaleListener never runs, so custom error pages and their records rendered in the default locale and ignored the locale in the URL (e.g. /nl/...). - ErrorController recovers the locale from the URL path and applies it to the request and the Twig sub-request, and syncs the locale-aware services via the LocaleSwitcher, so both {% trans %}/__() strings and the router's RequestContext (path()/url() in the template) follow it. The record locale is now passed explicitly to DetailController::record(). - The path is only consulted when routing never ran: with a matched route the locale is already set, and the first segment is a ContentType slug rather than a locale - a ContentType `it` must not render its error page in Italian. The sub-request and the locale-aware services are synced either way, because ErrorListener::duplicateRequest() hands the error page a fresh request that never saw the URL's locale. - Add redirectToDefaultLocaleOrFallback(): when there's no route to redirect to (error page / forwarded request), reset to the default locale and render instead of erroring. Guards the missing _route case that previously threw a TypeError (a 404-within-a-404). - ListingController uses the fallback so a forwarded listing renders in the default locale instead of erroring. Adds ErrorControllerTest and ListingControllerTest covering unrouted and routed 404s, default-locale rendering, translator and router-context locale recovery, the ContentType-slug guard, an end-to-end __() assertion through a fixture template, the 403 path, the non-localized ContentType case, the listing redirect, and the forwarded-listing fallback. --- phpstan-baseline.php | 12 - src/Controller/ErrorController.php | 47 ++- src/Controller/Frontend/ListingController.php | 5 +- src/Controller/TwigAwareController.php | 36 ++- .../Frontend/ErrorControllerTest.php | 288 ++++++++++++++++++ .../Frontend/Fixtures/locale_probe.html.twig | 3 + .../Frontend/ListingControllerTest.php | 63 ++++ 7 files changed, 436 insertions(+), 18 deletions(-) create mode 100644 tests/php/Controller/Frontend/ErrorControllerTest.php create mode 100644 tests/php/Controller/Frontend/Fixtures/locale_probe.html.twig create mode 100644 tests/php/Controller/Frontend/ListingControllerTest.php diff --git a/phpstan-baseline.php b/phpstan-baseline.php index 031a5190e..45f8daba6 100644 --- a/phpstan-baseline.php +++ b/phpstan-baseline.php @@ -1405,12 +1405,6 @@ 'count' => 1, 'path' => __DIR__ . '/src/Controller/Frontend/ListingController.php', ]; -$ignoreErrors[] = [ - 'message' => '#^Method Bolt\\\\Controller\\\\Frontend\\\\ListingController\\:\\:listing\\(\\) should return Symfony\\\\Component\\\\HttpFoundation\\\\Response but returns Symfony\\\\Component\\\\HttpFoundation\\\\Response\\|null\\.$#', - 'identifier' => 'return.type', - 'count' => 1, - 'path' => __DIR__ . '/src/Controller/Frontend/ListingController.php', -]; $ignoreErrors[] = [ 'message' => '#^Method Bolt\\\\Controller\\\\Frontend\\\\ListingController\\:\\:parseQueryParams\\(\\) return type has no value type specified in iterable type array\\.$#', 'identifier' => 'missingType.iterableValue', @@ -1495,12 +1489,6 @@ 'count' => 1, 'path' => __DIR__ . '/src/Controller/TwigAwareController.php', ]; -$ignoreErrors[] = [ - 'message' => '#^Method Bolt\\\\Controller\\\\TwigAwareController\\:\\:renderSingle\\(\\) should return Symfony\\\\Component\\\\HttpFoundation\\\\Response but returns Symfony\\\\Component\\\\HttpFoundation\\\\Response\\|null\\.$#', - 'identifier' => 'return.type', - 'count' => 1, - 'path' => __DIR__ . '/src/Controller/TwigAwareController.php', -]; $ignoreErrors[] = [ 'message' => '#^Method Bolt\\\\Controller\\\\TwigAwareController\\:\\:renderTemplate\\(\\) has parameter \\$parameters with no value type specified in iterable type array\\.$#', 'identifier' => 'missingType.iterableValue', diff --git a/src/Controller/ErrorController.php b/src/Controller/ErrorController.php index a4209e5a7..ea4bbc1a1 100644 --- a/src/Controller/ErrorController.php +++ b/src/Controller/ErrorController.php @@ -21,6 +21,7 @@ use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\Routing\Generator\UrlGeneratorInterface; +use Symfony\Component\Translation\LocaleSwitcher; use Throwable; use Twig\Environment; use Twig\Error\LoaderError; @@ -37,10 +38,17 @@ public function __construct( private readonly UrlGeneratorInterface $urlGenerator, private readonly Security $security, private readonly RequestStack $requestStack, + private readonly LocaleSwitcher $localeSwitcher, + string $locales, ) { parent::__construct($httpKernel, $this->templateController, $errorRenderer); + + $this->localeCodes = explode('|', $locales); } + /** @var list */ + private readonly array $localeCodes; + /** * Show an exception. Mainly used for custom 404 pages, otherwise falls back * to Symfony's error handling @@ -61,6 +69,8 @@ public function showAction(Environment $twig, Throwable $exception): Response // We need the parent request here, but fall back to current if not found if ($request = $this->requestStack->getParentRequest() ?? $this->requestStack->getCurrentRequest()) { + $this->setLocaleFromPath($request); + if ($code === Response::HTTP_SERVICE_UNAVAILABLE || $this->isMaintenanceEnabled($code)) { $twig->addGlobal('exception', $exception); @@ -156,6 +166,40 @@ private function isMaintenanceEnabled(int $code): bool return filter_var($this->config->get('general/maintenance_mode', false), FILTER_VALIDATE_BOOLEAN); } + /** + * Recovers the locale from the first path segment (e.g. `/de/...` => `de`), + * which Symfony's `LocaleListener` never did because no route matched. + * + * Applied to the given request and to the current one (a sub-request when an + * error page is rendered, which Twig's `app.request` resolves to). The + * `LocaleSwitcher` then syncs the translator and the router's `RequestContext`, + * so `{% trans %}` strings and `path()` calls follow suit. + */ + private function setLocaleFromPath(Request $request): void + { + // Only derive the locale from the path when routing didn't run. With a + // matched route the locale is already set, and the first segment is a + // ContentType slug rather than a locale: a ContentType `it` isn't Italian. + if (! $request->attributes->has('_route')) { + // Cast: `mb_trim()` is analysed as `string|false`, but `getPathInfo()` + // always returns a string. + $segment = explode('/', (string) mb_trim($request->getPathInfo(), '/'))[0]; + + if ($segment !== '' && in_array($segment, $this->localeCodes, true)) { + $request->setLocale($segment); + } + } + + // The error sub-request is a fresh Request that never saw the URL's locale, + // so propagate it regardless of how the locale was determined. + $currentRequest = $this->requestStack->getCurrentRequest(); + if ($currentRequest instanceof Request && $currentRequest !== $request) { + $currentRequest->setLocale($request->getLocale()); + } + + $this->localeSwitcher->setLocale($request->getLocale()); + } + private function attemptToRender(Request $request, string $item): ?Response { // First, see if it's a contenttype/slug pair: @@ -165,7 +209,8 @@ private function attemptToRender(Request $request, string $item): ?Response // We wrap it in a try/catch, because we wouldn't want to // trigger a 404 within a 404 now, would we? try { - return $this->detailController->record($request, $slug, $contentType, false, null); + // Pass the locale explicitly, or the record falls back to the default. + return $this->detailController->record($request, $slug, $contentType, false, $request->getLocale()); } catch (NotFoundHttpException) { // Just continue to the next one. } diff --git a/src/Controller/Frontend/ListingController.php b/src/Controller/Frontend/ListingController.php index 905775011..a7922b71f 100644 --- a/src/Controller/Frontend/ListingController.php +++ b/src/Controller/Frontend/ListingController.php @@ -45,8 +45,9 @@ public function listing(Request $request, ContentRepository $contentRepository, } // If the locale is the wrong locale - if (! $this->validLocaleForContentType($request, $contentType)) { - return $this->redirectToDefaultLocale($request); + if (! $this->validLocaleForContentType($request, $contentType) + && ($redirect = $this->redirectToDefaultLocaleOrFallback($request)) instanceof Response) { + return $redirect; } $page = (int) $this->getFromRequest($request, 'page', '1'); diff --git a/src/Controller/TwigAwareController.php b/src/Controller/TwigAwareController.php index d43695e2a..8b690e1a9 100644 --- a/src/Controller/TwigAwareController.php +++ b/src/Controller/TwigAwareController.php @@ -114,8 +114,9 @@ public function renderSingle(Request $request, ?Content $record, bool $requirePu } // If the locale is the wrong locale - if (! $this->validLocaleForContentType($request, $recordDefinition)) { - return $this->redirectToDefaultLocale($request); + if (! $this->validLocaleForContentType($request, $recordDefinition) + && ($redirect = $this->redirectToDefaultLocaleOrFallback($request)) instanceof Response) { + return $redirect; } $singularSlug = $record->getContentTypeSingularSlug(); @@ -145,8 +146,37 @@ protected function validLocaleForContentType(Request $request, ContentType $cont return $request->getLocale() === $this->defaultLocale; } + /** + * Redirects to the same route in the default locale. When there's no route to + * redirect to (a forwarded request, or an error page where routing never + * matched), resets the request to the default locale and returns `null` so the + * caller can render instead of erroring. + * + * Only the given request is reset, so on an error page `` may still + * show the URL locale while the record renders in the default one. Harmless: + * only non-localized content takes this path. + */ + protected function redirectToDefaultLocaleOrFallback(Request $request): ?Response + { + $redirect = $this->redirectToDefaultLocale($request); + + if ($redirect instanceof Response) { + return $redirect; + } + + $request->setLocale($this->defaultLocale); + + return null; + } + protected function redirectToDefaultLocale(Request $request): ?Response { + // No route matched (e.g. on an error page): nothing to redirect to. + $route = $request->attributes->get('_route'); + if (! $route) { + return null; + } + $request->getSession()->set('_locale', $this->defaultLocale); $params = $request->attributes->get('_route_params'); @@ -155,7 +185,7 @@ protected function redirectToDefaultLocale(Request $request): ?Response $params['_locale'] = $this->defaultLocale; } - return $this->redirectToRoute($request->get('_route'), $params); + return $this->redirectToRoute($route, $params); } private function setTwigLoader(): void diff --git a/tests/php/Controller/Frontend/ErrorControllerTest.php b/tests/php/Controller/Frontend/ErrorControllerTest.php new file mode 100644 index 000000000..fff943ebe --- /dev/null +++ b/tests/php/Controller/Frontend/ErrorControllerTest.php @@ -0,0 +1,288 @@ +seedLocalizedNotFoundPage(); + + $this->client->request('GET', '/nl/this-page-does-not-exist'); + $response = $this->client->getResponse(); + $body = (string) $response->getContent(); + + self::assertSame(404, $response->getStatusCode()); + self::assertStringContainsString('seedLocalizedNotFoundPage(); + + $this->client->request('GET', '/nl/pages/this-record-does-not-exist'); + $response = $this->client->getResponse(); + $body = (string) $response->getContent(); + + self::assertSame(404, $response->getStatusCode()); + self::assertStringContainsString('seedLocalizedNotFoundPage(); + + $this->client->request('GET', '/this-page-does-not-exist'); + $response = $this->client->getResponse(); + $body = (string) $response->getContent(); + + self::assertSame(404, $response->getStatusCode()); + self::assertStringContainsString('seedLocalizedPage(); + $this->setGeneralConfig('forbidden', ['page/' . $page->getId()]); + + // A frontend 403 isn't reachable from a routed request here: every + // `access_control` rule is backend, and the backend 403 redirects to the + // dashboard. So drive the error controller directly, as the kernel does. + // It returns a plain 200 - promoting the status is the kernel's job - so we + // assert only the locale handling that is this controller's responsibility. + $response = $this->renderError(new AccessDeniedHttpException(), '/nl/this-page-is-forbidden'); + $body = (string) $response->getContent(); + + self::assertStringContainsString('seedLocalizedPage(); + $this->setGeneralConfig('notfound', ['page/' . $page->getId()]); + + $this->renderError(new NotFoundHttpException(), '/nl/this-page-does-not-exist'); + + /** @var TranslatorInterface $translator */ + $translator = self::getContainer()->get('translator'); + self::assertSame('nl', $translator->getLocale()); + } + + public function testErrorPageRecoversRouterContextLocaleFromPath(): void + { + // Without this, `path()` in a Dutch-rendered error page emits `/en/...`. + $this->registerFixtureTemplatePath(); + $this->setGeneralConfig('notfound', ['locale_probe.html.twig']); + + $this->renderError(new NotFoundHttpException(), '/nl/this-page-does-not-exist'); + + /** @var RouterInterface $router */ + $router = self::getContainer()->get('router'); + self::assertSame('nl', $router->getContext()->getParameter('_locale')); + } + + public function testErrorPageKeepsLocaleWhenRouteMatched(): void + { + // With a matched route the locale is already set, and `it` is a ContentType + // slug rather than Italian. Recovering it from the path would be wrong. + $this->registerFixtureTemplatePath(); + $this->setGeneralConfig('notfound', ['locale_probe.html.twig']); + + $body = (string) $this->renderError(new NotFoundHttpException(), '/it/no-such-record', 'listing')->getContent(); + + self::assertStringContainsString('LOCALE_PROBE:Error 404', $body); + self::assertStringNotContainsString('Errore 404', $body); + } + + public function testErrorPageTranslatesUnderscoreFunctionInRequestedLocale(): void + { + // End-to-end: a `{{ __('...') }}` string in the rendered error page must be + // translated in the locale recovered from the URL. `http_error.name` is + // "Error %status_code%" (en) / "Fout %status_code%" (nl). + $this->registerFixtureTemplatePath(); + $this->setGeneralConfig('notfound', ['locale_probe.html.twig']); + + $body = (string) $this->renderError(new NotFoundHttpException(), '/nl/this-page-does-not-exist')->getContent(); + + self::assertStringContainsString('LOCALE_PROBE:Fout 404', $body); + self::assertStringNotContainsString('Error 404', $body); + } + + public function testErrorPageTranslatesUnderscoreFunctionInDefaultLocale(): void + { + // Counterpart of the test above: no locale segment, so `en`. + $this->registerFixtureTemplatePath(); + $this->setGeneralConfig('notfound', ['locale_probe.html.twig']); + + $body = (string) $this->renderError(new NotFoundHttpException(), '/this-page-does-not-exist')->getContent(); + + self::assertStringContainsString('LOCALE_PROBE:Error 404', $body); + self::assertStringNotContainsString('Fout 404', $body); + } + + public function testNotFoundPageWithNonLocalizedContentTypeDoesNotError(): void + { + // The default `notfound` is `blocks/404-not-found`, and `blocks` isn't + // localized - so `nl` sends it down the "wrong locale" path, which has no + // route to redirect to here. It must render the record rather than error. + $this->client->request('GET', '/nl/this-page-does-not-exist'); + $response = $this->client->getResponse(); + + self::assertSame(404, $response->getStatusCode()); + self::assertStringContainsString('404 Page not found', (string) $response->getContent()); + } + + /** Point the 404 page at a record with a per-locale `heading`. */ + private function seedLocalizedNotFoundPage(): void + { + $page = $this->seedLocalizedPage(); + + $this->setGeneralConfig('notfound', ['page/' . $page->getId()]); + } + + /** Give a published `pages` record distinct `heading` values per locale. */ + private function seedLocalizedPage(): Content + { + $page = $this->getPublishedPage(); + + $page->setFieldValue('heading', self::HEADING_EN, 'en'); + $page->setFieldValue('heading', self::HEADING_NL, 'nl'); + $page->getField('heading')->mergeNewTranslations(); + $this->getEm()->flush(); + + return $page; + } + + /** + * Invoke the configured `error_controller` directly, the way the kernel does, + * with a frontend request for the given path on the stack. + * + * Pass `$route` to simulate an error raised *after* routing matched. + */ + private function renderError(Throwable $exception, string $path, ?string $route = null): Response + { + $request = Request::create('http://localhost' . $path); + RequestZone::setToRequest($request, RequestZone::FRONTEND); + + if ($route !== null) { + $request->attributes->set('_route', $route); + } + + /** @var RequestStack $requestStack */ + $requestStack = self::getContainer()->get(RequestStack::class); + $requestStack->push($request); + + try { + /** @var ErrorController $errorController */ + $errorController = self::getContainer()->get(ErrorController::class); + /** @var Environment $twig */ + $twig = self::getContainer()->get('twig'); + + return $errorController->showAction($twig, $exception); + } finally { + $requestStack->pop(); + } + } + + /** + * Make `Fixtures/` resolvable by name, so a fixture template can be used as the + * `notfound` config. Adds to the existing loader rather than replacing it, the + * way TwigAwareController::setTwigLoader() does, so the path survives rendering. + */ + private function registerFixtureTemplatePath(): void + { + /** @var Environment $twig */ + $twig = self::getContainer()->get('twig'); + + $loader = $twig->getLoader(); + $loaders = $loader instanceof ChainLoader ? $loader->getLoaders() : [$loader]; + + foreach ($loaders as $candidate) { + if ($candidate instanceof FilesystemLoader) { + $candidate->addPath(__DIR__ . '/Fixtures'); + + return; + } + } + + self::fail('Could not find a FilesystemLoader to register the fixture template path on.'); + } + + private function getPublishedPage(): Content + { + $page = $this->getEm()->getRepository(Content::class) + ->findOneBy(['contentType' => 'pages', 'status' => Statuses::PUBLISHED]); + + self::assertInstanceOf(Content::class, $page, 'Expected a published "pages" record in the fixtures.'); + + return $page; + } + + /** + * Override a `general/*` configuration value at runtime. + * + * @param array $value + */ + private function setGeneralConfig(string $key, array $value): void + { + $config = self::getContainer()->get(Config::class); + + // Mutate only the `general` collection in place; rebuilding the whole data + // tree would turn `contenttypes` into plain collections and break typing. + $property = new \ReflectionProperty(Config::class, 'data'); + $property->getValue($config)->get('general')->put($key, $value); + } +} diff --git a/tests/php/Controller/Frontend/Fixtures/locale_probe.html.twig b/tests/php/Controller/Frontend/Fixtures/locale_probe.html.twig new file mode 100644 index 000000000..6e42dc17f --- /dev/null +++ b/tests/php/Controller/Frontend/Fixtures/locale_probe.html.twig @@ -0,0 +1,3 @@ +{# Probe for ErrorControllerTest: `http_error.name` is "Error %status_code%" (en), + "Fout %status_code%" (nl), "Errore %status_code%" (it). #} +LOCALE_PROBE:{{ __('http_error.name', {'%status_code%': 404}) }} diff --git a/tests/php/Controller/Frontend/ListingControllerTest.php b/tests/php/Controller/Frontend/ListingControllerTest.php new file mode 100644 index 000000000..c9c5ca46e --- /dev/null +++ b/tests/php/Controller/Frontend/ListingControllerTest.php @@ -0,0 +1,63 @@ +client->request('GET', '/fr/pages'); + $response = $this->client->getResponse(); + + self::assertSame(302, $response->getStatusCode()); + self::assertStringContainsString('/en/pages', (string) $response->headers->get('Location')); + } + + public function testListingFallsBackToDefaultLocaleWhenForwardedWithoutRoute(): void + { + // A homepage listing is forwarded to ListingController without a `_route`, + // so there's nothing to redirect to: it must render in the default locale. + $this->setHomepage('pages'); + + $this->client->request('GET', '/fr/'); + $response = $this->client->getResponse(); + + self::assertSame(200, $response->getStatusCode()); + self::assertStringContainsString('getContent()); + } + + private function setHomepage(string $homepage): void + { + $config = self::getContainer()->get(Config::class); + + // Mutate only the `general` collection in place; rebuilding the whole data + // tree would turn `contenttypes` into plain collections and break typing. + $property = new \ReflectionProperty(Config::class, 'data'); + $property->getValue($config)->get('general')->put('homepage', $homepage); + } +}