diff --git a/phpstan-baseline.php b/phpstan-baseline.php index 1bec5a750..b61adf37a 100644 --- a/phpstan-baseline.php +++ b/phpstan-baseline.php @@ -1423,12 +1423,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', @@ -1513,12 +1507,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..1ebf3bca5 100644 --- a/src/Controller/ErrorController.php +++ b/src/Controller/ErrorController.php @@ -21,6 +21,8 @@ use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\Routing\Generator\UrlGeneratorInterface; +use Symfony\Contracts\Translation\LocaleAwareInterface; +use Symfony\Contracts\Translation\TranslatorInterface; use Throwable; use Twig\Environment; use Twig\Error\LoaderError; @@ -37,10 +39,17 @@ public function __construct( private readonly UrlGeneratorInterface $urlGenerator, private readonly Security $security, private readonly RequestStack $requestStack, + private readonly TranslatorInterface $translator, + 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 +70,11 @@ 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()) { + // On a 404/403, no route matched, so Symfony's LocaleListener never set + // the locale from the URL. Recover it from the path so localized error + // pages (and their records) render in the right language. + $this->setLocaleFromPath($request); + if ($code === Response::HTTP_SERVICE_UNAVAILABLE || $this->isMaintenanceEnabled($code)) { $twig->addGlobal('exception', $exception); @@ -156,6 +170,41 @@ private function isMaintenanceEnabled(int $code): bool return filter_var($this->config->get('general/maintenance_mode', false), FILTER_VALIDATE_BOOLEAN); } + /** + * Sets the locale based on the first segment of the path, if it matches one + * of the configured locales (e.g. `/de/...` => `de`). + * + * The locale is applied to both the given request (used when rendering a + * record) and the current request (which Twig's `app.request` resolves to, + * and is a sub-request when an error page is being rendered). It's also set on + * the translator, so `{% trans %}` strings in the error template are localized + * too - normally Symfony's `LocaleListener`/`LocaleAwareListener` does this, + * but neither runs when no route matched. + */ + private function setLocaleFromPath(Request $request): void + { + // Cast: on PHP 8.4 `mb_trim()` is analysed as `string|false`, but `getPathInfo()` + // always yields a string, so the result is effectively always a string here. + $segment = explode('/', (string) mb_trim($request->getPathInfo(), '/'))[0]; + + if ($segment !== '' && in_array($segment, $this->localeCodes, true)) { + $request->setLocale($segment); + + $currentRequest = $this->requestStack->getCurrentRequest(); + if ($currentRequest instanceof Request && $currentRequest !== $request) { + $currentRequest->setLocale($segment); + } + } + + // The concrete translator is locale-aware; the contracts interface we depend + // on isn't, so guard the call to keep the dependency narrow. Always sync it to + // the (possibly default) request locale so a previous request's locale can't + // leak into a default-locale error page in long-running runtimes. + if ($this->translator instanceof LocaleAwareInterface) { + $this->translator->setLocale($request->getLocale()); + } + } + private function attemptToRender(Request $request, string $item): ?Response { // First, see if it's a contenttype/slug pair: @@ -165,7 +214,9 @@ 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 request's locale explicitly, so `DetailController` keeps + // it instead of falling back to the default locale. + 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..c0aa5496a 100644 --- a/src/Controller/Frontend/ListingController.php +++ b/src/Controller/Frontend/ListingController.php @@ -44,9 +44,12 @@ public function listing(Request $request, ContentRepository $contentRepository, throw new NotFoundHttpException('Content is not viewable'); } - // If the locale is the wrong locale - if (! $this->validLocaleForContentType($request, $contentType)) { - return $this->redirectToDefaultLocale($request); + // If the locale is the wrong locale, redirect to the default locale, or - + // when that's not possible (e.g. a forwarded request without a matched + // route) - render the listing in the default locale. + 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..16c85b0e8 100644 --- a/src/Controller/TwigAwareController.php +++ b/src/Controller/TwigAwareController.php @@ -113,9 +113,11 @@ public function renderSingle(Request $request, ?Content $record, bool $requirePu throw new NotFoundHttpException('Content is not viewable'); } - // If the locale is the wrong locale - if (! $this->validLocaleForContentType($request, $recordDefinition)) { - return $this->redirectToDefaultLocale($request); + // If the locale is the wrong locale, redirect to the default locale, or - + // when that's not possible - render the record in the default locale. + if (! $this->validLocaleForContentType($request, $recordDefinition) + && ($redirect = $this->redirectToDefaultLocaleOrFallback($request)) instanceof Response) { + return $redirect; } $singularSlug = $record->getContentTypeSingularSlug(); @@ -145,8 +147,41 @@ protected function validLocaleForContentType(Request $request, ContentType $cont return $request->getLocale() === $this->defaultLocale; } + /** + * Either redirect to the same route in the default locale, or - when there's + * no route to redirect to (e.g. a forwarded request, or an error page where + * routing never matched) - reset the request to the default locale and return + * `null`, so the caller can render in the default locale instead. + * + * Note: this resets the locale on the _given_ request only. When rendering an + * error page, Twig's `app.request` is a sub-request whose locale was set + * separately (see ErrorController::setLocaleFromPath()), so the `` + * may still reflect the URL locale while the - non-localizable - record content + * is rendered in the default locale. That's harmless: such content is identical + * across locales. + */ + 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 was matched (e.g. on an error page): there's nothing to + // redirect to, so let the caller decide how to handle this. + $route = $request->attributes->get('_route'); + if (! $route) { + return null; + } + $request->getSession()->set('_locale', $this->defaultLocale); $params = $request->attributes->get('_route_params'); @@ -155,7 +190,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..d8cbf8b90 --- /dev/null +++ b/tests/php/Controller/Frontend/ErrorControllerTest.php @@ -0,0 +1,267 @@ +seedLocalizedNotFoundPage(); + + $this->client->request('GET', '/nl/this-page-does-not-exist'); + $response = $this->client->getResponse(); + $body = (string) $response->getContent(); + + self::assertSame(404, $response->getStatusCode()); + // `htmllang()` (rendered as ``) reflects the current locale. + 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 doesn't arise from a routed request in the test app (all + // `access_control` rules are backend, and the backend 403 path redirects to + // the dashboard rather than rendering the custom page). So we drive the + // configured `error_controller` directly, exactly as Symfony's error + // handling does, with the URL whose locale should be recovered. A 403 + // `AccessDeniedHttpException` routes to `showForbidden()`, which renders the + // configured `general/forbidden` page (set above to our localized record). + // + // Note: `showAction()` returns the rendered page as a plain 200; promoting + // the status to the exception's 403 is the kernel's job (see + // HttpKernel::handleThrowable()), which we bypass here. So we assert the + // locale handling that *is* this controller's responsibility, not the code. + $response = $this->renderError(new AccessDeniedHttpException(), '/nl/this-page-is-forbidden'); + $body = (string) $response->getContent(); + + // `htmllang()` (rendered as ``) reflects the current locale. + 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 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 + { + // The counterpart of the test above: with no locale segment in the URL, the + // same `__()` string must fall back to the default locale (en) - and must not + // leak a previous request's locale onto the shared translator. + $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` points at `blocks/404-not-found`. The `blocks` + // ContentType is *not* localized, so the requested `nl` locale can't be + // honored for the record. This must not error (regression: it used to + // attempt an invalid redirect, throwing a TypeError); instead it renders + // the not-found record. + $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 published, localized `pages` record with distinct + * `heading` values per locale. + */ + 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 Symfony's error + * handling does, with a frontend request for the given path on the stack so + * the locale can be recovered from it. + */ + private function renderError(Throwable $exception, string $path): Response + { + $request = Request::create('http://localhost' . $path); + RequestZone::setToRequest($request, RequestZone::FRONTEND); + + /** @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 the `Fixtures/` directory resolvable by name, so a fixture template can be + * pointed at via the `notfound` config. Mirrors how Bolt prepends paths to the + * existing filesystem loader (see TwigAwareController::setTwigLoader()) rather than + * replacing it, so the added 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..df8a3218a --- /dev/null +++ b/tests/php/Controller/Frontend/Fixtures/locale_probe.html.twig @@ -0,0 +1,4 @@ +{# Test fixture for ErrorControllerTest: renders a `__()` string so the test can + assert error pages translate via the translator locale recovered from the URL. + `http_error.name` is "Error %status_code%" (en) / "Fout %status_code%" (nl). #} +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..175cc7bac --- /dev/null +++ b/tests/php/Controller/Frontend/ListingControllerTest.php @@ -0,0 +1,77 @@ +client->request('GET', '/fr/pages'); + $response = $this->client->getResponse(); + + self::assertSame(302, $response->getStatusCode()); + self::assertStringContainsString('/en/pages', (string) $response->headers->get('Location')); + } + + /** + * When a listing is reached *without* a matched route (a forwarded request, + * e.g. the homepage rendered as a listing) and the locale isn't supported, + * there's no route to redirect to — so it must fall back to rendering in the + * default locale instead of erroring. (The fallback after the redirect branch.) + */ + public function testListingFallsBackToDefaultLocaleWhenForwardedWithoutRoute(): void + { + // Configure the homepage as a (non-singleton) listing, so HomepageController + // forwards the request to ListingController without a `_route`. + $this->setHomepage('pages'); + + $this->client->request('GET', '/fr/'); + $response = $this->client->getResponse(); + + // No redirect, no error: the listing is rendered in the default locale. + 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); + } +}