Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
In Drupal 6 language_url_rewrite()
is called only for multilingual sites. The same should happen for locale_url_outbound_alter()
, otherwise we will rewrite URLs but language negotiation won't work as it is run only on multilingual sites.
Comment | File | Size | Author |
---|---|---|---|
#38 | language-835212-38.patch | 4.16 KB | plach |
#34 | 835212-34.patch | 4.53 KB | jhodgdon |
#22 | 835212-22.patch | 5.42 KB | jhodgdon |
#19 | 835212-19.patch | 5.3 KB | jhodgdon |
#18 | language-835212-18.patch | 5.67 KB | plach |
Comments
Comment #1
plachWe just need to check if the site is multilingual.
Comment #3
plachwindows newlines
Comment #4
jhodgdonThis seems quite reasonable to me. There is certainly nothing to negotiate if there is only one language, and URLs indeed shouldn't be changed.
However, does it need a test?
Comment #5
plachYes, it needed a test :)
Comment #6
plachwhitespaces...
Powered by Dreditor.
Comment #7
jhodgdonSmall thing: These two POSTs in the test could probably be combined into one form submission in the test:
Also... I'm just not quite convinced that the test is correct... Besides what was there before for a multilingual site, the added lines in the test seem to do the following for a monolingual French site:
- Go to $path_alias
- Verify that 'fr/' . $path_alias doesn't appear anywhere in the HTML of the page
What does this really tell us?
It seems like what you want to do is verify that on a monolingual French site:
(a) if you do drupalGet($path_alias), you end up on URL $path_alias and not on 'fr/' . $path_alias
(b) if you do drupalGet($path_alias), you end up on the node you expect to end up on
(c) if you do url($path_alias) or url('node/' . $nid), you do not end up with 'fr/' . $path_alias as the result
Comment #8
plachNo, because the defaut language cannot be disabled. You have to change the default language first and then disable the previous default.
Exactly. The problem here is that if you have a monolingual site with locale enabled (to translate the interface) and for some reason you enable URL language detection, URLs will be rewritten but will lead to 404 pages. The only way to test this I found is to get a page without URL prefix and check that URLs are not prefixed.
If you replicate what the test does on a fresh installation you will see what I am talking about :)
Comment #9
jhodgdonOK on point 1 - that makes sense. Maybe a comment could be added to the test code there so no one else wonders the same (wrong) thing I was wondering?
Regarding the test procedure, I still am not convinced that
is verifying that that the URL is not prefixed. assertNoRaw() asserts that the text you specified is not present in the raw HTML of the page. It doesn't assert that the URL is correct, or verify that the node is displayed on the page.
Comment #10
plachPerhaps I did not make myself understood: those lines are not checking that the URL is not prefixed, they are checking that a page reached with an unprefixed URL does not contain prefixed URLs in its anchor elements (which is the current case).
The attached patch should provide better comments.
Comment #11
jhodgdonThanks, those comments are better.
So I see that you have checked that the specific URL 'fr/' . $path_alias is not present on the page. But would a page really have a link to itself inside the HTML anyway? And you haven't checked that other 'fr/' paths are not present in other links on the page. And I still think the tests suggested (a,b,c in #7) would be good additional tests.
Thoughts?
Comment #12
plachSorry, I misunderstood your suggestions from #7: the attached patch implements them (and fix a problem with static caching to make things work).
Comment #14
plachCannot reproduce the error on my box.
#12: language-835212-12.patch queued for re-testing.
Comment #16
plachAdded a debug line in path.test.
Comment #18
plachThe URL check did not work without clean URLs.
Comment #19
jhodgdonI'm not sure why all that static stuff in the patch is so complex? Let's try this one. I also edited slightly some of the comments and assert messages.
Comment #20
plachThe previous one is the advanced
drupal_static()
pattern: aslocale_url_outbound_alter()
is called for every url, a static variable is more performant than a function call. See http://api.drupal.org/api/function/arg/7 for an example.Powered by Dreditor.
Comment #21
jhodgdonOK... But why does it need to be so obscure -- was the line saying
static $callbacks
not working?
Comment #22
jhodgdonHow about this?
Comment #23
plachNo, because in the test we need to reset the static cache otherwise the test passes also without the change in the line below:
and does not capture the current (broken) behavior.
Please restore that (ugly) advanced static pattern.
Comment #24
jhodgdonIf it's just for the test, that is not really a good reason. The test could be broken up into two separate test functions or even two separate test classes if necessary.
Comment #25
jhodgdonOK. I'm going to rewrite the test, verify that it fails without the code changes, fix the code, and get the test to pass.
Comment #26
jhodgdonSo...
I wrote a pretty careful test, and I am not seeing any evidence that (without any patching of locale_url_outbound_alter(), that is) the test is failing. It looks to me as though on a monolingual site, Drupal is not prefixing links.
Are you seeing evidence of this problem on a real site?
If so, can you please provide steps to reproduce the problem?
Comment #27
plachTry to reproduce the steps implemented in #18 (without applying any patch)...
Comment #28
jhodgdonI'm dense I guess... Can you write them out as a step-by-step procedure, starting with a clean Drupal install? For instance, does it really require all the steps -- i.e. do you have to start out with a bilingual site (French/English), make some French and English content, then change your mind and switch the site over to just French, to see the problem?
Comment #29
plachIt should be enough adding a language, disabling english and enabling URL language detection to see the problem.
Comment #30
plachI thought about this for a while: when I wrote the code introducing that static caching I thought using
drupal_static()
was an optional choice. After re-reading #254491: Standardize static caching I think we should perform the conversion from the static caching to the advanced static pattern anyway. Hence whatever solution you come up with, IMO you should re-introduce thedrupal_static()
change.Comment #31
jhodgdonOK, I see it now -- thanks.
Here's what I did to reproduce this error:
a) Added Spanish to my site.
b) Made Spanish the default.
c) Disabled English.
d) Went to the Detection and Selection page and enabled URL detection.
The URL of the page I was on immediately changed to:
(my site)/es/admin/config/regional/language
which gave me a 404 not found.
In addition, all the links I could see (toolbar, shortcuts) had the '/es/' in them, and clicking them gave me 404 pages.
In addition, if I typed in a URL to go to (my site)/admin/config for instance, that URL worked, but all the links shown on the page were wrong.
I'll add a test to follow these exact steps, now that I know what they are and what the result is.
Comment #32
jhodgdonI also see what you are saying about wanting to use drupal_static rather than the PHP static keyword, as this is the standard Drupal pattern now.
I think it can be done in a less obtuse way in this function rather than what was done in the patch in #18... and I'm not sure we should combine that change with this patch either... But anyway I'll see what I can do.
Comment #33
jhodgdonBy the way, that static variable was apparently missed in
#422364: convert locale.inc to use new static caching API
Comment #34
jhodgdonOK. Here's a new patch.
- Uses drupal_static() in place of the PHP static construction, but in a simpler way than the patch in #18.
- Adds a test that follows the simple reproduce steps in #31. The test fails without the changes to locale.module, and passes with the changes to locale.module.
Comment #35
jhodgdonComment #36
plachFirst things first: this new test looks great!
The static chaching code was written after that patch was committed. As I said, I thought using it was unneeded in this case.
I agree it's pretty ugly but I wouldn't call it obtuse: it provides a great performance gain for frequently used functions (see #619666: Make performance-critical usage of drupal_static() grokkable and http://api.drupal.org/api/function/drupal_static/7). Provided that
locale_url_outbound_alter()
is called every timeurl()
is, I thought this was a good case to adopt the advanced static pattern.Since it seems we don't agree on this point and that the change above is not needed anymore for the patch to work, I'd revert it and leave it to a dedicated issue.
It seems the above lines are not needed: I run the test with and without them, both with the change in locale.module and without, and got the same results.
Aside from those minor things above the patch looks RTBC :)
42 critical left. Go review some!
Comment #37
jhodgdonI put them in because when I ran the tests interactively and looked at the intermediate debug screens for the language settings, they weren't coming out right. But I can look again.
Comment #38
plachHere is the streamlined version of #34. I could not find anything wrong with the debug messages comparing them with the ones generated by #34.
Comment #39
jhodgdonOK. I don't know what was happening in my previous tests. Agreed that the current test is working correctly without the other lines I had put in there, and is failing on the lines we expect to fail without the patch for locale.module.
I'm ready to mark this RTBC. I assume that since plach submitted the last patch, that plach agrees. And the test bot is fine with the patch too (I've verified that the tests fail without the locale module fixes). And no one else seems to be paying any attention to this issue.
So... let's get it in!
Comment #40
plachObviously I'm ok with the RTBC status :)
Comment #41
plach#38: language-835212-38.patch queued for re-testing.
Comment #42
plachI think this meets the definition of major.
Comment #43
webchickLooks good.
For some reason the indentation was off a bit in the testPageLinks() function, but I fixed that and committed to HEAD.
Comment #44
webchickOh, and great tests!
Comment #45
plachTrue!