Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-language redirects #2435

Open
parkersweb opened this issue Apr 8, 2019 · 2 comments · May be fixed by #3004
Open

Multi-language redirects #2435

parkersweb opened this issue Apr 8, 2019 · 2 comments · May be fixed by #3004
Assignees

Comments

@parkersweb
Copy link

We've got a live site running 1.3.1 which has the re-direct /about-us/company: /about-us. Our new site has an upgraded version of Grav (I've tried 1.5.10 and also 1.6-rc4).

On the old site that re-direct would match for all languages i.e. only the slug part is important - as per the docs (https://learn.getgrav.org/15/content/routing#site-level-routes-and-redirects). On the upgraded site however, whilst the root https://example.com/about-us/company redirects it no longer matches the language versions i.e. https://example.com/en-gb/about-us/company and https://example.com/en-us/about-us/company

If I modify the redirect to use/en-(gb|us)/about-us/company: /about-us it does correctly redirect.

Not sure if the logic has changed in this area - but any help you could give would be very much appreciated!

@mahagr mahagr self-assigned this Dec 13, 2019
@nbusseneau
Copy link
Contributor

Bumping this up: I can confirm it is still happening in 1.6.27.

I have a multi-language website (en and fr). When setting up site.yaml like this, redirects for requests to /en/foo/2020 won't work:

redirects:
  /foo/(\d+): /bar/$1

But they do with the following:

redirects:
  (/.*)?/foo/(\d+): /bar/$2

I expected the first to work, as the documentation mentions:

All redirect rules apply on the slug-path beginning after the language part (if you use multi-language pages)

Investigation

I noticed there was a big refactor in Grav 1.5 around multi-language redirects, perhaps explaining why OP had it working in prior versions. I tried to skim through it, but couldn't make out how the changes would impact matching source URL with language prefixes.

Instead, I had a look at the dispatch() code responsible for handling redirects. Here is the code for regex redirects:

/** @var Uri $uri */
$uri = $this->grav['uri'];
/** @var \Grav\Framework\Uri\Uri $source_url */
$source_url = $uri->uri(false);
// Try Regex style redirects
$site_redirects = $config->get('site.redirects');
if (is_array($site_redirects)) {
foreach ((array)$site_redirects as $pattern => $replace) {
$pattern = '#^' . str_replace('/', '\/', ltrim($pattern, '^')) . '#';
try {
$found = preg_replace($pattern, $replace, $source_url);
if ($found !== $source_url) {
$this->grav->redirectLangSafe($found);
}
} catch (ErrorException $e) {
$this->grav['log']->error('site.redirects: ' . $pattern . '-> ' . $e->getMessage());
}
}
}

As we can see, it tries to match redirect source $pattern (after sanitizing) against $source_url retrieved via $uri->uri(false). However, $source_url contains the language prefix, and thus cannot match $pattern as-is.

So I tried to use $uri->path() instead, as it does not contain the language prefix, and hooray! It does seem to work. Considering the $route parameter to the dispatch() function is actually already supposed to be $uri->path() (as per PagesServiceProvider), we can directly use $route for matching purposes instead of $source_url (which becomes an unused variable and can be cleaned up).

Thus, the question is: why are regex redirects and routes relying on $uri->uri(false) rather than the given dispatch() $route? Is there any specific reason? If not, then I am ready to submit a PR to fix this.

nbusseneau added a commit to nbusseneau/grav that referenced this issue Sep 12, 2020
Multilang redirects and routes were not working due to language prefixes
being included in URL when trying to match source URL and redirect/route
source pattern.

Using `$route` parameter fixes the issue as given route does not include
language prefixes (coming from `$uri->path()` in `PagesServiceProvider`).

Fixes getgrav#2435.
nbusseneau added a commit to nbusseneau/grav that referenced this issue Sep 12, 2020
Multilang redirects and routes were not working due to language prefixes
being included in URL when trying to match source URL and redirect/route
source pattern.

Using `$route` parameter fixes the issue as given route does not include
language prefixes (coming from `$uri->path()` in `PagesServiceProvider`).

Fixes getgrav#2435.
@nbusseneau nbusseneau linked a pull request Sep 12, 2020 that will close this issue
@mahagr
Copy link
Member

mahagr commented Sep 14, 2020

I agree that based on the documentation, your proposal seems to be the better choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants