When I set the destination parameter (or any parameter in fact) in the logintoboggan field 'Redirect Path on Registration' or Confirmation, it gets encoded into a nonexisting url.

When I have set
/mhp/register?destination=user
logintoboggan tries to redirect to
/mhp/register%3Fdestination=user

Which breaks the ability to give more advaced urls.

CommentFileSizeAuthor
#5 logintoboggan.module_0.txt41.52 KBdkruglyak
#4 logintoboggan_support_queries_0.patch2.78 KBBèr Kessels
#1 logintoboggan_support_queries.patch1.87 KBBèr Kessels

Comments

Bèr Kessels’s picture

Bèr Kessels commented
Status: Active » Needs work
StatusFileSize
new logintoboggan_support_queries.patch1.87 KB

First patch. To show my direction of thinking.

Idea is to let logintoboggan_process_login() return an array with query, path and fragment, which can be passed into drupal_goto().

This patch does not yet work, it now redirects to
/path?query#destination=user
instead of
/mhp/register?destination=user

sidenote: this module has a serious problem with return values and functions returning values vs doing stuff. They are really scattered all over the place, making a simlpe change like this issue an enourmous all over the place overhaul.
another sidenote: one should never check $foo != '' instead use !empty($foo)

markhope’s picture

markhope commented
Title: ?destination= parameter in redicrect url gets encoded » subscribe

subscribe

Bèr Kessels’s picture

Bèr Kessels commented
Title: subscribe » ?destination= parameter in redicrect url gets encoded

After some conversation on IRC I thought that hacking some $_REQUEST[] changes in may help. Like this:

  if ($redirect != '') {
    list($path, $destination) = explode('?destination=', $redirect);
    if ($destination) {
      $_REQUEST['destination'] = $destination;
    }
    return $path;
  }

But that actually redirects us to $destination, instead of appending that to the url... grrr.

Bèr Kessels’s picture

Bèr Kessels commented
Status: Needs work » Needs review
StatusFileSize
new logintoboggan_support_queries_0.patch2.78 KB

w00t. it works on our installation with the latest patch.
I moved some return values around and removed one layer of spagetti :) so maybe custom modules calling such an PI will now break?

dkruglyak’s picture

dkruglyak commented
Version: 4.7.x-1.0 » 4.7.x-1.x-dev
StatusFileSize
new logintoboggan.module_0.txt41.52 KB

Not sure if this is 100% related but I believe this is a different manifestation of the same problem...

I am using 'me' module, which I patched according to http://drupal.org/node/127913 (you can get the file to test).

What I have is /user/me redirect to /user/login (logintoboggan) to log in the user and then redirect back to /user/me. When logintoboggan opens the login form it grabs destination URL and shows it on login form as a text message.

I was able to temporary get rid of the problem by adding unset($form['message']); in logintoboggan_form_alter under user_login case. However, I wonder if this 'message' could/should be preserved.

Attached is my 4.7 file patched.

hunmonk’s picture

hunmonk commented
Status: Needs review » Needs work

@dkruglyak: you can't expect me to review a patch like that. please submit you patch in the proper unified diff format.

@ber:

  • isn't there a more elegant way to parse the destination string, like a native php function?
  • //always return a string: consistency++ <-- this code comment seems off to me.
  • your logic in function logintoboggan_user_register_submit looks backwards. i would think you'd want to set those redirects if $redirect path _was_ empty.
hunmonk’s picture

hunmonk commented
Title: ?destination= parameter in redicrect url gets encoded » allow query and fragment parts in redirects
Version: 4.7.x-1.x-dev » 5.x-1.x-dev
Category: bug » feature
Status: Needs work » Patch (to be ported)

@ber: i wasn't happy with some of the aspects of your implementation, so i made adjustments accordingly. mostly, it makes more sense to use parse_url() than to do things by hand.

i've committed the change to 6.x. i kind of view this as a new feature, not a bug -- as such i'm not compelled to backport it. however, i'll leave this marked as to be ported for now. if somebody comes along and ports it before the feature freeze for 5.x, then i will review/commit -- otherwise this will be a 6.x and forward feature.

hunmonk’s picture

hunmonk commented
Status: Patch (to be ported) » Closed (won't fix)

5.x no longer supported.