Hi Guys

I'm opening this issue to contrib some changes that we have for globalredirect that we currently use on every site we build.
The problem we have is that globalredirect makes a wrong use of prefixes, and it only works with the language prefix, but when you use a module that also uses prefixes, things wil break.

What we do, is change the what we compare things.

The way we see things:

  1. First we should get where the user is standing
  2. We then check where we want the user to be (alias)
  3. Then we compare if the user is where we want him/her to be
  4. if different, redirect.

So what does this patch changes?

  1. There is no need to run hook_url_outbound more than once. First invocation is enough to get the final modified path. We remove the second invocation, which by the way, passes an $alias as the path, which makes no sense since we only want to modify unaliased paths.
  2. We keep the original $request_path (globalredirect_request_path()) $current_path (current_path()) and we play around with $path which was initially the same as $current_path. This will be our reference and is from where we get the alias.
  3. This $path DOES NOT includes prefixes nor does the $alias which we compare to $request_path which DOES contain prefixes.
  4. We need to include prefixes in language check, as not having them is one of the faults of the module (line 171+), this causes infinite loops when other modules implement their own prefixes.
  5. Prefix needs a slash at the end EVEN if we have no alias, (line 187+) This causes again infinite loops when prefixes other than language are used.
  6. Prefixes need to be taken into account again in case insensitive check (line 207+), again infinite loop
  7. Redirects should not be made to aliases, but to drupal paths, as they get converted afterwards. (line 217+)

Also, included in the patch is a different version of globalredirect_request_path() which emulates request_path(). This is as I see it, a more accurate way fo doing it as it mimics drupal's way better, but feedback on that is appreciated.

If any explanation is required please let me know.

I would really appreciate if this gets reviewed and committed. We have't been able to use the original module at all, because of the infinite redirect with multiple prefixes.

Cheers

CommentFileSizeAuthor
#23 globalredirect-correct_use_of_prefixes-1843500-23.patch8.4 KBsgdev
#20 globalredirect-correct_use_of_prefixes-1843500-20.patch8.61 KBgnucifer
#20 interdiff-1843500-5-20.txt1.24 KBgnucifer
#19 globalredirect-correct_use_of_prefixes-1843500-19.patch8.5 KBgnucifer
#19 interdiff-1843500-5-19.txt970 bytesgnucifer
#15 globalredirect-correct_use_of_prefixes-1843500-6.patch8.3 KBbendev
#6 globalredirect-correct_use_of_prefixes-1843500-5.patch8.27 KBcyberschorsch
#4 globalredirect-correct_use_of_prefixes-1843500-4.patch2.41 KBjm.federico
#3 globalredirect-correct_use_of_prefixes-1843500-3.patch5.72 KBjm.federico
#3 globalredirect-base globalredirect_request_path_in_ request_path-1843500-3.patch1.91 KBjm.federico
#1 globalredirect-correct_use_of_prefixes-1843500-1.patch7.62 KBjm.federico

Comments

jm.federico’s picture

jm.federico commented
StatusFileSize
new globalredirect-correct_use_of_prefixes-1843500-1.patch7.62 KB
jm.federico’s picture

jm.federico commented
Status: Active » Needs review
jm.federico’s picture

jm.federico commented
StatusFileSize
new globalredirect-base globalredirect_request_path_in_ request_path-1843500-3.patch1.91 KB
new globalredirect-correct_use_of_prefixes-1843500-3.patch5.72 KB

I'm splitting in 2 patches (just to have it clearer).
Attached patches agains 1.5

Cheers

jm.federico’s picture

jm.federico commented
StatusFileSize
new globalredirect-correct_use_of_prefixes-1843500-4.patch2.41 KB

Ok, I'm attaching a new patch which fixes the domain language redirection.

dooug’s picture

dooug commented

These patches in #3 and #4 applied cleanly (if applied in order) to the latest 7.x dev of global redirect. Seems to resolve any of the redirect loops that I was experiencing. I'm not familiar enough with the module code to comment on the validity of the patches' changes.

cyberschorsch’s picture

cyberschorsch
Primary language German
commented
StatusFileSize
new globalredirect-correct_use_of_prefixes-1843500-5.patch8.27 KB

I failed applying all patches together so I manually applied the last patch in #4. I created a single patch for all of these changes.

This fixes the Issue for me.

mike stewart’s picture

