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-domain language setup: non-default language domain 302 redirects to default domain #1382

Closed
texnixe opened this issue Jan 20, 2019 · 17 comments

Comments

@texnixe
Copy link
Contributor

@texnixe texnixe commented Jan 20, 2019

Describe the bug
As in the title

To Reproduce
Steps to reproduce the behavior:

  1. Set up two languages in Starterkit and assign a language-specific URL to each
  2. In config, set languages to true and languages.detect to false (should be the default, anyway)
  3. Try to open non-default domain in a browser
  4. See it redirecting to the default domain (Chrome network tab shows a 302 redirect)

Expected behavior
Needless to say that no redirect should happen unless language detection is active.

Kirby Version
3.0.0

Desktop (please complete the following information):

  • macOS
  • chrome, safari

Additional context
https://forum.getkirby.com/t/using-different-domains-for-each-language-in-kirby3-does-not-work-for-me-here-need-ideas-to-solve-the-issue/12282

@bastianallgeier bastianallgeier modified the milestones: 3.0.x, 3.0.1 Jan 21, 2019
@bastianallgeier bastianallgeier modified the milestones: 3.0.1, 3.0.x, 3.0.2 Jan 29, 2019
@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Feb 2, 2019

Specifying: it works for me with language-specific URL parts. But I can reproduce the bug with whole domains per language.

Edit: I guess the title also says that XD

@distantnative distantnative changed the title In a multi-domain language setup non-default language domain 302 redirects to default domain Multi-domain language setup: non-default language domain 302 redirects to default domain Feb 2, 2019
@bastianallgeier bastianallgeier modified the milestones: 3.0.2, 3.0.x, 3.0.3 Feb 12, 2019
@bastianallgeier bastianallgeier modified the milestones: 3.0.3, 3.1.0 Feb 25, 2019
@bastianallgeier bastianallgeier modified the milestones: 3.1.0, 3.1.1 Mar 12, 2019
@7pdf

This comment has been minimized.

Copy link

@7pdf 7pdf commented Mar 20, 2019

Hello Bastian! Hopefully it is possible for you and your team to fix that bug for the next build 3.1.1 - We are waiting for this fix some months now...Let me know if we can help you here... :-) THANK YOU VERY MUCH!! Kind regards!

@bastianallgeier bastianallgeier modified the milestones: 3.1.2, 3.2.0 Apr 1, 2019
@distantnative distantnative modified the milestones: 3.2.0, 3.2.1 May 10, 2019
@bastianallgeier bastianallgeier modified the milestones: 3.2.1, 3.2.0, 3.3.0 May 28, 2019
@distantnative distantnative removed this from the 3.3.0 milestone Jun 12, 2019
@bastianallgeier bastianallgeier added this to the 3.3.0 milestone Jul 30, 2019
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Jul 30, 2019

@7pdf I'm really sorry for postponing this all the time. We really struggle with the implementation. It was already hackish in v2 and we want to create a clean solution for it in v3. We just discussed that we want to give this issue a higher priority to not let you wait much longer. Sorry again and thanks for your patience.

@7pdf

This comment has been minimized.

Copy link

@7pdf 7pdf commented Aug 8, 2019

Sounds phantasic - thank you so much!

@findthebug

This comment has been minimized.

Copy link

@findthebug findthebug commented Aug 8, 2019

thanks. if you need tester, just let me know.

@hummeljoachim

This comment has been minimized.

Copy link

@hummeljoachim hummeljoachim commented Sep 3, 2019

Looking forward to this fix, we also need this in an upcoming project.

@findthebug

This comment has been minimized.

Copy link

@findthebug findthebug commented Sep 3, 2019

Regarding the issue described, is there a problem with https://getkirby.com/docs/cookbook/setup/multisite as well? or is this feature working as expected?

@afbora

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Oct 1, 2019

I've done a lot of tests on this issue. In fact, it works successfully in general.
But I think the real problem here is that the language that is being request does not know.

  • defaultLanguage() : exists
  • detectedLanguage() : exists
  • requestedLanguage() : non exists

We always try to solve the router process by checking whether it is the default language or not. I'm not sure but guess we need to something like that requestedLanguage() method 🤔

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Oct 1, 2019

@afbora Thanks for your research! Could you please explain your findings some more? I can't quite follow what you mean.

@afbora

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Oct 2, 2019

@lukasbestle I tried to rewrite the router part that met the request.
Still missing, but it works out of a few minor bugs.

