Redirection not working on multi-lang site with prefixes [#1735762]

With a D7.15 multi-lang site with prefixes, I had to make the following changes to get it to work,
first _r4032login_remove_language need fixing:

function _r4032login_remove_language($destination) {
  global $language;
  #$prefix = arg(0, $destination);
  $languages = language_list();    // get list of valid languages on this site

  while (list(, $lang) = each($languages)) {
    if (strpos($destination, $lang->language .'/') == 1 ) {  // is language prefixed?
      $destination = substr($destination, strlen($lang->language) + 1); // return string after "/prefix"
      watchdog(__FUNCTION__, "remove " . $lang->language . " rewrite destination=" . $destination);
      return $destination;
    }
    #if (strcasecmp($prefix, $lang->language) == 0) {  // TODO: who care if the current and destn lang are the same?
    #  watchdog(__FUNCTION__, "remove prefix, destination=" . substr($destination, strlen($prefix) + 1));
    #  return substr($destination, strlen($prefix) + 1); // remove prefix and slash: return string after "/prefix"
    #}
  }
  return $destination;
}

Then in r4032login_redirect() a leading slash had to be stripped:

function r4032login_redirect() {
  global $user, $language;
  if (user_is_anonymous()) {
    if (variable_get('r4032login_display_denied_message', TRUE)) {
      $message = variable_get('r4032login_access_denied_message', t('Access denied. You must login to view this page.'));
      drupal_set_message(t($message), 'error');
    }
    $path = _r4032login_remove_language(request_uri());
    $path = ltrim($path, '/'); // SB remove leading slash, if any

Patch attached.

Comment	File	Size	Author
#16	nid_1735762_login_multi_lang.patch	774 bytes	xiaoluan0318
#15	r4032login-redirection_not_working_on_multi-lang_site_with_prefixes-1735762-15.patch	2.19 KB	Sk8erPeter
#9	r4032login-multilang_fix-1735762-9.patch	1.22 KB	firebird
#5	r4032login_multilang_1735762_5.patch	1.33 KB	boran
	r4032login_multilang.patch	971 bytes	boran

Comments

Comment #1

facal commented 9 September 2012 at 21:16

Status:

Active

» Reviewed & tested by the community

Thanks boran, works like a charm.
In fact, applying your patch is a mandatory step for anyone willing to use this module.

Please commit it soon!

Comment #2

deekayen commented 10 September 2012 at 15:01

It's easier to review and commit patches when they follow the guidelines.

http://drupal.org/patch/submit

Comment #3

boran commented 10 September 2012 at 21:31

Whats your point precisely?
I provided a verbose explanation of the changes above, and a (less verbose) git patch as an attachment.

Comment #4

deekayen commented 10 September 2012 at 21:32

Sorry, that's not a git patch. Will post a git patch when I re-make your patch so you can see.

Comment #5

boran commented 10 September 2012 at 21:52

Status	File	Size
new	r4032login_multilang_1735762_5.patch	1.33 KB

Oops, see attached. Also removed the watchdogs and 'sb' from comments

Comment #6

deekayen commented 5 October 2012 at 19:48

Status:

Reviewed & tested by the community

» Needs work

deekayen-macbook Sites/r4032login ‹7.x-1.x› » git apply ~/tmp/r4032login_multilang_1735762_5.patch.txt
error: patch failed: r4032login.module:64
error: r4032login.module: patch does not apply

Comment #7

deekayen commented 12 October 2012 at 15:58

Version:

7.x-1.5

» 7.x-1.x-dev

I looked to see if I could manually apply the changes. Whatever you diffed against doesn't exist anymore in the redirect function.

Comment #8

Sk8erPeter commented 13 October 2012 at 22:55

Priority:

Normal

» Critical

I posted my problem here for the first time: http://drupal.org/node/1741698#comment-6585498
But maybe it's really rather related to this topic.

I upgraded Redirect 403 to User Login to 7.x-1.5+13-dev, and the redirection is STILL wrong when Drupal is in a subdirectory - and the correct redirection is one of the main tasks of this module, so I'm a bit surprised it's still not fixed yet (that's why I set the priority to "critical").

The bug in details:

I have Drupal in a subdirectory called "drupal". The site is multilingual.
I type the following:
http://xyz.local/drupal/en/admin
I'm not logged in, so I get the message "Access denied. You must login to view this page.", and I get the login form, which is OK.
BUT the URL looks like this:
http://xyz.local/drupal/en/user/login?destination=/drupal/en/admin
The destination equals to /drupal/en/admin, which is NOT correct, because it should only be destination=admin (I just tried it, and this WOULD work correctly).
After that, the module redirects me to
http://xyz.local/drupal/en//drupal/en/admin
!! So drupal/en/ is doubled.
Of course, I get a not found page:
"The requested page "/drupal/en/drupal/en/admin" could not be found."

Comment #9

firebird commented 19 November 2012 at 09:23

Status	File	Size
new	r4032login-multilang_fix-1735762-9.patch	1.22 KB

This seems to have been mostly fixed in the latest dev version. However, it checks for a Drupal variable called "language_negotiation", which on my multilanguage site isn't set. As a result, the language prefix isn't removed, and the user is redirected to an invalid URL after login.

This patch simply removes the if-clause from around the process that removes the language prefix. This can cause unexpected behaviour on sites that don't have a multilanguage setup, but that do have language prefixes in the URLs.

Comment #10

firebird commented 19 November 2012 at 09:23

Status:

Needs work

» Needs review

Comment #11

lolandese commented 21 November 2012 at 20:03

Status:

Needs review

» Reviewed & tested by the community

Patch applies smoothly and fixes the issue. Tested with several language prefixes.
Thanks.

"This can cause unexpected behaviour on sites that don't have a multilanguage setup, but that do have language prefixes in the URLs."
Good to mention, but I think it would be a rare case.

Comment #12

deekayen commented 21 November 2012 at 20:37

Status:

Reviewed & tested by the community

» Fixed

Committed #9.

Comment #13

Sk8erPeter commented 23 November 2012 at 12:20

Status:

Fixed

» Needs review

I don't understand: why do you have to mess with such ugly explode(), implode() and other functions for getting the right language prefixes, prepend them, forcing it to appear in destination query string, when there are nice built-in Drupal functions like drupal_get_destination()?

For example, in r4032login_redirect(), in the if (user_is_anonymous()) condition block, I simply changed the following line:

    header('Location: ' . url($login_path, array('query' => array('destination' => _r4032login_destination()), 'absolute' => TRUE)), TRUE, 302);

to this one:

    header('Location: ' . url($login_path, array('query' => drupal_get_destination(), 'absolute' => TRUE)), TRUE, 302);

and it currently seems to work perfectly. So I completely left out the usage of _r4032login_destination(), because it seems like it's unnecessary. Or can you mention any cases when it's really needed? I would like to try that, too.

For example, when I try to open the following URL:
http://example.com/drupal/hu/node/2/edit/
as an anonymous user, it redirects me to this address:
http://example.com/drupal/hu/user/login?destination=node/73/edit
and after logging in, the redirection works perfectly. So forcing the language prefix in the destination query string seems like totally unnecessary, because it's prepended in front of user/login!

I've also tried that with Clean URLs disabled
In this case, when I try to open
http://example.com/drupal/?q=hu/node/87/edit
my previous code redirects the user to
http://example.com/drupal/?q=hu/user/login&destination=node/87/edit
and it also works correctly.

Please tell me if there is any case in which using drupal_get_destination() doesn't work the way I showed.

Comment #14

nikosnikos commented 13 December 2012 at 15:19

The simple solution in #13 works for me.

Comment #15

Sk8erPeter commented 14 December 2012 at 02:14

Status	File	Size
new	r4032login-redirection_not_working_on_multi-lang_site_with_prefixes-1735762-15.patch	2.19 KB

Thanks for your feedback, nikosnikos!

I attached a simple patch which removes _r4032login_destination() (which seems unnecessary and works badly, see my post here!) and uses the core drupal_get_destination() instead, please test it!
Thanks!

Comment #16

xiaoluan0318 commented 3 January 2013 at 09:38

Version:

7.x-1.x-dev

» 6.x-1.4

Status	File	Size
new	nid_1735762_login_multi_lang.patch	774 bytes

I had a patch for version 6.x-1.4
Fix a bug in function _r4032login_remove_language()

Comment #17

Sk8erPeter commented 3 January 2013 at 10:53

Version:

6.x-1.4

» 7.x-1.x-dev

This topic should remain related to 7.x-1.x-dev, until the corrections are done. Btw., instead of trying to fix the ugly code, you should also use drupal_get_destination() in r4032login_redirect() function's first header() call, because it works, this is why I attached the patch in #15. The original code does NOT work correctly when having Drupal in a subdirectory with language prefixes used.
Switching back version.

Comment #18

giorgio79 commented 2 May 2013 at 14:04

Sk8erPeter your patch is fantastic! Perhaps you could apply for comaintainership looking at the pace of this issue? :)

Comment #19

heyyo commented 22 August 2013 at 05:42

Works great for me also.

Comment #20

devin carlson commented 30 August 2013 at 16:36

Status:

Needs review

» Reviewed & tested by the community

Tested #15 with four fresh D7 installs: a site with Drupal in the server root with a single language, the same configuration with multiple languages enabled (using path prefixes) and a site with Drupal in a subdirectory with a single language and the same configuration with multiple languages enabled (using path prefixes).

Users were redirected properly in all environments and I didn't run into side effects.

Comment #21

deekayen commented 30 August 2013 at 17:01

Version:	7.x-1.x-dev	» 6.x-1.x-dev
Status:	Reviewed & tested by the community	» Patch (to be ported)

committed to 7.x-1.x

Comment #22

Sk8erPeter commented 30 August 2013 at 17:10

Version:	6.x-1.x-dev	» 7.x-1.x-dev
Status:	Patch (to be ported)	» Reviewed & tested by the community

Thanks for the tests, giorgio79, heyyo and Devin Carlson! :)

(By the way, in case someone is interested, LoginToboggan is a good alternative, it provides "a login form on Access Denied pages for non-logged-in (anonymous) users" (without redirection, which may be a better approach), and some other features related to logging in or registration. I tried 7.x-1.x-dev, and was satisfied.)

EDIT:
Wow, while I was writing my reply, the patch was committed by deekayen, thanks. :)

Comment #23

Sk8erPeter commented 30 August 2013 at 17:11

Version:	7.x-1.x-dev	» 6.x-1.x-dev
Status:	Reviewed & tested by the community	» Patch (to be ported)

While I was writing the post, the status and version has been changed (this is why I accidentally changed them when sending the form :) ), switching it back...

Patch (to be ported)

» Fixed

I think this was fixed by #2103303: Query string parameters not preserved in redirect

Comment #27

7 November 2013 at 01:10

Status:

Fixed

» Closed (fixed)

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

Comment #28

knalstaaf commented 13 December 2013 at 12:15

@pwolanin: this is not the case for D6.

D6 requires the patch in #15.

Redirection not working on multi-lang site with prefixes

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Comment #19

Comment #20

Comment #21

Comment #22

Comment #23

Comment #24

Comment #25

Comment #26

Comment #27

Comment #28

Referenced by

News items

Our community

Documentation

Drupal code base

Governance of community