I just peeked over this module, since i am currently developing something similar. Now that i solved it, i want to share some.

As i can see for 6.x, this module utilizes its own redirect header statement. I suggest to use drupal_goto('path', drupal_get_destination()), since it will shutdown drupal's flow correctly. Also drupal_get_destination() includes other query-variables, not only the internal drupal-path, as well as messages that has been set before.

The mentioned recursive redirect loop can be avoided by checking the path against 'user' in arg(0).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Bevan’s picture

Status: Needs review » Needs work

Redirect gets in infinite loop as anonymous user hitting node/add with i18n path prefixes.

Bevan’s picture

Status: Needs work » Needs review
FileSize
595 bytes

This version of the patch employs drupal_get_destination(), but not drupal_goto() or the check for arg(0) == 'user'. This enables the module to work with i18n language prefixes, and doesn't infinitely redirect.

Without using drupal_get_destination() while i18n path prefixes are enabled, the module redirects to, for example en/en/node/add. Note the double-occurrence of the language prefix en. This results in a 404 Page not found error.

deekayen’s picture

Status: Needs review » Fixed

committed to DRUPAL-6--1 and HEAD

@Bevan, your patch didn't conform to http://drupal.org/patch/create, but I used it anyway since it was so small.

deekayen’s picture

Category: feature » bug
Status: Fixed » Needs work

From Bevan:

Upon going to /admin as an anonymous user, the browser is redirected to /user/login?destination=r4032login instead of /user/login?destination=admin.

@Bevan: it's your patch... roll it back?

deekayen’s picture

@Bevan: I added you to CVS so you can just fix it instead of having to roll patch. I ended up committing it to DRUPAL-5, DRUPAL-6--1, DRUPAL-7--1, and HEAD, so this makes quite a mess.

deekayen’s picture

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

Nevermind, I rolled it back. Is there still some underlying problem from the original issue submission that needs to be resolved?

Bevan’s picture

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

(#518534: Redirects to /r4032login instead of old path was marked duplicate.)

I haven't tested whether #518534: Redirects to /r4032login instead of old path is a result of this (#339120: Redirect issues) change or not. I'm merely wondering. I would expect that I would have noticed a bug as significant as this in developing the patch. Perhaps there are other or more obscure causes? Have you tested?

I don't have time to work on it this week, but will probalby get around to it sooner or later...

Bevan’s picture

It turns out drupal_get_destination() is indeed the cuprit here. This needs rerolling. Possibly we should state on the project page that this module is not compatible with i18n path prefixes, in the meanwhile?

ianchan’s picture

subscribe

Bevan’s picture

I think the cleanest way to fix this is to check for language prefixes in $_GET['q']. But this would require quite a bit of fairly ugly code, and I couldn't find the code that does this already in Drupal to see if there is something re-usable.

Breakerandi’s picture

Version: 6.x-1.x-dev » 6.x-1.2

i18n redirect still not working on 6.12, will it be fixed soon?

Bevan’s picture

Breakerandi; Patches welcome

Breakerandi’s picture

My profession is to report bugs, I can't do anymore for the community because I know nothing about php.. sorry..

geraldito’s picture

I don' t get what's the point of #4 as for me patch from #2 fixed the redirection problem with i18n.
@#1: anonymous user hitting node/add with i18n path prefixes doesn't effect an infinite loop on my site (maybe because there is content creation allowed for anonymous users?)
@#1: anonymous user hitting /admin gets redirected to user/login?destination=admin so for me everything seems to be fixed with this patch.

mike-l’s picture

FileSize
599 bytes

I find this bug too... and I see only 1 way how to fix this issue.
idea is:
remove laguage prefix.

in the
r4032login.module

$destination = drupal_urlencode($_REQUEST['q']);

i suggest to rewrite like this:
$destination = removeLanguagePrefixFromDestination(drupal_urlencode($_REQUEST['q']));

and add new function:

function removeLanguagePrefixFromDestination($destination)
{
$elements = explode('/', $destination);
$prefix = $elements[0];
$languages = language_list();
foreach ($languages as $lang)
if(strcasecmp($prefix, $lang->language)==0)
return substr($destination, strlen($prefix)+1); // remove prefix with slash
return $destination;
}

and everything works fine

any thoughts about this solution are welcome )
-----------------------------------------------------------------------------------
patch #2 doesn't work in my case

mike-l’s picture

FileSize
806 bytes

i'm sorry but in my previous message patch.patch was not correctly generated .I recommited it in this message.

Bevan’s picture

mike-l, Please see the Drupal coding standards. Among other things removeLanguagePrefixFromDestination() should be r4032login_remove_language_prefix_from_destination().

However I think there is a Drupal core API function for this – I think it's arg() with no parameters, or drupal_get_normal_path().

deekayen’s picture

Status: Needs work » Needs review

I applied the general idea for a fix to DRUPAL-6--1, DRUPAL-7--1, and HEAD. Please try it so I can make a 6.x-1.3 release if it works.

vlad.k’s picture

