Fix error pages in multilang

[distantnative]

Description

Summary of changes

• If a language has no specific URL path, its pattern doesn't match anymore (:all). Instead it only matches all that does not start with the URL path prefix of any other page.
• Because of that, a path without any matching page now renders as error page in the specific language. While before it was always considered as a path of the default language/language without any URL prefix.

Reasoning

• To me it makes sense with specific prefixes for each language to treat anything within these namespace as only belonging to that particular language. Any other language, also if not defining itself any prefix, should not interfere here.
• Error pages are correctly translated now.
• As this is only used in the catch-all route for each language (https://github.com/getkirby/kirby/blob/main/src/Cms/LanguageRoutes.php#L28-L47), the effect is scoped to only protect the languages from each other, while custom routes etc. are not affected (or only those that apply a language scope - and there I'd say protecting those from each other make sense).

Additional context

Touching those routing basics etc. is always a delicate matter. I'd be fine with moving this to 5.0, if preferred.

Changelog

Fixes

• Error pages are rendered in the current, not always the default language
  Error page always rendered in default language for some languages #4834

Ready?

• In-code documentation (wherever needed)
• Unit tests for fixed bug/feature
• Tests and CI checks all pass

For review team

• Add changes & docs to release notes draft in Notion

[distantnative]

This probably could use another unit tests, that really checks the behavior in matching full paths in the router. Any Idea where to add this unit test?

Too soon

[bastianallgeier]

@distantnative I think it's already covered in the LanguageRoutesTest::testFallback. To be sure, I added @afbora's test case from the issue. It all seems to be fine to me.

[distantnative]

@bastianallgeier I can't follow quite. You just replaced the languages with different ones in the existing test, or? But the test is the same as before. Which then would mean that it doesn't test what this PR fixes, cause otherwise it couldn't have passed the checks before this PR.

[bastianallgeier]

I thought that we had some weird edge case when en is the default language. That's why I changed the tests.

[distantnative]

My point is: if those tests passed before this PR, they aren't testing the fix this PR provides.

[bastianallgeier]

As far as I understood @afbora's comment, it worked for some reason with English before, but not with other languages. But I did not double-check if the new test case would have failed before the PR.

[distantnative]

It doesn't fail before this PR either. Will need to look into how a test that really covers the issue needs to look like.

[afbora]

I can able to test the PR tonight.

[distantnative]

I hate this, cause I cannot find a unit test. Also it's no issue if English is the default language and I don't know why because with the fix in this PR it should also apply to English beforehand. Moving this to 4.5 so we can really be sure that this is the right fix.

[afbora]

@distantnative The PR fixes the issue. But I'm not sure about that. Something smells not good. I found a clue while trying to write a unit test for this case For ex:

Following both codes doesn't give same output (current state, not for this PR). I think loading language ways are different.

Set languages from root argument as default

$app = new Kirby([
    'roots' => [
        'index'     => __DIR__,
        'languages' => __DIR__ . '/site/languages',
        'templates' => __DIR__ . '/site/templates'
    ],
    'site' => [
        'children' => [
            [
                'slug'     	   => 'error',
                'template' 	   => 'error',
                'translations' => [
                    [
                        'code'    => 'fr',
                        'content' => [
                            'title' => 'Erreur'
                        ]
                    ],
                    [
                        'code'    => 'en',
                        'content' => [
                            'title' => 'Error'
                        ]
                    ]
                ]
            ]
        ]
    ]
]);

echo $app->render('en/not-exist');

Set languages from languages argument (for ex: for unit tests)

$app = new Kirby([
    'roots' => [
        'index' => __DIR__,
        'templates' => __DIR__ . '/site/templates'
    ],
    'languages' => [
        [
            'code'     => 'fr',
            'name'    => 'French',
            'default'  => true,
            'url'         => '/'
        ],
        [
            'code'   => 'en',
            'name'  => 'English',
            'url'       => null
        ],
    ],
    'site' => [
        'children' => [
            [
                'slug'     	   => 'error',
                'template' 	   => 'error',
                'translations' => [
                    [
                        'code'    => 'fr',
                        'content' => [
                            'title' => 'Erreur'
                        ]
                    ],
                    [
                        'code'    => 'en',
                        'content' => [
                            'title' => 'Error'
                        ]
                    ]
                ]
            ]
        ]
    ]
]);

echo $app->render('en/not-exist');

[afbora]

@distantnative Here you can able to test via 20c1a1b unit test.

[distantnative]

@afbora thank you! I will have a closer look on the weekend. I agree that doesn't smell right at first at least.