mike stewart commented

After nearly a year, and multiple patches, it'd be nice see a maintainer of globalredirect at least respond to this issue. Does the direction make sense? Any reason it hasn't been committed to globalredirect?

raulmuroc’s picture

raulmuroc commented

Agree with @mike_stewart that more continuity is needed! Anyway, I would apply more testing to the latest patch before commit it. It needs communiti rewiew&test!

Thanks!

stmh’s picture

stmh commented

patch #6 seems to work in combination with a complicated purl-setup on my end.

raulmuroc’s picture

raulmuroc commented

@stmh, could you describe your "complicated purl-setup" through steps that other Drupal users can benefit of it?

Thank you in advance.

aaronbauman’s picture

aaronbauman
he/him
commented

have been using patch #6 in production for about a year now, working good

mike stewart’s picture

mike stewart commented
Assigned: Unassigned » mike stewart

Realized I just noticed this was never RTBC. I am assigning to me so I can re-test this in the next few days and hopefully get it some new attention now that there seems to be new maintainers wth commit access. I plan to accomplish it by Tuesday: 9-30-2014.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Sounds like a significan cleanup.
While there is some risk / concerns with Drupal 7, i would expect it to go in more easily in 8.x.

Also with 8.x we tried to push test coverage to a next level and we really have tests for every process that matters.
Sure, all the infinite loops you're referring to need to get a test so we can be sure things are clean now.
Hope you find time to work on some tests!

mike stewart’s picture

mike stewart commented
Assigned: mike stewart » Unassigned

ok, lots of life & client work came up ... hope to jump on this next week ... but unassigning in the meantime

bendev’s picture

bendev commented
StatusFileSize
new globalredirect-correct_use_of_prefixes-1843500-6.patch8.3 KB

Here is a new version of patch for the current code.
it seems to work for me as well Thanks; Will continue further testing

TelFiRE’s picture

TelFiRE commented

I'm just wondering where things are at on this issue, since the issue linked in the home page as the reason globalredirect is incompatible with this module, links here as its solution.

Incompatible with Global Redirect: #774950: Incompatible with hook_url_inbound_alter()

Rameez’s picture

Rameez commented

Patch at #6 works fine. Wonder when this will be committed.

gnassar’s picture

gnassar commented
Status: Needs review » Reviewed & tested by the community

Patch still applies to 7.x-1.x-dev. Ready for RTBC.

gnucifer’s picture

gnucifer commented
StatusFileSize
new interdiff-1843500-5-19.txt970 bytes
new globalredirect-correct_use_of_prefixes-1843500-19.patch8.5 KB

@TelFiRE I was wondering the same thing, it seems like the current patch in this thread does not fix the incompatibility issue with hook_url_inbound_alter() as discussed in https://www.drupal.org/node/774950. I created a revised patch that also takes care of this issue. Hopefully the change does not break anything else. The logic in globalredirect_init() is pretty complicated so hard to tell, I tried to be as nonintrusive as possible. It is possible that the whole logic can be refactored and simplified, but I choose to sneak in the change at the "right" spot in the current flow instead.

gnucifer’s picture

gnucifer commented
StatusFileSize
new interdiff-1843500-5-20.txt1.24 KB
new globalredirect-correct_use_of_prefixes-1843500-20.patch8.61 KB

I think my previous patch could cause a redirect loop if the normalized request path also have an alias. I have refactored the patch using more a defensive approach that should not result in this problem.

BarisW’s picture

BarisW commented
Status: Reviewed & tested by the community » Needs work

Can we please have a re-roll against the current dev version?

sgdev’s picture

sgdev commented
Status: Needs work » Reviewed & tested by the community

The patch in #20 applies cleanly to 7.x-1.x-dev. I see no need for a re-roll.

Yes, it does not apply cleanly to the current live version (7.x-1.6), nor should it.

sgdev’s picture

sgdev commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new globalredirect-correct_use_of_prefixes-1843500-23.patch8.4 KB

Examining more closely, there is an issue that needs to be resolved due to a bit of code already being moved in dev. Attached is a new patch for review.

Status: Needs review » Needs work

The last submitted patch, 23: globalredirect-correct_use_of_prefixes-1843500-23.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

artematem’s picture

artematem commented
Status: Needs work » Reviewed & tested by the community

Patch #23 fixed the issue for me.