404 Page Not Found missing correct localization on multilingual sites

[jlucki]

404 pages have a few issues on multilingual sites, some seem like bugs, others might fall under the purview of enhancements/nice to haves, but I'll post them all here anyway since they all kinda fall under the same umbrella.

I've set this up on the the development branch but these have been present since at least 8.2.1.

Steps to reproduce:

Note: I've included my local urls, but change those where appropriate.

1. Install concrete5, I've set it up with the empty theme but this doesn't matter.

2. Navigate to http://localhost:8888/index.php/dashboard/system/multilingual/setup

3. Add locale
   Details: I added French, template Full, page name Home, slug fr
   3a) I've also enabled toolbar titles for a bit more clarity

4. Add <?php echo \Concrete\Core\Localization\Localization::activeLocale(); ?> to concrete/themes/elemental/page_not_found.php

5. Navigate to http://localhost:8888/index.php/404
   This results in a standard 404 page, with en_US shown on the page as the locale, so this expected behaviour

6. Now try it with the french locale slug http://localhost:8888/index.php/fr/404
   This is where things get confusing. This still loads the standard 404 page, but the localization is lost, we are still seeing en_US as the locale, even though we're on the french localization of the site

   Additionally, concrete5 seems to not know what language to display here, as we're seeing two related pages language buttons in the toolbar

   

7. Now, go back to http://localhost:8888/index.php/dashboard/system/multilingual/setup and change the default locale to French

8. Go back to http://localhost:8888/index.php/fr/404

   I would expect the locale to now show as fr_FR, but it still shows as en_US

This all makes localization of 404 pages very difficult.

Halfway solution:

This isn't really a solution, but it worked for my use case. Hopefully it at least points to a resolution.

http://localhost:8888/index.php/dashboard/system/multilingual/setup change the default locale back to english

The issues are all, as far as I can tell, in: https://github.com/concrete5/concrete5/blob/develop/concrete/src/Http/ResponseFactory.php

Line 92, I've replaced

$item = '/page_not_found';

with

$home = \Page::getByID(\Page::getCurrentPage()->getSiteHomePageID());
$slug = $home->getCollectionHandle();
$item = '/' . $slug . '/page_not_found';

and line 237, I've replaced

if ($collection->getCollectionPath() != '/page_not_found') {

with

if (strpos($collection->getCollectionPath(), '/page_not_found') == false) {

I also had to copy the Page Not Found system page across to all locales using the sitemap in the dashboard.

Now visit http://localhost:8888/index.php/fr/404

This leads to something slightly more in line with expected behaviour. This doesn't fix the duplicate related pages buttons, but they both appear as the correct language now, and the localization locale appears correct now too.

It seems to me that the locale isn't actually ever set on system pages, regardless of the settings. System pages will just always be whatever language the site was installed with.

[aembler]

This is related to this: #3350 but I think there's value keeping both of these open.

Your solution sounds like a good one but I'm not sure it's one we want to take wholesale into the core – we should have a nicer solution than this.

This is also somewhat related to #6447

[mlocati]

Currently, pages that are not associated to a language (for example: login, page not found, forbidden, ...) use the last known language.
For instance, the login page is in French if the last visited page was in French, and in English if the last visited page was in English.
Maybe for the page not found page we could have an exception, because you may arrive to the non existing page coming from an external link,

[iampedropiedade]

I reckon it would make sense to have this order of priority for the page_not_found language:

1. if user lands on /X/non/existing/page and X is a language set up on dashboard, then use X
2. if not 1), then use last known language
3. if not 1) and 2), then use default language defined on dashboard

[jlucki]

@mlocati That does indeed work for the login page, locale is set to fr_FR if the last visited page was French, but it doesn't work for page not found. If I set up two languages as above, and visit /fr, and then visit /fr/404, the 404 page should appear with the French localization, but this still always shows en_US.

[mlocati]

@jlucki This is strictly related to #6447 (and I don't know why @aembler closed that issue).
The PageNotFound page is associated to the default language, the Login page is not.

Indeed, this SQL query

select
	PagePaths.cPath,
	Pages.siteTreeID
from
	PagePaths
	inner join Pages on PagePaths.cID = Pages.cID
where
	PagePaths.cPath in ('/login', '/page_not_found')

results in

cPath	siteTreeID
/login	0
/page_not_found	1

That means that there's no language associated to the Login page (so, concrete5 will use the last one, falling back to the default one).
On the contrary, the PageNotFound page is explicitly associated to the language identified by the id 1.

I can't solve this issue with the current database structure. As I wrote in #6447, I think we'd need a new column in the Pages table (to associate a page to a site, but not to a specific language in that site).

[jlucki]

@mlocati @aembler Is there a reason page_not_found is associated with siteTreeID 1? I've disassociated this on my local (changed it to 0), and the 404 page now behaves as expected.

http://localhost:8888/index.php/fr/404 results in:

It's even removed the related pages buttons which really have no reason to be visible on a 404 page.

[jlucki]

https://github.com/concrete5/concrete5/compare/develop...jlucki:fix-6500?expand=1

The above fixes my 404s issues. I've made page_not_found match page_forbidden settings. Does that look ok? I can open a PR if so.

[aembler]

I believe this is a bug – with the idea that we might want to have a page not found page that honors different themes in a multi-site approach. But obviously this doesn't really work when we keep it in a single site tree. I think your approach is probably the right one. I'm going to assign this to the 8.4.0 milestone and we will get this updated in there.