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

wrong 404 page shown for URLs that have no suffix #1172

Closed
fritzmg opened this issue Nov 4, 2017 · 18 comments
Closed

wrong 404 page shown for URLs that have no suffix #1172

fritzmg opened this issue Nov 4, 2017 · 18 comments
Assignees
Labels
Milestone

Comments

@fritzmg
Copy link
Contributor

fritzmg commented Nov 4, 2017

Tested with Contao 4.4.7.

Reproduction:

  1. Set the primary and secondary language of your browser to English.
  2. Leave the contao.url_suffix at its default value (.html).
  3. Set prepend_locale: true, clear the cache.
  4. Create a website root for language en.
  5. Create a 404 page with English text within that website root.
  6. Create a website root for language de.
  7. Create a 404 page with German text within that website root.
  8. Test the following URLs:

The first URL will show the German 404 page as expected.

The second URL will show the English 404 page, even though the URL clearly has the /de language fragment in it.

As indicated with the first reproduction step, the language shown for such a URL depends on the Accept-Language of the request header actually. So if your browser is set to German, you can test with the following URLs instead:

The first URL will show the English 404 page, the second one will show the German 404 page.

@leofeyer
Copy link
Member

leofeyer commented Nov 7, 2017

@contao/developers Do you think we should handle this case? URLs without the URL suffix are not valid Contao URLs and therefore Contao should not try to extract anything from them IMO.

@ausi
Copy link
Member

ausi commented Nov 8, 2017

If it’s an easy fix, I think we should do it.

@Toflar
Copy link
Member

Toflar commented Nov 8, 2017

URLs without the URL suffix are not valid Contao URLs and therefore Contao should not try to extract anything from them IMO.

I agree, it's not a Contao URL so why does it redirect to the English 404 page then? :D

@fritzmg
Copy link
Contributor Author

fritzmg commented Nov 8, 2017

It does not redirect, it simply generates the 404 page of the primary Accept-Language (or fallback).

@bytehead
Copy link
Member

bytehead commented Nov 8, 2017

URLs without the URL suffix are not valid Contao URLs

What if I configured an empty URL suffix?

@fritzmg
Copy link
Contributor Author

fritzmg commented Nov 8, 2017

What if I configured an empty URL suffix?

Then such URLs would show the regular 404 page and URLs with an URL suffix possibly will not.

@bytehead
Copy link
Member

bytehead commented Nov 8, 2017

Contao should only handle this kind of cases, when a route with a contao_ prefix matches.

@fritzmg
Copy link
Contributor Author

fritzmg commented Nov 8, 2017

Contao should only handle this kind of cases, when a route with a contao_ prefix matches.

Which is true for any URL ;) routing.yml#L25-L33

# The catch all route must be the last one!
contao_catch_all:
    path: /{_url_fragment}
    defaults:
        _scope: frontend
        _token_check: true
        _controller: "ContaoCoreBundle:Frontend:index"
    requirements:
        _url_fragment: .*

@bytehead
Copy link
Member

bytehead commented Nov 8, 2017

contao_catch_all should be the very last rule of the routing config.

@fritzmg
Copy link
Contributor Author

fritzmg commented Nov 8, 2017

Yes of course, but that's not the point. Any URL (that is not physically present or catched otherwise) is a valid Contao URL currently.

@bytehead
Copy link
Member

bytehead commented Nov 8, 2017

Any URL is a valid Contao URL currently.

Thats a problem, if somebody has a Symfony application and adds the contao\core-bundle later.
Contao should only care about Contao URLs and not about others.

@fritzmg
Copy link
Contributor Author

fritzmg commented Nov 8, 2017

Yes, but Contao URLs are currently only processed via the contao_root and contao_catch_all route.

If you use the core-bundle in your Symfony app, you yourself need to make sure that its routes are the last ones loaded.

@Toflar
Copy link
Member

Toflar commented Jan 29, 2018

This is indeed an issue, we have to do something about it. It was completely confusing for me and I debugged quite a bit and didn't even remember that issue here :) So either the issue is that it does not find the correct 404 page to render or the issue is that it tries to render one even though it should not be considered a Contao request.
We can handle this case properly once we switch to proper router handling but until then, we have to make a decision here.

@leofeyer
Copy link
Member

As discussed in Mumble on February 15th, a first step to solving the problem would be to move this block

// Extract the language
if (\Config::get('addLanguageToUrl'))
{
$arrMatches = array();
// Use the matches instead of substr() (thanks to Mario Müller)
if (preg_match('@^([a-z]{2}(-[A-Z]{2})?)/(.*)$@', $strRequest, $arrMatches))
{
\Input::setGet('language', $arrMatches[1]);
// Trigger the root page if only the language was given
if ($arrMatches[3] == '')
{
return null;
}
$strRequest = $arrMatches[3];
}
else
{
return false; // Language not provided
}
}

before this block

// Remove the URL suffix if not just a language root (e.g. en/) is requested
if ($strRequest != '' && (!\Config::get('addLanguageToUrl') || !preg_match('@^[a-z]{2}(-[A-Z]{2})?/$@', $strRequest)))
{
$intSuffixLength = \strlen(\Config::get('urlSuffix'));
// Return false if the URL suffix does not match (see #2864)
if ($intSuffixLength > 0)
{
if (substr($strRequest, -$intSuffixLength) != \Config::get('urlSuffix'))
{
return false;
}
$strRequest = substr($strRequest, 0, -$intSuffixLength);
}
}

@leofeyer leofeyer added this to the 4.4.15 milestone Feb 15, 2018
@leofeyer leofeyer self-assigned this Feb 20, 2018
@leofeyer
Copy link
Member

Fixed in fae22c6.

@fritzmg
Copy link
Contributor Author

fritzmg commented Apr 24, 2018

This still happens in Contao 4.4.18.

@fritzmg
Copy link
Contributor Author

fritzmg commented Oct 25, 2018

Still happening in Contao 4.4.26 & 4.6.6

@leofeyer leofeyer reopened this Oct 29, 2018
@leofeyer leofeyer modified the milestones: 4.4.15, 4.4.27 Oct 29, 2018
@leofeyer
Copy link
Member

Apparently, the change from fae22c6 is no longer there. It is actually not in the history, either. 🤔

Re-added in contao/contao@65b5d55.

@leofeyer leofeyer modified the milestones: 4.4.27, 4.4 May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants