Throw page not found if no explicit page was found but language was provided

[qzminski]

This is rather an edge case issue but still a problem, especially if you configure the site structure in a extraordinary way. Both Contao 4.4 and 4.5 are affected.

Steps to reproduce on demo contao.org. Have three published website roots:

• DE
  • Alias: foobar-de
  • Language: de
• EN1
  • Domain name: demo.contao.org
  • Alias: en
  • Language: en
• EN2
  • Domain name: foobar.contao.org
  • Alias: en
  • Language: en

Now if you open the URL https://demo.contao.org/de/en.html Contao will find two website root points: EN1 and EN2. In the class Contao\FrontendIndex at line 96 the script will try to determine which page to use. Unfortunately both pages have the English language whereas we specified the de/ in the URL. Thus none of the statements on lines 130-146 will be executed and $objPage will remain unaltered. This will lead to the LogicException on line 162.

According to me, this is wrong because if there is no single page or multiple pages matching the provided language, then you should definitely throw the page not found exception. The ambiguous pages logic exception makes sense if there are multiple pages that actually match the language.

https://github.com/contao/core-bundle/blob/master/src/Resources/contao/controllers/FrontendIndex.php#L90

/cc @Toflar

[leofeyer]

This should be replaced with !empty($_GET['lang']).

[qzminski]

You sure? A few lines above you treat the \Input::get('language') as the source of truth for language: https://github.com/qzminski/contao-core-bundle/blob/b42099f8575999984b7838dda0a242aaa9462015/src/Resources/contao/controllers/FrontendIndex.php#L137

[leofeyer]

Your code will end in an variable $lang might not have been defined warning, which is why you had to add the isset() check. Using elseif (!empty($_GET['lang'])) will solve both.

[qzminski]

Changed in c035f92.

[leofeyer]

I just tried to reproduce this but it only worked when I did not set a language fallback. Did you happen to forget to set the language fallback?

[Toflar]

I just tried to reproduce again on demo.contao.org and it's reproducible exactly as described. And yes, fallbacks are properly enabled.

[leofeyer]

Thank you @Toflar, that helped me reproduce it. However, I think the fix is wrong. It should be:

--- a/src/Resources/contao/controllers/FrontendIndex.php
+++ b/src/Resources/contao/controllers/FrontendIndex.php
@@ -128,6 +128,12 @@ class FrontendIndex extends Frontend
 				$arrLangs = $arrPages['*'] ?: array(); // empty domain
 			}
 
+			// Throw an exception if there are no results (see #1522)
+			if (empty($arrLangs))
+			{
+				throw new PageNotFoundException('Page not found: ' . \Environment::get('uri'));
+			}
+
 			// Use the first result (see #4872)
 			if (!\Config::get('addLanguageToUrl'))
 			{

Because in this special case, there are no entries in $arrLang, so we should not execute lines 130–146. Also, the same situation might occur with prepend_locale => false, so we should not depend on $_GET['language'] IMHO.

[Toflar]

A diff or PR would greatly help me to review this 😄

[leofeyer]

I'm not getting diffs most of the time, either, so just copy the three lines and paste them. 😄

[leofeyer]

I have made you a diff anyways. 😄

[Toflar]

I'm fine with the changes if they fix the issue on demo.contao.org :)

[leofeyer]

Fixed in 3ea4cd3 then.

[qzminski]

I have to re-open this one, as your code in 3ea4cd3 doesn't fix this. See my little debugging info down there. The error comes up when I try to access the domain1.com/de/en.html URL.

// print_r($arrPages);
Array
(
    [domain1.com] => Array
        (
            [en] => Contao\PageModel Object
                (
                    [blnDetailsLoaded:protected] => 1
                    [arrData:protected] => Array
                        (
                            [id] => 55
                            [pid] => 0
                            [alias] => en
                            [type] => root
                            [language] => en
                            [dns] => domain1.com
                            // ...
                        )
                )
        )
    [domain2.com] => Array
        (
            [en] => Contao\PageModel Object
                (
                    [blnDetailsLoaded:protected] => 1
                    [arrData:protected] => Array
                        (
                            [id] => 417
                            [pid] => 0
                            [alias] => en
                            [type] => root
                            [language] => en
                            [dns] => domain2.com
                            // ...
                        )
                )
        )
)

// print_r($arrLangs);
Array
(
    [en] => Contao\PageModel Object
        (
            [arrData:protected] => Array
                (
                    [id] => 55
                    [pid] => 0
                    [alias] => en
                    [type] => root
                    [language] => en
                    [dns] => domain1.com
                    // ...
                )
        )
)

// var_dump($objPage->count());
int(2)

// var_dump($objNewPage);
NULL

As you can see there are several root pages found with the given alias, but none of them has a matching language (in this case de). I think my PR would solve the problem here because it covers the case when there are multiple pages and language is provided, yet none of those pages matches the language.

[leofeyer]

Could you give me access to the installation so I can debug this myself?

[qzminski]

@Toflar your call here.

[leofeyer]

Ok, I was able to reproduce this now. 👍

@qzminski Why did you add elseif (!empty($_GET['language']))? Shouldn't we throw the exception every time there is no page, so just else instead of elseif?

[aschempp]

Be aware that there is a behavioral difference between FrontendIndex::run and Frontend::getPageIdFromUrl. I stubled upon this while implementing #1653 , but was not aware this would probably be a bug. I think https://github.com/contao/core-bundle/blob/master/src/Resources/contao/classes/Frontend.php#L184-L188 is the correct implementation (as @leofeyer) suggested. Obviously the variables have different names 🙄 😂

[qzminski]

@leofeyer good question. I think I have added elseif as there was no else there initially and I'm not sure why. My elseif statement explicitly refers to this language issue and having the general else would also cover other cases and likely prevent the second if statement from being executed (the one where you throw \LogicException).

[leofeyer]

Fixed in contao/contao@04aa041.