grav
grav copied to clipboard
getgrav
Fix multilang redirects and routes
Multilang redirects and routes were not working as per the documentation ("All redirect rules apply on the slug-path beginning after the language part (if you use multi-language pages)") 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 #2435.
Note: see issue #2435 for discussion regarding potential shortcomings.
Thus, the question is: why are regex redirects and routes relying on
$uri->uri(false)rather than the givendispatch()$route? Is there any specific reason?
@rhukster Can you confirm my findings?
The issue is that the redirect is done based on the CURRENT URL and not the route which was given as a parameter to the method.
OK, looking into the history of this, the code used to be using $route however, about 3 years ago it was changed to use the URL because the prior approach didn't take into account any query or params on the URL. I think the ultimate solution would be to use the $route plus rebuild any query or params that were requested in the original URL.
I think the ultimate solution would be to use the $route plus rebuild any query or params that were requested in the original URL.
I can have a poke at that. I take it query parameters are ?foo=bar&bar=qux and params are Grav-specific parameters /foo:bar/baz:qux?
I think that the whole concept of having redirect in that method is wrong. Redirecting should be part of routing, not in method that gets a page. I will need to think a bit, but in the mean time there is this in Grav 1.7:
https://github.com/getgrav/grav/blob/1.7/system/src/Grav/Common/Service/PagesServiceProvider.php#L79-L105
It is not being called in this case as there's $page = $pages->dispatch($path); call in line 63, but I'm wondering if we should have the whole logic here and just call $pages->find(), which doesn't do the redirecting.
@mahagr ~~I don't know if something changed in 1.7 since your last message, but I can't reproduce the issue anymore after upgrading to 1.7.3. I didn't investigate to understand why dispatch() is now correctly handling multilang redirects even though code did not change, but I can if it's not something you changed intentionally.~~
Forget what I said, I'm stupid and forgot that redirects work when using the workaround stated in #2435 (prefixing with (/.*)? to catch language fragment). Without the workaround, redirects still fail.
You are right, looks like configuration redirects still happen and are wrong, even when you're using $pages->find().
The above pull request will not fix the language redirects needing the language code, but will only separate the logic between find() and dispatch() methods and make site.routes more usable.
For the language codes, I still think that site.redirects should not be based on routes, but they should blindly match the current URL (including query parameters etc) and do transformation based on that.
However it could be a good idea to be able to pass more information to the redirects, such as '/({LANG})/foo/(\d+)': '/$1/bar/$2' which would match all language codes.
The reason why I'd like to do it in that way is that you do not want to restrict redirects to Grav routes as you may have migrated from another CMS.
@mahagr Revisiting old PRs -- I see there was some work on this via #3266. If you have some additional thoughts on how to advance this further I am open for taking a jab at it. The workaround outlined above has been working fine forever, but still think it would be nice to have it work as intended by the documentation (or fix the documentation, which I can also do if we think that's better).