Closed (fixed)
Project:
Context Redirect
Version:
7.x-1.x-dev
Component:
Code
Priority:
Critical
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
29 Apr 2011 at 19:22 UTC
Updated:
12 Dec 2012 at 23:00 UTC
Jump to comment: Most recent file
Comments
Comment #1
felipepodesta commented+1
Subs
Comment #2
thomjjames commentedHi,
Could you list the conditions & reactions within your context and also the path you are on and the path/url you want to redirect to?
Cheers
Tom
Comment #3
mishac commentedI've got to change some of the names because it's for a project that's not public yet but here it goes:
context name: mobile_mixing
Conditions:
Theme: mytheme_mobile
Path: mixing-101
Reactions:
Redirect: mixing-101/mobile
Comment #4
jvdurme commentedI'm having the same problem.
I'm redirecting the front page to another page for authenticated users and admins.
Weird thing is that the redirect is triggered every other reload. Eg:
- load front: redirects properly
- load front again: no redirect
- load front again: redirects properly
- load front again; no redirect
- ...
Weird, ain't it? ;-)
Hope you can solve this, great module!
Comment #5
mynameiscorey commentedI'm having exact same issue a post #4.
It's a shame because I have a lot of contextual redirects to do and this would make things so quick and easy.
Is this being looked in to?
Thanks.
Comment #6
thedavidmeister commentedthis module does its redirect by cherry picking functionality from drupal_goto. It should work fine, but it also might have problems with caching?
Comment #7
markwk commentedYeah, I'm getting some irregular redirects too. After looking at the code some, I think this module might benefit from following how ctools' http response code works: http://drupalcode.org/project/ctools.git/blob/refs/heads/7.x-1.x:/page_m...
Comment #8
rbennion commentedSame exact problem as well, working every other time:
Here is my context:
[x] Require all conditions
Condition:
Context: path=portal/login/*
User Role: Authenticated
Action:
Redirect: portal
Comment #9
hazah commentedI get the same problem with redirection being triggered (at least fully) every other time. @mynameiscorey The quick and dirty fix is to clone the context. The doubling does force the redirect to actually happen, but at the cost of maintaining two identical contexts.
Comment #10
thedavidmeister commented@hazah - so not worth it >.< this module is "competing" against just doing a drupal_goto inside hook_init. Duplicate contexts sounds like a solid investment in future wtf :P
Comment #11
hazah commentedI didn't say that it was *the* solution. I didn't even say I liked it. It was what I had done to get it working for a deadline. As such, I can't say I "recommend" it. I can only say that it worked like a poor man's bandaid.
Comment #12
thedavidmeister commentedyeah, fair enough. Either way, this module doesn't yet look like a viable replacement for the many alternate ways of redirecting a user to a new page.
I'm surprised to see that this hasn't been pushed to critical earlier considering that this issue is basically "module doesn't do what it is supposed to do some noticeable % of the time"
A redirect is one of those things that is only useful if it works 100% of the time.
Comment #13
hazah commentedIs this module dead on arrival?
Comment #14
jorditr commentedHi hazah, I wouldn't recommend your trick on #9 neither,... but it's true, it works! and meanwhile it saves my day :-P
Maybe we have here a nice "clue" about what may be happenning...
Comment #15
hazah commentedAt this point, I'm not sure this module is of any use. It would be very convenient to be able to use it, but Panels & Rules offer non coding tricks already, and considering the hoops you have to go through for this module, you may as well by pass it until it actually works as advertized. In the mean time, if you wish to use a redirect based on a set Context I recommend: http://drupal.org/project/context_rules. Should be able to set it up to do exactly what this module promised -- in a clean way. Cheers.
Comment #16
thedavidmeister commented@hazah - The Rules redirection suffers from a similar problem in that it won't redirect you if the page is cached too aggressively. This will be true for any Drupal module that attempts to respond to a page request for a path that it didn't define with a redirect - it is always possible to configure your setup to not fire the hooks that result in the redirection so you can break the Rules implementation.
There are many developers who wouldn't want to install a behemoth like panels on their site purely to achieve a redirect, but I assume these people are also familiar with drupal_goto() :P
This context redirection is a good idea but it obviously isn't being actively maintained as this issue is well over a year old and is kind of a deal breaker :(
Comment #17
hazah commentedAh, I see. I do not know enough about the concepts. I do know that Rules have not yet failed on me. I suppose I will eventually discover that to be the case. However, they must have done something right...
Comment #18
thedavidmeister commentedThey'll only fail if you set "aggressive caching" on in D6 or tell D7 not to fire hooks on cached pages.
Comment #19
jorditr commentedI've never set aggressive cache on any site, even I have not set cache on many site because it's not required, they perform very well. And on another "big" sites the solution has been Varnish. So, for me, "aggresive cache" is a corner case.
Comment #20
hazah commentedSo is this fundamentally a Drupal Core issue not being able to accomodate for the idea? What I'm getting is that caching is involved a step above the hooks Context depends on, and that if a task is not already defined as a redirect in hook_menu, it fails? I'm trying to understand if this is a solvable problem or a completely dead end.
Comment #21
thedavidmeister commentedSay you have path X and you define path X's page callback to be a redirect. That's no problem, that will work all the time. That's the way that you'd do things through hook_menu() and that's the way that Panels would do things if you're a Panels fan. The limitation is that this approach can often require a little more php/dev experience and planning ahead to setup.
Alternatively, you have path X but you haven't defined the page callback to be a redirect, instead it renders some content, maybe even just an empty string to create a "blank" page. You now add drupal_goto() to hook_init() to fire whenever the current url matches path X. This is how Rules would work; Rules never defined path X, but it does have the ability to respond with things like redirects if it notices that you're on path X.
The problem with the latter approach is that Drupal can be configured for "aggressive" caching, or to "not fire page hooks" as I previously mentioned. This means that hook_init() is never called for that page load, so the redirect never happens. We can try moving our drupal_goto() call into hook_boot() but we'll find that the function drupal_goto() is not yet defined at this point so we get fatal errors everywhere. We can try redefining drupal_goto() ourselves, but we'll find that it calls other functions which also don't exist at hook_boot() and we basically are heading down a rabbit hole at this point and it would be easier to just define a new path that handles our redirect as in the first example. We could also try the approach presented by the http://drupal.org/project/dynamic_cache module which allows you to disable the cache conditionally in hook_boot(), so you could say "on path X, disable the aggressive cache so I can guarantee that my redirect happens later in hook_init" but this is also arguably harder to setup and maintain than taking the approach in the first example even though it does work just fine.
You can also say "this crazy aggressive caching stuff" is an edge case but it's how really popular modules like http://drupal.org/project/boost work, where they make the assumption that for anonymous traffic at least, you can save yourself a lot of server load by not running *any* php code at all and simply sending back a cached, static html page from a file on the server. So maybe it's an edge case for you, but in the wider community of Drupal users it's actually quite a popular approach to speeding up sites on cheaper/smaller infrastructure where they might not necessarily have access to "advanced" methods like Varnish.
In summary, if you want to be 100% sure that your redirection will work on all server/Drupal configuration combinations with minimal effort and you have paths X and Y, setup a path Z in hook_menu() with a callback that does nothing whether decide whether the user should end up at path X or Y and make sure all incoming links point to Z OR handle the redirection at the server level, rather than the PHP level in a .htaccess file or similar which will end up being much faster and reliable but that's possibly much harder to maintain longer term depending on how the server is managed.
Comment #22
thedavidmeister commentedThe problem runs even deeper for this module based on Context.
Assume we want to workaround the issue by setting up a Dynamic Cache style implementation so we conditionally disable the cache in hook_boot() when we want to perform a redirect.
Unfortunately, because of the way Context works, each "condition" defined by modules has to declare that it has has been met somehow before any "reaction" should be able to execute. Conditions declare themselves as "met" inside hooks, the earliest hook usable being hook_init(), so by the time we realise that we can determine whether we've met any conditions at all it's already well past the point that we could theoretically disable or modify the cache configuration. The only way to do it would be to force cache settings to be toned down a bit for *all* pages, probably in a settings.inc but this would lead to a lot of "wtf" for a lot of developers trying to work with this module, so we couldn't do that.
Essentially though, all of this isn't even a bug in Context, or even in Drupal Core. It comes down to the fact that if you use PHP to both determine whether a redirect should happen and then perform the redirect in the same request, you'll set yourself up for failure whenever the server is configured in a way where PHP isn't actually run for every page request - which is most often encountered when we're implementing caches for entire pages, but could plausibly be encountered in other situations.
So yeah, IMO if you want a solution that works 100% of the time, this module is probably never going to cut it for you even if it was more actively maintained..
Comment #23
hazah commentedI see. Thank you for taking the time to explain. From my experience, I can say that the two concepts cannot be mixed. When you use Context or Rules, it is assumed that you would run PHP. You can say that, because you're interested in taking different execution paths based on conditions. So it would make ZERO sense to cache that aggressively and expect Context to do *anything*. Same goes for Rules. I'd file a setup such as that under "design bug by site builder". Now, lets assume there is nothing preventing hook_init from firing.. I know I haven't set up caching to be aggressive, why would this module fail where Rules do not?
Comment #24
jorditr commentedI agree with hazah on the lack of interest about if something is not working with aggressive cache on, since I believe that situation is a really an edge case. Many other modules as i18n, or some ubercart elements, or statistics elements neither work under those conditions.
On the other hand I have to check if using "Context Rules" performs better than "Context Redirect", because maybe the "Rules" way to perform the redirection could be the answer on how to improve "Context Redirect". I would prefer to set my redirections on Context rather than on Rules. I use both on every site but the context redirection thing is been useful in conjuction with "Context Mobile Detect" to build a mobile solution on Drupal 7. On that environment I prefer to set redirections on Context :-)
Comment #25
thedavidmeister commentedWell once I had to get the front page of a site that was aggressively cached with Boost to redirect to a mobile subdomain and disabling that cache just wasn't an option. That wasn't "edge case" at all. Honestly, Boost is a really popular module, it has over 20k reported installs. We ended up taking the approach of performing the redirect in the .htaccess, but yeah, I can see how this module would still be useful to a lot of people if it worked for non-cached pages. I'm just saying that it will also always "Not always trigger" for a lot of people too.
So, I don't think anyone really disagrees about what's going on. I think I was just thrown by the title of the issue - "Not always triggered" is never going to be fixed.
I'll just update the title of the thread :)
The module doesn't look very complicated though so it shouldn't be too hard to debug.
Can someone experiencing the problem put a dpm in context_redirect_context_page_reaction() just to see if that is getting fired on every page load?
Also add one on line 38 of context_redirect_reaction to see if your url is "validating" because if it doesn't you won't get redirect.
also dpm $_SESSION['context_redirect_processed'] somewhere to make sure that it is getting set and unset as you'd expect.
I have a feeling that if you do all of that it should become clear why you need to add the context twice to get it to work :)
Comment #26
thedavidmeister commentedThe session variable being checked before redirects seems to be causing this bug. Can't quite tell why it is there since context_redirect_validate_redirect() should handle avoiding infinite redirects.
Try this patch, works for me in a local dev site.
Comment #27
jorditr commentedGreat!!! David, your patch on #26 works for me. I'm building a big site and I'm setting the mobile part with "context mobile detect", "Context redirect" and "Context reaction theme". I was living with the double redirect context trick on #9 but your patch works for me without having to duplicate the context. Your patch looks neat and clever. Maybe it's not going to work on aggressive cache on, and I think that it would be enough on announcing it on the cache page as other module do :-)
Thanks again. Let's see if more people checks it...
Comment #28
thedavidmeister commentedRight... so if that patch works for you too I think we should pull that $_SESSION['context_redirect_processed'] variable out completely.
What it currently does is, if you go to a page where a redirect should trigger it sets that variable to true and performs the redirect. If you go to a page where a redirect should trigger and the variable was previously set, it unsets the variable and does not perform a redirect. Hence, adding two redirections on a single page will make the redirections start working consistently. The patch works by unconditionally unsetting that variable at the last moment before the redirection happens.
I *think* it's trying to work around the situation where if you have a redirect from X to Y and another from Y to X you'll end up with a redirect loop. While it does avoid the infinite redirect in that particular setup it also means that only every second redirection works :/
I personally feel that avoiding infinite redirections of this type should be the responsibility of site builders, not the module providing the redirection. If you guys agree I'll submit a patch that pulls that session variable out.
@JordiTR - there's some discussion about handling redirection for mobile devices in Drupal in this issue #681396: Boost and Mobile Tools if you're interested.
Comment #29
jorditr commentedDavid, I think you're working an interesting proposal and I would be interested in testing it. If you want to provide a patch I would be very pleased to test it.
Regarding on the Boost and Mobile tools discussion thanks a lot, despite the fact that Mobile Tools is getting outdated on Drupal 7 and has not been able to provide a working solution, maybe because a lack of resources to invest on the new platform. Personally I would say that if that "Context redirection" complement to Context would work consistently, together with "Context mobile detect" and "Context reaction theme" are a very proficient solution. I'm still working on seeing how to fit all the pieces work together (you see, I'm a big fan of Context and Rules!!! ;-)
Comment #30
hazah commented@thedavidmeister I think you're right on both counts, it looks to me that it's indeed for the case of page A redirecting to page B, where page B is redirecting to page A. I agree that it's probably a bit too much hand holding, and the "feature" is really turning out to be a bug for all other cases. That said, perhaps the way forward isn't to remove the variable, but to elaborate on it. My thinking is that instead of a boolean, it should be an array providing information about what triggered the redirect. Then we'd have a better execution context (the concept, not the data structure) to work from. I maybe overthinking this, but it seems like it could open more doors than shut.
Comment #31
thedavidmeister commentedI also thought about storing an array of sources but it isn't hard to come up with a situation where that is problematic too.
Say you have pages X, Y and Z. X redirects to Y on some session based variable, Y sets the session variable back to something else and redirects the user back to X. X, seeing the session variable being changed wants to redirect the user to Z.
If we restrict redirects based on an array the user will get stuck on the second visit to X and never be redirected to Z.
I was thinking if we wanted to go down this road we could keep track of all the *conditions* that we've met instead, as it's hard to imagine a scenario where a site builder would deliberately be redirecting users to a path where they want the exact same context reaction to fire again. As far as I can see that would always lead to a potential infinite redirect scenario.
I'll put that into a patch and you guys can say yay or nay.
Comment #32
thedavidmeister commentedPatch attached. Here we keep the session variable more or less intact, but we key the TRUEs by the name of the context that set them so that no individual context can fire a redirect twice in a single redirection chain. This won't pick up on any external methods of redirection, so if a context redirection interacts with a redirect being performed in a .htaccess config or similar to result in a loop we still can't see that, but it's better than nothing.
We unconditionally unset the session variable on every page load that doesn't result in a redirect through context so there's no cruft accidentally floating around for us to find later.
There's also a warning message displayed to site builders prompting them to fix any detected redirect loop.
Comment #33
thedavidmeister commentedComment #34
thedavidmeister commentedThis updated patch fixes a php notice when $_SESSION is not set for the patch in #32.
We have a new project owner, kevinquillen and I'm now a co-maintainer #1815454: Requesting Maintainership/Ownership of Context Redirect.
As soon as this patch is RTBC we can get it committed.
Thanks.
Comment #35
kevinquillen commentedI removed the $_SESSION use when addressing the other issue #1831312: Better documentation for context_redirect_goto() before I read this one, as I found the redirect bug happening too even in the most simple context setup (i.e. path is node/11, redirect to front).
It seems to me unlikely that you would have redirects going to pages causing more redirects. I've read through this thread but still unsure as to why use of $_SESSION checking is needed?
Comment #36
thedavidmeister commentedThe patch in #34 simply tracks which context conditions have already been met due to previous redirects. Since it uses the machine name of the context it's not susceptible to the type of issues I outlined in #31.
Having more than one redirect in a single page load is a perfectly valid thing to do.
I do think it makes sense to limit each reaction to only be able to redirect the user once per page load and setting/unsetting a single variable is pretty negligible overhead to achieve that.
Comment #37
thedavidmeister commentedWas thinking a bit more about this today. Realised that without the $_SESSION variable all it would take is two site-wide contexts with redirects as one of the reactions pointing to any two internal paths in order to break a site to a degree beyond that which a novice site builder could easily recover it.
Comment #38
thedavidmeister commentedbut then again $_SESSION isn't even really supposed to be used for anonymous users... so it wouldn't work anyway if it was being forcibly removed for performance reasons.
Comment #39
thedavidmeister commentedMaybe just bail on the $_SESSION stuff. This issue, specifically the issue of redirects not always being triggered is well and truly fixed by the recent refactor. I've been testing the latest 7.x-1.x
If someone really wants the extra safety net (that was never properly implemented in the first place) re-worked and re-instated and they have a concrete use-case that they can share with us they can open a separate issue and it can be dealt with there.
Comment #40
kevinquillen commentedIf there is a remote chance someone may make overlapping redirects with multiple Contexts, perhaps there could be a reports page that lists every context with redirect Reactions and where they lead to so they could narrow down where an issue may lie?
Comment #41
hazah commented@kevinquillen that's probably the best route. Perhaps that page should also completely ignore the redirect, just in case there is an infinite loop going. That way you can have a URL that is at least useful while the site is down.
Comment #42
thedavidmeister commentedIn the latest dev branch the watchdog messages provide a really good diagnostic tool for this kind of thing already. I'm not totally convinced that we have to roll a whole new UI for this, especially when nobody has actually put their hand up to say "This actually happened to me". It's all still hypothetical atm. We could add a few urls to the redirection blacklist to ensure that users can always get to the modules page to enable dblog in the least.
We can also add some information in the README.txt documenting that watchdog is currently the best way to diagnose redirection loops.
Comment #43
hazah commented@thedavidmeister, what do you mean when you say: 'especially when nobody has actually put their hand up to say "This actually happened to me".'?
The problem occures consistently. 50% of all redirects did not trigger. I am definately saying this had happened to me. Did I miss interpret?
Comment #44
kevinquillen commentedI think he meant overlapping contexts resulting in a redirect loop, not the issue of the context not always redirecting. Even I got that with a basic context (and no others setup). Removing the $_SESSION tracking eliminated that entirely for me.
Comment #45
hazah commentedAh thanks. Yeah, I did feel like I missed the point and wasn't really trusting the one I got out of it.
Comment #46
thedavidmeister commentedAlso, the overlapping redirects discussion is a separate issue. If you want to discuss it, go here #1853414: It is possible to create an infinite redirect loop with overlapping redirects, does the blacklist need to be extended?.