Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This is a patch that solves plenty of problems with infinite loops, including the one with SubPath URL Alias (aka Subpathauto for D7) #346911: Redirect Loop and custom_url_rewrite ignored BUT ONLY IN D7
It was an almost impossible task in D6 but now with the hook for url_outbound_alter it seems to be the way!
D7 branch still tries to invoke
custom_url_rewrite_outbound(...);
which is not used anymore.
Instead we should call
drupal_alter('url_outbound', ...);
Issue #346911: Redirect Loop and custom_url_rewrite ignored for D6 is fixed in D7 with this.
Comment | File | Size | Author |
---|---|---|---|
#1 | globalredirect-7.x-1.x-dev_1130210_1.patch | 766 bytes | jm.federico |
Comments
Comment #1
jm.federico CreditAttribution: jm.federico commentedPatch attached
Comment #2
joelstein CreditAttribution: joelstein commentedGood catch. However, I'm using this in conjunction with Persistent URL (the D7 version is available here: http://drupal.org/node/1070222), and I still get redirect loops. If I make Global Redirect lighter than PURL, then I don't get any redirect loops anymore. But the PURL rewriting doesn't work in this case.
Seems like it's close...
Comment #3
jm.federico CreditAttribution: jm.federico commentedYeap, I had the same issue with PURL in D6, and it gave me headaches. I stopped using it since it gave me more trouble than solutions, although I would love to get it fixed.
I submitted a hackis patch in #346911-83: Redirect Loop and custom_url_rewrite ignored a while ago where I found a solution for D6, but it really is hackis
Basically what it does is it emulates the way PURL modifies the URL before making the comparison. it works, but I don't like it. globalredirect_clean_path()
For D7, I think hook_url_outbound_alter needs to be invoked in more places for it to work.
Cheers
Comment #4
Kiphaas7 CreditAttribution: Kiphaas7 commentedjm.federico: if I'm right, you're patch is trying to implement it's own url_outbound_alter hook. Because one registers new alter hooks with drupal_alter(). So it's wrong per definition, see drupal_alter and the already used code of drupal_alter('url_outbound', ...) in url():
http://api.drupal.org/api/drupal/includes--module.inc/function/drupal_al...
http://api.drupal.org/api/drupal/includes--common.inc/function/url
I think a more suitable implementation would be something along the lines catch proposes in this issue:
#774950: Incompatible with hook_url_inbound_alter()
Comment #5
jm.federico CreditAttribution: jm.federico commentedYeap, that is correct for D7, but since in D6 custom_url_rewrite_outbound wasn't a hook (bump) there was no way for module to alter URL's in a consistent way, there was no hook for it, so one had to be cerated. In D7 we have HOOK_url_outbound which should solve the problem.
BTW, you might want to check the patch in #346911-85: Redirect Loop and custom_url_rewrite ignored it was a cleaner version.
Anyway, if you check my patch, you'll see that I was calling the function I created in 3 places, which is where we need to get the clean verison of the url. I have no installation of PURL anywhere nearby to thoroughly test, but I'm pretty sure that if you modify the D7 version to call HOOK_url_outbound where I was calling my PURL fix function, it should work.
Comment #6
nicholasThompsonI have committed this into 7.x-1.x.dev:
http://drupalcode.org/project/globalredirect.git/commitdiff/671a8663c52f...
Question: How do I go about testing this? I'm considering adding a submodule to GR which is only used for simpletests (one which implements the appropriate hook inbound/outbound alters, for example). I have the beginnings of one here: #774950: Incompatible with hook_url_inbound_alter().
Comment #7
Kiphaas7 CreditAttribution: Kiphaas7 commentedErr, why? From the discussion above it was already shown that the patch does absolutely nothing for drupal 7. It was intended for drupal 6, I think. Confusing also for me, since this issue is filed against d7.
I doubt registering the url_outbound hook should be done in the globalredirect module (dedicated module preferably), but that is a side note.
Committing this to the d7 branch is just wrong IMHO, see my previous reply.
Comment #8
jm.federico CreditAttribution: jm.federico commentedHello
The patch is for D7 and fixes redirects in plenty of situations, like when using http://drupal.org/sandbox/davereid/1078890 which is a sandbox for the D7 version of http://drupal.org/project/subpath_alias
It still needs some work to fix the problems with PURL, but if someone tries what I mention in number #5 I think it would fix purl problem.
Just to make sure we are all in the same page:
@Kiphaas7 Did you actually read the patch? And the issue? My patch implements drupal_alter('url_outbound') in the same way url() does. And it does fixes a problem. Also, correct me if I'm wrong, but registering a hook is not the same as invoking it. Here I'm not registering it, I'm invoking it.
Cheers
Comment #9
nicholasThompsonI agree with jm.federico - Invoking a hook (either via drupal_alter, module_invoke or module_invoke_all) is not the same as defining one.
The patch seems to address several issues.
The suggestion which catch made in #774950: Incompatible with hook_url_inbound_alter() needs to be considered on two points: 1) it's for inbound, not outbound (two different ways of altering links) and (2) it "disables" GR if it thinks a URL has been altered. Ideally we don't want that.
I'm gonna mark this as fixed (as the patch does what it says on the tin). It'd be great if we could cover any specific issues in separate tasks.
Comment #10
Kiphaas7 CreditAttribution: Kiphaas7 commentedI stand corrected. For the record, I did read the issue, just made a fundamentally wrong assumption. Thanks for explaining jm.federico!
Comment #11
Akaoni CreditAttribution: Akaoni commentedFYI: this also fixes an issue with a module I'm working on:
http://drupal.org/node/1145382
Comment #13
rudiedirkx CreditAttribution: rudiedirkx commentedIt's very strange to me that globalredirect calls url_outbound on the request path... The request path should be the literal request path, shouldn't it? Outbound alter should be called to determine the page's alias, not to change the incoming/request path.
Can someone explain why it does that?