if ($kirby->multilang() === true) 
{    
    // Multi-language home
    $after[] = [
        'pattern' => '',
        'method'  => 'ALL',
        'env'     => 'site',
        'action'  => function () use ($kirby) 
        {
            $home = $kirby->site()->homePage();
            $defaultLanguage = $kirby->defaultLanguage();
            $detectedLanguage = $kirby->detectedLanguage();
            
            if ($kirby->option('languages.detect') === true) 
            {
                if ($defaultLanguage->url() !== $detectedLanguage->url()) 
                {
                    if($kirby->url() === $detectedLanguage->url()) 
                    {
                        return $detectedLanguage->router()->call();
                    } 
                    elseif ($home && $kirby->url() !== $home->url()) 
                    {
                        return $kirby
                            ->response()
                            ->redirect($detectedLanguage->url());
                    } 
                    else 
                    {
                        return $defaultLanguage->router()->call();
                    }
                } 
                else 
                {
                    return $defaultLanguage->router()->call();
                }
            } 
            elseif ($home && $kirby->url() !== $home->url()) 
            {
                if($kirby->url() === $detectedLanguage->url()) 
                {
                    return $detectedLanguage->router()->call();
                } 
                else 
                {
                    return $kirby
                        ->response()
                        ->redirect($kirby->site()->url());
                }
            } 
            else 
            {
                return $defaultLanguage->router()->call();
            }
        }
    ];
    
    if($kirby->option('languages.detect') === true && $kirby->url() === $kirby->detectedLanguage()->url()) 
    {
        $after[] = [
            'pattern' => '(:all)',
            'method'  => 'ALL',
            'env'     => 'site',
            'action'  => function ($path = null) use ($kirby) {
                return $kirby->detectedLanguage()->router()->call($path);
            }
        ];
    }

    foreach ($kirby->languages() as $language) 
    {
        $after[] = [
            'pattern' => trim($language->pattern() . '/(:all?)', '/'),
            'method'  => 'ALL',
            'env'     => 'site',
            'action'  => function ($path = null) use ($kirby, $language) {
                return $language->router()->call($path);
            }
        ];
    }

    // fallback route for unprefixed default language URLs.
    $after[] = [
        'pattern' => '(:all)',
        'method'  => 'ALL',
        'env'     => 'site',
        'action'  => function (string $path) use ($kirby) 
    {
            // check for content representations or files
            $extension = F::extension($path);

            // try to redirect prefixed pages
            if (empty($extension) === true && $page = $kirby->page($path)) 
            {
                $url = $kirby->request()->url([
                    'query'    => null,
                    'params'   => null,
                    'fragment' => null
                ]);

                if ($url->toString() !== $page->url()) 
                {
                    return $kirby
                        ->response()
                        ->redirect($page->url());
                }
            }

            return $kirby->defaultLanguage()->router()->call($path);
        }
    ];
}

https://github.com/getkirby/kirby/blob/develop/config/routes.php#L112-L180

You will see many if else uses in the code I write for testing purposes. The reason for this is to determine the language requested. I think that it is because of this deficiency that it is difficult to solve this problem.

If we write a method that gives the requested language before we can respond to this request, we can easily resolve it by just checking for detection option.

That really even hard to explain.

Example requests

Options

  • Multi-language enabled
  • EN and DE languages defined
  • EN language is default
  • EN language url defined as http://en.getkirby.com
  • DE language url defined as http://de.getkirby.com

Request to default language: http://en.getkirby.com

Method Value
$kirby->site()->url() http://en.getkirby.com
$kirby->url() http://en.getkirby.com
$kirby->language()->url() http://en.getkirby.com
$kirby->defaultLanguage()->url() http://en.getkirby.com
$kirby->detectedLanguage()->url() http://de.getkirby.com

Request to secondary language: http://de.getkirby.com

Method Value
$kirby->site()->url() http://en.getkirby.com
$kirby->url() http://de.getkirby.com
$kirby->language()->url() http://en.getkirby.com
$kirby->defaultLanguage()->url() http://en.getkirby.com
$kirby->detectedLanguage()->url() http://de.getkirby.com

As you can see, $kirby->language()->url() continues to return the default language.
In this part of the router process, $kirby->language()->url() should give the requested language, or I think we need a new method like that requestedLanguage().

I may be wrong, but I hope I can explain what I mean.

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Oct 3, 2019

What would be the difference between a new requestedLanguage() and the existing detectedLanguage() method? That one should already return the "current" language. Because it apparently doesn't do that correctly in all cases, it probably has a bug we should fix there directly.

@afbora

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Oct 3, 2019

@lukasbestle Let me give you another example:

Default Language: EN
Requested Language: TR
Detected Language: DE (browser language)

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Oct 3, 2019

Ah, now I get it. You are right, I mixed it up.

I actually think that the $app->language() method called without params should already return the correct language. Currently it always returns the default language if the language is not already set in the $app object.

@afbora

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Oct 3, 2019

As you said, the default language always returns. If the language() method returns the current/requested language, we can easily solve this issue 👍

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Oct 21, 2019

I'm super happy to say that this will finally be fixed in 3.3.0. I'm really sorry that it took so long! The PR is still waiting for review, but I'm going to close this anyway as it will definitely be in the release.

@7pdf

This comment has been minimized.

Copy link

@7pdf 7pdf commented Oct 21, 2019

bastianallgeier added a commit that referenced this issue Oct 21, 2019
bastianallgeier added a commit that referenced this issue Oct 21, 2019
bastianallgeier added a commit that referenced this issue Oct 22, 2019
bastianallgeier added a commit that referenced this issue Oct 22, 2019
bastianallgeier added a commit that referenced this issue Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.