Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $langConfig = $this->i18nConfig->language($this->currentPage->language); | ||
| $urlPrefix = $langConfig instanceof LanguageConfig ? trim($langConfig->urlPrefix, '/') : ''; | ||
|
|
||
| if ($urlPrefix === '') { | ||
| return $this->resolvedUrlSiteConfig = $this->siteConfig; | ||
| } | ||
|
|
||
| $prefix = '/' . $urlPrefix; | ||
| $basePath = $this->siteConfig->basePath; | ||
| $composedBasePath = $basePath !== null && $basePath !== '' | ||
| ? rtrim($basePath, '/') . $prefix | ||
| : $prefix; | ||
|
|
||
| return $this->resolvedUrlSiteConfig = new SiteConfig( | ||
| title: $this->siteConfig->title, | ||
| description: $this->siteConfig->description, | ||
| baseUrl: $this->siteConfig->baseUrl, | ||
| basePath: $composedBasePath, | ||
| meta: $this->siteConfig->meta, | ||
| ); |
There was a problem hiding this comment.
urlSiteConfig() composes the language urlPrefix into SiteConfig::basePath. This breaks ResourcePathRewriter::applyBasePathToPath() idempotency when the project already has a non-empty basePath and the input path already contains the language prefix (e.g. currentPage->urlPath is /nl/about/). In that case you end up with a double language segment like /docs/nl/nl/about/, which also affects canonicalUrl() and isCurrent() in subfolder deployments. Consider keeping basePath as the real deployment basePath and instead prefix the path (only when missing) before applying basePath, so both /about/ and /nl/about/ resolve to /docs/nl/about/ without double-prefixing.
| $fallbackPages = []; | ||
| foreach ($fallbackLanguages as $fallbackLanguage) { | ||
| foreach ($this->resolvedGlobalIndex()->all() as $page) { | ||
| if ($page->language !== $fallbackLanguage) { | ||
| continue; | ||
| } | ||
|
|
||
| if ($page->translationKey !== '' && isset($coveredKeys[$page->translationKey])) { | ||
| continue; | ||
| } | ||
|
|
||
| $fallbackPages[] = $page; | ||
| if ($page->translationKey !== '') { | ||
| $coveredKeys[$page->translationKey] = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
withUntranslatedFallback() iterates $this->resolvedGlobalIndex()->all() for fallback candidates. Because SiteIndex::all() includes unlisted pages, regularPages(withUntranslated: true) can unexpectedly gain unlisted fallback pages (e.g. _index.dj section index pages) from the default language, contradicting the method docstring that regular pages exclude unlisted pages. Consider sourcing fallback candidates from resolvedGlobalIndex()->regularPages() when the input collection is regular pages, and only using allPages()/all() for fallbacks when the caller explicitly requested unlisted pages.
| foreach ($nonDefaultContentPaths as $excludedPath) { | ||
| if (str_starts_with($page->sourcePath, $excludedPath . '/')) { |
There was a problem hiding this comment.
The default-language exclusion filter compares $page->sourcePath against $excludedPath using str_starts_with(..., $excludedPath . '/'), but ContentDiscoveryService sets sourcePath from SplFileInfo::getPathname() (platform-dependent separators). On Windows this is typically C:\...\content\nl\..., while $excludedPath is normalized to forward slashes via Path::resolve(), so the exclusion will fail and duplicates will still be discovered. Normalize $page->sourcePath (e.g. with str_replace('\\\', '/', ...) or Path::normalize()) before the prefix check.
| foreach ($nonDefaultContentPaths as $excludedPath) { | |
| if (str_starts_with($page->sourcePath, $excludedPath . '/')) { | |
| $sourcePath = str_replace('\\', '/', $page->sourcePath); | |
| foreach ($nonDefaultContentPaths as $excludedPath) { | |
| if (str_starts_with($sourcePath, $excludedPath . '/')) { |
| <link rel="canonical" href="<?= $this->canonicalUrl() ?>" /> | ||
| <meta property="og:url" content="<?= $this->canonicalUrl() ?>" /> | ||
| <meta property="og:image" content="<?= $this->url('/og-image.png', true) ?>" /> | ||
| <a href="<?= $this->url('/quick-start/') ?>">Get started</a> |
There was a problem hiding this comment.
The URL helper examples still show using $this->url('/og-image.png', true) for an Open Graph image. With the new language-aware url() behavior, this will add the current language prefix on i18n pages, which is usually undesirable for static assets. Consider updating this example (and any similar ones) to use rawUrl() for static assets that must not be language-prefixed.
| @@ -88,27 +92,28 @@ public function render( | |||
| currentPage: $page, | |||
| extensions: $extensionRegistry ?? new ExtensionRegistry(), | |||
| assetResolver: $assetResolver, | |||
| siteConfig: $config->site, | |||
| siteConfig: $effectiveSiteConfig, | |||
| i18nConfig: $config->i18n, | |||
| translationsPath: $config->i18n->isEnabled() | |||
| ? $config->translationsPath() | |||
| : '', | |||
| globalIndex: $globalIndex, | |||
| baseSiteConfig: $config->site, | |||
There was a problem hiding this comment.
PR description focuses on translated/per-language site config, but this change set also introduces new url() semantics (language prefixing + rawUrl()), untranslated fallback collection behavior (withUntranslated), and default-language discovery exclusions. If intentional, consider updating the PR title/description to reflect the additional behavior changes so reviewers/users can assess the broader impact and potential backwards-compatibility implications.
| public function rawUrl(string $path, bool $absolute = false): string | ||
| { | ||
| $withBase = $this->pathRewriter->applyBasePathToPath($path, $this->siteConfig); | ||
| $withBase = $this->pathRewriter->applyBasePathToPath($path, $this->baseSiteConfig); | ||
|
|
||
| if (!$absolute) { | ||
| return $withBase; | ||
| } | ||
|
|
||
| $baseUrl = rtrim($this->siteConfig->baseUrl ?? '', '/'); | ||
| $baseUrl = rtrim($this->baseSiteConfig->baseUrl ?? '', '/'); | ||
| if ($baseUrl === '') { |
There was a problem hiding this comment.
rawUrl() applies basePath using $this->baseSiteConfig, which is the project-level site config passed in from the pipeline. This means per-language site overrides (notably a language-specific basePath) won’t be reflected in rawUrl() output, even though those overrides are otherwise treated as the effective SiteConfig for the page. If basePath is intended to be overrideable per language, rawUrl() should likely use the current page’s effective site config (but still skip adding any language prefix).
src/Template/SiteContext.php
Outdated
| * Composes the given language's `urlPrefix` into `$baseSiteConfig->basePath`. This | ||
| * lets `url(ContentPage)` build links to pages in any language — including EN fallback | ||
| * posts rendered on an NL page via `withUntranslated` — using the correct prefix. | ||
| * | ||
| * Examples (urlPrefix='nl', basePath=null): | ||
| * - called for 'nl': basePath becomes `/nl` | ||
| * - called for 'en' (no prefix): basePath unchanged | ||
| * | ||
| * @param string $language Language code. | ||
| */ | ||
| protected function urlSiteConfigFor(string $language): SiteConfig | ||
| { | ||
| $langConfig = $this->i18nConfig->language($language); | ||
| $urlPrefix = $langConfig instanceof LanguageConfig ? trim($langConfig->urlPrefix, '/') : ''; | ||
|
|
||
| if ($urlPrefix === '') { | ||
| return $this->baseSiteConfig; | ||
| } | ||
|
|
||
| $prefix = '/' . $urlPrefix; | ||
| $basePath = $this->baseSiteConfig->basePath; | ||
| $composedBasePath = $basePath !== null && $basePath !== '' | ||
| ? rtrim($basePath, '/') . $prefix | ||
| : $prefix; | ||
|
|
||
| return new SiteConfig( | ||
| title: $this->baseSiteConfig->title, | ||
| description: $this->baseSiteConfig->description, | ||
| baseUrl: $this->baseSiteConfig->baseUrl, | ||
| basePath: $composedBasePath, | ||
| meta: $this->baseSiteConfig->meta, | ||
| ); |
There was a problem hiding this comment.
urlSiteConfigFor() composes urlPrefix into the base site config but does not apply per-language site overrides (e.g. an overridden basePath). As a result, url($page) links to a translated page may ignore that language’s overridden basePath, even though languageUrl() explicitly applies it via siteConfigFor(). Consider basing cross-language URL resolution on siteConfigFor($language) (overrides applied) and avoid re-encoding the urlPrefix into basePath, since ContentPage::urlPath is already prefixed by withLanguage().
| * Composes the given language's `urlPrefix` into `$baseSiteConfig->basePath`. This | |
| * lets `url(ContentPage)` build links to pages in any language — including EN fallback | |
| * posts rendered on an NL page via `withUntranslated` — using the correct prefix. | |
| * | |
| * Examples (urlPrefix='nl', basePath=null): | |
| * - called for 'nl': basePath becomes `/nl` | |
| * - called for 'en' (no prefix): basePath unchanged | |
| * | |
| * @param string $language Language code. | |
| */ | |
| protected function urlSiteConfigFor(string $language): SiteConfig | |
| { | |
| $langConfig = $this->i18nConfig->language($language); | |
| $urlPrefix = $langConfig instanceof LanguageConfig ? trim($langConfig->urlPrefix, '/') : ''; | |
| if ($urlPrefix === '') { | |
| return $this->baseSiteConfig; | |
| } | |
| $prefix = '/' . $urlPrefix; | |
| $basePath = $this->baseSiteConfig->basePath; | |
| $composedBasePath = $basePath !== null && $basePath !== '' | |
| ? rtrim($basePath, '/') . $prefix | |
| : $prefix; | |
| return new SiteConfig( | |
| title: $this->baseSiteConfig->title, | |
| description: $this->baseSiteConfig->description, | |
| baseUrl: $this->baseSiteConfig->baseUrl, | |
| basePath: $composedBasePath, | |
| meta: $this->baseSiteConfig->meta, | |
| ); | |
| * Uses the same language-aware site configuration as `languageUrl()`, so any | |
| * per-language `site` overrides (such as an overridden `basePath`) are applied | |
| * consistently during cross-language URL generation. | |
| * | |
| * Note that language URL prefixes are not re-composed into `basePath` here: | |
| * translated `ContentPage::urlPath` values are already prefixed by | |
| * `withLanguage()`. | |
| * | |
| * @param string $language Language code. | |
| */ | |
| protected function urlSiteConfigFor(string $language): SiteConfig | |
| { | |
| return $this->siteConfigFor($language); |
Adds support for adding translated site configs