URL language detection with empty path prefix not working

Yes this will require tests

Also issue summary update for proposed solution.

Well it looks like this even got worse or I never noticed it that bad before! That's why I'm raising priority and I'm going to work on this, especially tests, together with @Grevil in the next days.

I just found out that language detection behaves inconsistently based on the page you're on. While the frontpage for example shows all menu links in the current, correct language, other path's show wrong URLs losing the current language from URL.
This only happens when using a setup with an empty language path prefix for one language.

I was able to reproduce this quite simple on a vanilla Drupal installation.

As written above, I think it's still a regression or sth. left over from #2411343: Only allowing to have empty language path prefix for the default language is inappropriate →
Being able to reproduce this shows that existing tests are not sufficient.

Update: Testing the MR and clearing caches for that, I saw that now after clearing caches the situation changed, so this might alternatively or additionally be a caching issue in menus.

We'll try to find out more next week! Does anyone else here have further information already? Are you still using the patch from DieterHolvoet's MR actively?

Alright, I added a test and did a quick phpcs fix-up!

I originally intended to also provide a "testEmptyLanguagePrefixForNonDefaultLanguage()" test, but you can't even leave the non default/fallback language empty:

The prefix may only be left blank for the selected detection fallback language.

Unfortunately, I don't have the permission to change the MR target to Drupal 11, nor reset this fork to current drupal-dev.

Locally the test will fail with the following errors WITHOUT the patch:

Current response header "X-Drupal-Cache" is "", but "MISS" expected.

At line 146.
Temporarily changing line 146 to $this->assertSession()->responseHeaderDoesNotExist('X-Drupal-Cache'); (to let the test throw the error message we are expecting) will make the test fail later on with the expected error message:

Current response header "Content-Language" is "fr", but "en" expected.

With the patch applied, the test becomes green! 🙂

Locally tested using:

• Drupal 10.2.6
• PHP 8.3.6
• PHPUnit 9.6.19

RTBC!

Great work @Grevil, RTBC+1 as I can confirm this fixes the issue. Test shows that.

This should please also be committed to 10.3.x to fix it there.

Happy to see this fixed finally. It would be great to also have confirmation by the other participating users here, especially @DieterHolvoet. Thanks!

Updated issue summary.

Test coverage looks great, nice job.

Careful when testing with $this->rootUser, as it may no longer be an admin. See 📌 [Meta] Fix all tests that rely on UID1's super user behavior Active for a meta issue where we're removing all use of that in child issues. Setting to NW for that.

@kristiaanvandeneynde, thanks for the info! Didn't know that. :)

Adjusted the tests accordingly!

Looks better. Might get a merge conflict with the effort in core, though. So setting back to RTBC, but ideally this needs to be targeted vs 11.x and have the latest changes from 11.x merged in (or be rebased onto it)

Agreed. I'll give it a shot.

Ok, no chance on properly rebasing or merging the branch on 11.x, I dirtily "rebased" it through commits, somebody should target the MR on 11.x, and then we can see if the changes are still there 😅 (They should in theory).

Hopefully we won't get any merge conflicts 😬.

Perhaps ask in #contribute on Slack for some help

Accidentally added changes twice after "rebase".

Alright, that should do the trick! Back to RTBC!

Spellchecking failed: https://git.drupalcode.org/issue/drupal-2953640/-/pipelines/174980/failures anyone got an idea? The error looks quite weird.

@Grevil something went wrong with your MR. First of all it broke our builds as latest version does not apply to latest D10. And secondly, if you try to access patch file via url https://git.drupalcode.org/project/drupal/-/merge_requests/2807.patch you'll see its file size is 105MB:

$ wget https://git.drupalcode.org/project/drupal/-/merge_requests/2807.patch -O /tmp/2807.patch
$ ls -lash /tmp/2807.patch 
105M -rw-rw-r-- 1 hideaway hideaway 105M May 20 11:21 /tmp/2807.patch

Yea, thought so. I rebased the branch through applying the diff between 9.5.x and current 11.x, as I had a thousand rebase / merge conflicts, when rebasing / merging normally.

Just going to reverse it for now and let somebody else with more knowledge of rebasing branches do the honor.

Branch is back to the original 9.5.x state, when this issues fork was created (https://git.drupalcode.org/project/drupal/-/commit/56683561dc7499326d3cf...)

Alright, branch is back on track with 11.x + issue related changes.

@hideaway

First of all it broke our builds as latest version does not apply to latest D10

I am sorry for that, but stuff like this can always happen. I generally wouldn't recommend to use branch diffs as patches for composer (because of their dynamic nature). Instead, always make sure, that a static patch is provided!

Yeah right. Let's please provide a static patch copy. And we'll create one before such heavy rebases in the future.

Hopefully we'll get this fixed soon!

#51 is correct, it's also risky in terms of security. Others could inject risky code into the dynamic patch files.

Static patch against 11.x attached!

Tests are failing (also some strange ones)