Hey,

I'll just explain what I want to accomplish. I thought before I start tearing apart the code perhaps there is an easy way to do this, or you have figured it out already yourself.

I create a redirect: http://mysite.com/page

to goto: http://mysite.com/this_is_the_title_of_the_node

BUT I want to add URL queries to the redirect and have them transfer over to the redirected page...

http://mysite.com/page?arg=somearg&another=anotherarg

redirects to

http://mysite.com/this_is_the_title_of_the_node?arg=somearg&another=anot...

I think that explains it..

Thanks

CommentFileSizeAuthor
#40 reroll_4-25-2014.patch3.26 KBShalomPisteuo
#28 361150_querystring_simplified_28.patch2.09 KBgreggles
#18 361150_querystring_simple_with_tests-D6.patch5.43 KBgreggles
#17 path_redirect-361150.patch2.78 KBpobster
#12 361150_querystring_simplified.patch2.65 KBgreggles
#8 361150-path_redirect-preserve-querystring.patch2.9 KBmrfelton
#3 path_redirect.module.361150-5.x-1.x-dev.patch2.53 KBjaydub
#2 path_redirect.module.361150.5.x.1-2.patch2.34 KBjaydub
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mstef’s picture

mstef CreditAttribution: mstef commented

OK i just went out and did it myself...

I'll show the added parts in path_redirect_init()

Line 39: (added $query_string)

// If there is no match against path and query string, check just the path
if (!$r) {
    $query_string = ereg_replace('.*\?', '', $path);
    $path = preg_replace('/\?.*/', '', $path);
    $r = db_fetch_object(db_query("SELECT rid, redirect, query, fragment, type FROM {path_redirect} WHERE path = '%s' OR path = '%s'", $path, urlencode(utf8_encode($path))));
}

Line 46: (added second IF statement)

