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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Status: Active » Needs review
Issue tags: +Quick fix
FileSize
678 bytes

We just need to check if the site is multilingual.

Status: Needs review » Needs work

The last submitted patch, language-835212-1.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
662 bytes

windows newlines

jhodgdon’s picture

This 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?

plach’s picture

FileSize
4.36 KB

Yes, it needed a test :)

plach’s picture

FileSize
4.36 KB
+++ modules/path/path.test	29 Jun 2010 23:37:54 -0000
@@ -286,8 +285,10 @@ class PathLanguageTestCase extends Drupa
+    ¶

whitespaces...

Powered by Dreditor.

jhodgdon’s picture

Status: Needs review » Needs work

Small thing: These two POSTs in the test could probably be combined into one form submission in the test:

+    $edit = array('site_default' => 'fr');
+    $this->drupalPost('admin/config/regional/language', $edit, t('Save configuration'));
+    $edit = array('enabled[en]' => FALSE);
+    $this->drupalPost('admin/config/regional/language', $edit, t('Save configuration'));

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

plach’s picture

These two POSTs in the test could probably be combined into one form submission in the test:

No, because the defaut language cannot be disabled. You have to change the default language first and then disable the previous default.

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

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 :)

jhodgdon’s picture

OK 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

+    $this->drupalGet($path_alias);
+    $this->assertNoRaw("fr/$path_alias", t('Path alias prefix is disabled when a single language is enabled.'));

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.

plach’s picture

Status: Needs work » Needs review
FileSize
4.61 KB

Regarding the test procedure, I still am not convinced that [...] is verifying that that the URL is not prefixed

Perhaps 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.

jhodgdon’s picture

Thanks, 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?

plach’s picture

FileSize
5.59 KB

Sorry, I misunderstood your suggestions from #7: the attached patch implements them (and fix a problem with static caching to make things work).

Status: Needs review » Needs work
Issue tags: -Quick fix

The last submitted patch, language-835212-12.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

Cannot reproduce the error on my box.

#12: language-835212-12.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Quick fix

The last submitted patch, language-835212-12.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
5.7 KB

Added a debug line in path.test.

Status: Needs review » Needs work

The last submitted patch, language-835212-16.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
5.67 KB

The URL check did not work without clean URLs.

jhodgdon’s picture

FileSize
5.3 KB

I'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.

plach’s picture

Status: Needs review » Needs work
+++ modules/locale/locale.module	2 Jul 2010 16:19:15 -0000
@@ -961,8 +961,8 @@
+    $callbacks = &drupal_static(__FUNCTION__);

The previous one is the advanced drupal_static() pattern: as locale_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.

jhodgdon’s picture

OK... But why does it need to be so obscure -- was the line saying
static $callbacks
not working?

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
5.42 KB

How about this?

plach’s picture

Status: Needs review » Needs work

OK... But why does it need to be so obscure -- was the line saying
static $callbacks
not working?

No, because in the test we need to reset the static cache otherwise the test passes also without the change in the line below:

+  if (!$options['external'] && drupal_multilingual()) {

and does not capture the current (broken) behavior.

Please restore that (ugly) advanced static pattern.

jhodgdon’s picture

If 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.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

OK. I'm going to rewrite the test, verify that it fails without the code changes, fix the code, and get the test to pass.

jhodgdon’s picture

Status: Needs work » Postponed (maintainer needs more info)

So...

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?

plach’s picture

Try to reproduce the steps implemented in #18 (without applying any patch)...

jhodgdon’s picture

I'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?

plach’s picture

It should be enough adding a language, disabling english and enabling URL language detection to see the problem.

plach’s picture

If it's just for the test, that is not really a good reason.

I 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 the drupal_static() change.

jhodgdon’s picture

Status: Postponed (maintainer needs more info) » Active

OK, 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.

jhodgdon’s picture

I 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.

jhodgdon’s picture

By the way, that static variable was apparently missed in
#422364: convert locale.inc to use new static caching API

jhodgdon’s picture

Status: Active » Needs review
FileSize
4.53 KB

OK. 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.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
plach’s picture

First things first: this new test looks great!

By the way, that static variable was apparently missed in #422364: convert locale.inc to use new static caching API

The static chaching code was written after that patch was committed. As I said, I thought using it was unneeded in this case.

I think it can be done in a less obtuse way in this function rather than what was done in the patch in #18...

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 time url() is, I thought this was a good case to adopt the advanced static pattern.

+++ modules/locale/locale.module	7 Jul 2010 23:54:32 -0000
@@ -961,9 +961,8 @@
+    $callbacks = &drupal_static(__FUNCTION__);

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.

+++ modules/path/path.test	7 Jul 2010 23:54:32 -0000
@@ -388,3 +388,74 @@
+    cache_clear_all('variables', 'cache_bootstrap');
+    variable_initialize();
+    drupal_static_reset('language_list');
+++ modules/path/path.test	7 Jul 2010 23:54:32 -0000
@@ -388,3 +388,74 @@
+    cache_clear_all('variables', 'cache_bootstrap');
+    variable_initialize();
+    drupal_static_reset('language_list');
+++ modules/path/path.test	7 Jul 2010 23:54:32 -0000
@@ -388,3 +388,74 @@
+    cache_clear_all('variables', 'cache_bootstrap');
+    variable_initialize();
+    drupal_static_reset('language_list');
+
+    // Force languages to be initialized.
+    drupal_language_initialize();

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!

jhodgdon’s picture

I 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.

plach’s picture

FileSize
4.16 KB

Here is the streamlined version of #34. I could not find anything wrong with the debug messages comparing them with the ones generated by #34.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

OK. 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!

plach’s picture

Obviously I'm ok with the RTBC status :)

plach’s picture

#38: language-835212-38.patch queued for re-testing.

plach’s picture

Priority: Normal » Major

I think this meets the definition of major.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks good.

For some reason the indentation was off a bit in the testPageLinks() function, but I fixed that and committed to HEAD.

webchick’s picture

Oh, and great tests!

plach’s picture

True!

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix

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