subscribing...

John Franklin’s picture

Is there a patch for this that I can apply to a 6.x-1.2 install? (I hate to run dev versions of a module on a production site, but backporting specific patches is usually OK.)

rolfmeijer’s picture

lotyrin’s picture

Title: Redirect issues » Query string is lost on redirect

Updating title. This is the concern of this issue, correct? If not, feel free to change it back.

I am also experiencing this problem.

lotyrin’s picture

Title: Query string is lost on redirect » Redirect issues

Okay, in addition to the query string getting lost, there is an assumption that a language's prefix is always the same as the language.

Writing a patch.

lotyrin’s picture

Here are some improvements:

drupal_goto() should be used (to invoke exit hooks, save session state, etc.), and *can* be used as long as we unset the destination.

calling exit() denies modules that hook exit from having their hooks called. We don't need to call theme('page'), just return.

Wrote a new function that determines the proper destination, accounting for query strings and improving upon the language prefix fixing code (prefix testing was not accounting for the possibility of disabled languages, or of languages having custom prefixes). This code is inspired by code solving the same problem from logintoboggan, but differs in that it is smarter than logintoboggan.

A couple simple tweaks to logic flow and variable scope.

These fixes are against 6.x-1.x, but if we don't want them there, I can re-roll against a different head.

If these patches are accepted in one head, I will own re-rolling for other heads.

lotyrin’s picture

RobLoach’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev

Hey lotyrin, would you like commit acccess to commit these patches? We would need to make sure they are persistent across both the 6.x and 7.x releases though.

BenK’s picture

Subscribing

lotyrin’s picture

@Rob Loach re #26, sure. I'll make sure to get them in all heads.

marcvangend’s picture

Hi, what's the status here? The patches do not apply to 7.x-1.5 and the language fix that was committed in #18 does not work - at least not anymore. (Looks like it has something to do with leading/trailing slashes in the destination path.)

What's the best approach to get r4032login working on a multilingual D7 site?

deekayen’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs review » Needs work

I just marked #1304864: Doesn't work in subdirectory install as duplicate of this because it's working on the same lines of code for a similar redirect breakage. I'm completely confused as to why there are 5 different patches working on totally different parts of the module for this issue and then why the issue was bumped to 7 when the proposed fixes are for 6, which is probably a good place for them to start. Can we focus on fixing the breakage first?

deekayen’s picture

Is perhaps the Redirect API a better way to handle this as a new dependency?

http://drupal.org/project/redirect

Herr Lehmann’s picture

This a very straight forward approach, initially created in #1304864: Doesn't work in subdirectory install. I will have a look at http://drupal.org/project/redirect, to check how it can provide any help.
For right now I reworked my patch (for 7.x-1.x) and converted tabs to spaces. This is my first creation of a patch, so please be patient ;)

lotyrin’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Assigned: Unassigned » lotyrin

I fixed these once in 6, I'll do so again in both branches.

lotyrin’s picture

Status: Needs work » Fixed

The following have been fixed for both 6.x and 7.x (some were fixed in 7.x already):

Unicode characters in destination
Sites that are installed in subdirectories
Sites that don't have Clean URLs enabled
Sites which use language prefixes other than the default

Any futher problems should open new issues.

I'll let the other maintainers decide if these changes are ready for a stable release, but until then they're in -dev.

Status: Fixed » Closed (fixed)

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

darthf1’s picture

Status: Closed (fixed) » Active

Still having the multilanguage path prefix issue, with the latest dev (7.x-1.x-dev 2012-Apr-20)

nl/user/login?destination=nl/admin results in redirecting me to nl/nl/admin

ddangel’s picture

@ fcjversc

me too. But if you work with regex, you can fix this very fast.
[My urls are de/.../.../bla_word_with_de_bla, en/.../.../bla_word_with_de_bla, [lang_code]/.../.../bla_word_with_de_bla]

just change r4032login.module like:

// Get alias for destination if there is one
  $destination = drupal_get_path_alias($destination);
  $destination = preg_replace('/^de\//','',$destination);
  $destination = preg_replace('/^en\//','',$destination);
  $destination = preg_replace('/^ru\//','',$destination);
  $destination = preg_replace('/^kmr\//','',$destination);
  $destination = preg_replace('/^ckb\//','',$destination);
  $destination = preg_replace('/^ar\//','',$destination);
  $destination = preg_replace('/^ka\//','',$destination);
  $destination = preg_replace('/^hy\//','',$destination);
  return $destination;

and this should also works and looks better:

// Get alias for destination if there is one
  $destination = drupal_get_path_alias(preg_replace('/^[$variable_for_lang_code_which_i_dont_understand_in_this_module]\//','',$destination));
  return $destination;
boran’s picture

Status: Active » Needs review

With a D/ 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

Could someone else verify this on their own site please?

lotyrin’s picture

Status: Needs review » Closed (fixed)

> Any futher problems should open new issues.

Please open new issues, include affected version information and include steps to reproduce.

Changes should be patches and should be able to pass review.