// only redirect if allow_bypass is off or bypass is not requested
if ($r && !(variable_get('path_redirect_allow_bypass', 0) && !empty($_GET['redirect']) && $_GET['redirect'] == 'no') && url($r->redirect) != url($path)) {
    //If no query set in DB and query provided to redirect, use that
    if(!$r->query && $query_string) {
    	$r->query = $query_string;	
    }
jaydub’s picture

jaydub CreditAttribution: jaydub commented
FileSize
path_redirect.module.361150.5.x.1-2.patch2.34 KB

I've ended up needing to consider this option as well. I've attached my patch which uses a configurable value for what to do with a query string.

This patch is based off of the current stable release 5.x-1.2. I see that the latest code for 5 has some significant changes. I'll attempt to follow up with a patch against the -dev at a later time.

jaydub’s picture

jaydub CreditAttribution: jaydub commented
FileSize
path_redirect.module.361150-5.x-1.x-dev.patch2.53 KB

and a patch against the latest dev snapshot

jaydub’s picture

jaydub CreditAttribution: jaydub commented
Assigned: Unassigned » jaydub
nickorr’s picture

nickorr CreditAttribution: nickorr commented

Jaydub,

Have you put the same patch into the v6 version yet? Do you know if there is any similar change that will work in v6?

Thanks,
Nick

jaydub’s picture

jaydub CreditAttribution: jaydub commented

I thought I had posted a patch for d6 too but I guess I forgot. I will see if I can add a patch when I am at the office.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Version: 5.x-1.2 » 6.x-1.x-dev
Status: Active » Needs work

I will only work on feature requests for the Drupal 6 or 7 branches only.

mrfelton’s picture

mrfelton CreditAttribution: mrfelton commented
Assigned: jaydub » mrfelton
Status: Needs work » Needs review
FileSize
361150-path_redirect-preserve-querystring.patch2.9 KB

Here is a D6 version, which seems to be working well so far.

EDIT: Please use the -p1 strip option with this patch.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

I think this idea is generally very useful/important. I don't entirely understand the need for append vs. replace and think this could probably be a "yes/no" style option.

For anyone else on this thread who uses this feature, can you explain why it should be a choice? My proposal is that we should always append the information using the current system "// Values from the requested will override those specified in the redirect if they collide."

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

I would think the query string set in the redirect will always stay. The current query string on the page being redirected would be merged in. If I've set a redirect with specific query strings, I've probably meant for those to not be changed at all and not overwritten.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

Well, I think that's why there are three options in the patch and perhaps room for a 4th option ;)

I don't really care: my use of this is for a situation without any options in the redirect and so the incoming querystring will always be used 100%. But I would like to get this committed and into a stable release, so I want to know what other people think.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
FileSize
361150_querystring_simplified.patch2.65 KB

Here's an updated patch simplified per discussion with Dave in irc.

I looked to write some simpletests for this, but the current tests are failing (poor excuse, I know).

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Yep, and excuse no more. Just a simple test case will do.

pobster’s picture

pobster CreditAttribution: pobster commented

+1 for this, has it been forgotten about? It's graceful and it works, I like it!

Pobster

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Issue tags: +Needs tests

Needs tests.

pobster’s picture

pobster CreditAttribution: pobster commented

Aha, right... I'll write some tomorrow.

Thanks,

Pobster

pobster’s picture

pobster CreditAttribution: pobster commented
Status: Needs review » Patch (to be ported)
FileSize
path_redirect-361150.patch2.78 KB

Apologies, tomorrow turned into the day after... Etc... Life often gets in the way of fulfilling promises...

Anyways, let me know your thoughts on this patch - it doesn't include the original patch from http://drupal.org/node/361150#comment-3270038 which will need to be applied first.

Pobster

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Status: Patch (to be ported) » Needs review
Issue tags: -Needs tests
FileSize
361150_querystring_simple_with_tests-D6.patch5.43 KB

I think needs review is the right status here.

I've combined them which fixes some offsets. And removing the needs tests tag since it's got tests.

Status: Needs review » Needs work

The last submitted patch, path_redirect-361150.patch, failed testing.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Status: Needs work » Needs review

Setting back to needs review so testbot will test my reroll patch.

pobster’s picture

pobster CreditAttribution: pobster commented

Just to point out that *maybe* it would be nice to report the query parameters in the displayed message, I didn't bother but well... Just sayin'

$this->assertEqual($this->getUrl(), url($redirect, array('absolute' => TRUE) + $options), t('Redirected from %request to %redirect.', array('%request' => $request, '%redirect' => $redirect)));

Maybe test if it's populated, implode it or print_r($options, TRUE); dunno... Wasn't my intention to make it pretty, just make it work!

Also, I did initially write a 'fail' test to show that it does fail if the setting is FALSE but I removed it as I didn't really think it that relevant. If you want me to put it back in - please give me a shout!

Pobster
edit: Also I'm sure it was obvious but I did purposefully set that status as I knew my patch would fail testing due to not having the original patch applied to it!

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented

subscribing

pobster’s picture

pobster CreditAttribution: pobster commented

Hmmm any idea why testbot is taking so long?

We're successfully using this on the Economist site right now!

Thanks,

Pobster

asak’s picture

asak CreditAttribution: asak commented

+1 - works like a charm.

YK85’s picture

YK85 CreditAttribution: YK85 commented

+1 subscribing

doublejosh’s picture

doublejosh CreditAttribution: doublejosh commented

Zoinks, I really really need this too!

doublejosh’s picture

doublejosh CreditAttribution: doublejosh commented

Applied patch 361150 with full success.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
FileSize
361150_querystring_simplified_28.patch2.09 KB

Reroll, still applies.

pobster’s picture

pobster CreditAttribution: pobster commented

The patch is missing the simpletest?

Pobster

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Status: Needs review » Needs work

Sorry, I forgot about those. Care to re-roll?

pobster’s picture

pobster CreditAttribution: pobster commented

Yeah no worries, I'll do it tomorrow morning... Which... Is only about six hours from now... I should probably go to bed!

Thanks,

Pobster

doublejosh’s picture

doublejosh CreditAttribution: doublejosh commented

Reping about re-rolling.
I, for one, use this patch on production.

tangent’s picture

tangent CreditAttribution: tangent commented

Does the patch still need work? This issue is becoming ancient.

joep.hendrix’s picture

joep.hendrix CreditAttribution: joep.hendrix commented
Issue summary: View changes

please commit!

joep.hendrix’s picture

joep.hendrix CreditAttribution: joep.hendrix commented
Status: Needs work » Reviewed & tested by the community

The last submitted patch, 18: 361150_querystring_simple_with_tests-D6.patch, failed testing.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Status: Reviewed & tested by the community » Needs work

It doesn't apply, so needs a reroll before it can be committed.

ShalomPisteuo’s picture

ShalomPisteuo CreditAttribution: ShalomPisteuo commented

I will try to reroll this following the instructions I found on this page https://drupal.org/patch/reroll.

The last submitted patch, 18: 361150_querystring_simple_with_tests-D6.patch, failed testing.

ShalomPisteuo’s picture

ShalomPisteuo CreditAttribution: ShalomPisteuo commented
FileSize
reroll_4-25-2014.patch3.26 KB
ShalomPisteuo’s picture

ShalomPisteuo CreditAttribution: ShalomPisteuo commented
Status: Needs work » Needs review
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

thanks shalomPisteuo for the reroll. The patch in #40 is a bit larger than the one from #28. It looks like you added some code related to a test, which is great! It would be good to describe that change to help others know about it :)

ShalomPisteuo’s picture

ShalomPisteuo CreditAttribution: ShalomPisteuo commented

Where do you want me to put the description of the changes? In a code comment or here on this thread?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

In a comment here on this thread. I think the commenting in the patch is fine.

ShalomPisteuo’s picture

ShalomPisteuo CreditAttribution: ShalomPisteuo commented

Sure no problem. My contribution was only a re-roll per comment #28. The tests came from pobster in comment #17. The beautiful fix I believe came from greggles. I hope this helps to clarify the contribution. Any chance on getting this merged?