Currently URLs are allowed to be rewritten with disabled languages, OTOH language negotiation (correctly) recognizes only enabled languages so the rewritten URLs won't work.

CommentFileSizeAuthor
#34 language-926212-34.patch5 KBplach
#32 language-926212-32.patch5.13 KBplach
#29 language-926212-29-test.patch3.47 KBplach
#29 language-926212-29.patch4.76 KBplach
#28 language-926212-28-test.patch3.37 KBbforchhammer
#25 language-926212-25-test.patch3.46 KBbforchhammer
#23 language-926212-23.patch4.66 KBplach
#23 language-926212-23-test.patch3.46 KBplach
#14 language-926212-14-test.patch5.01 KBplach
#14 language-926212-14.patch6.31 KBplach
#12 language.926212-10.png11.91 KBbforchhammer
#12 language.926212-10-test.png31.03 KBbforchhammer
#11 language-926212-11.png9.73 KBplach
#11 language-926212-11-2.png21.22 KBplach
#10 language-926212-10.patch4.73 KBplach
#10 language-926212-10-test.patch3.44 KBplach
#5 language-926212-5.patch3.98 KBplach
#5 language-926212-5-test.patch2.68 KBplach
#1 language-926212-1.patch1.29 KBplach

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Active » Needs review
StatusFileSize
new language-926212-1.patch1.29 KB

Here is a patch for the bot. Needs tests.

good_man’s picture

good_man commented

I can only see (without your patch) this function getting the current active language, not all languages. OTOH, URL rewrite displays only active languages.

bforchhammer’s picture

bforchhammer commented

I tested the patch in #1 in conjunction with the patch in #284625 and can confirm that it works.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

This comes from #284625: Non-localized links in translation table....

Steps to reproduce:

  1. add a couple of additional languages, say german and french
  2. disable french
  3. enable URL language detection
  4. create a node in english
  5. try to translate it in french

Edit: You'll see the issue after applying the latest patch in the issue above.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
StatusFileSize
new language-926212-5-test.patch2.68 KB
new language-926212-5.patch3.98 KB

And here is the test.

The test-only version of the patch is supposed to fail testing to show that the complete patch fixes the issue.

bforchhammer’s picture

bforchhammer commented

The test-only version of the patch is supposed to fail testing to show that the complete patch fixes the issue.

Hm, so why is the test-only version NOT failing the test-bot then? ;-)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

Strange, on my box it fails. I'll give it a look. But I suppose the problem is it's difficult to configure a locale-proof test environment.

Can you test the two patches on your machine? You just need to run the UI Language negotiation test.

bforchhammer’s picture

bforchhammer commented

Can you test the two patches on your machine? You just need to run the UI Language negotiation test.

I'll give it a go later today.

bforchhammer’s picture

bforchhammer commented
Status: Needs review » Needs work

I just tried the test-only patch, and it also passes... looks like this needs more work.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs work » Needs review
StatusFileSize
new language-926212-10-test.patch3.44 KB
new language-926212-10.patch4.73 KB

Tests were missing a static cache reset. I don't know how could they fail on my box yesterday...

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
StatusFileSize
new language-926212-11-2.png21.22 KB
new language-926212-11.png9.73 KB

:(

@bforchhammer, what about you?

bforchhammer’s picture

bforchhammer commented
Status: Needs review » Needs work
StatusFileSize
new language.926212-10-test.png31.03 KB
new language.926212-10.png11.91 KB

Seems like the results are the same on my test machine...

Why are they different on the test-bots? Is that something that's happened before? Is it a caching issue? I guess that's something that we should probably figure out before this becomes rtbc...?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

Sure, I'm trying a different approach.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs work » Needs review
StatusFileSize
new language-926212-14.patch6.31 KB
new language-926212-14-test.patch5.01 KB

Let's try this.

bforchhammer’s picture

bforchhammer commented

Works as expected. I like the changes, the test-case seems more clear now.

Let's see what the bot says...

bforchhammer’s picture

bforchhammer commented
Status: Needs review » Needs work

Still the same...

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs work » Needs review

Oh, fuck :(

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs review » Needs work

I'll try and see if it's a clean ulr issue, but I doubt...

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs work » Needs review

#14: language-926212-14-test.patch queued for re-testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

#14: language-926212-14.patch queued for re-testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

I can't reproduce the bot's behavior on my box(es). I filed an issue in the infrastructure queue: #931020: Test passing when it should fail.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

#14: language-926212-14-test.patch queued for re-testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
StatusFileSize
new language-926212-23-test.patch3.46 KB
new language-926212-23.patch4.66 KB

Rerolled after the latest commits.

Status: Needs review » Needs work

The last submitted patch, language-926212-23-test.patch, failed testing.

bforchhammer’s picture

bforchhammer commented
StatusFileSize
new language-926212-25-test.patch3.46 KB

Wrong line endings...

bforchhammer’s picture

bforchhammer commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, language-926212-25-test.patch, failed testing.

bforchhammer’s picture

bforchhammer commented
Status: Needs work » Needs review
StatusFileSize
new language-926212-28-test.patch3.37 KB

Hm... and this time fixed for real.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
StatusFileSize
new language-926212-29.patch4.76 KB
new language-926212-29-test.patch3.47 KB

@DamZ helped me fix the test. Now we should be ok.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

yay!

bforchhammer’s picture

bforchhammer commented
Status: Needs review » Reviewed & tested by the community

Finally :)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
StatusFileSize
new language-926212-32.patch5.13 KB

Improved PHPdocs

Status: Reviewed & tested by the community » Needs work

The last submitted patch, language-926212-32.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs work » Needs review
StatusFileSize
new language-926212-34.patch5 KB

windows newlines...

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs review » Reviewed & tested by the community
tom_o_t’s picture

tom_o_t commented

#34: language-926212-34.patch queued for re-testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.