I installed this module, configured it with the URL for my mobile site and tested it on a mobile device (iPhone). It worked properly. However, if I tried to go to the full site again, it didn't redirect unless I cleared my mobile browser's cookies.

Is this the way it is suppose to work? I thought it would only disable the redirect if you added ?nomobi=true to the URL (which I didn't do).

Files: 
CommentFileSizeAuthor
#16 redirect_rewrite-1619732-16.patch19.54 KBtyphonius
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch redirect_rewrite-1619732-16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 redirect_rewrite-1619732-13.patch19.27 KBtyphonius
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch redirect_rewrite-1619732-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 oneTimeRedirectFix-1619732-12.patch9.32 KBjnicola
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in oneTimeRedirectFix-1619732-12.patch.
[ View ]
#11 addSessionFunctionality-1619732-10.patch10.86 KBjnicola
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in addSessionFunctionality-1619732-10.patch.
[ View ]

Comments

soulston’s picture

FOR DRUPAL 7
This was the case for me when I was redirecting to a path rather than a url:

Doesn't work:
redirection to /path

Works:
redirection to http://m.mydomain.com

I then used themekey to switch the theme based on the cookie (system:cookie = smr_redirect=1).

Maybe this is the issue that you are having with D6

nfriend’s picture

This same thing is happening for me using 7x-1.1. I am using the full http.://m.domainname.com and for both Android and IPhone it only works first time.

I clear the phone cache and it works again.

Any tips?

Thanks

Neil

aaronott’s picture

Category:support» feature

soulston, nfriend,

This is the way the module is designed to work. I'm thinking about adding the ability to select whether this is a one-time redirect or a redirect every time.

As I see it, redirects could fall into a couple different categories:
* Once - one time only redirect
* Once per session - each time the user comes back to the site
* Once per cookie_expire - A variable timeout that would be set in the cookie expire field.

Please let me know your thoughts on this as I know there are many more use cases than what I use this module for.

As this is actually new functionality, I'm switching this to a feature request.

djmantra’s picture

Aaronnott,

Thanks for creating this simple clean easy to use plugin. Yes please keep the users on mobile site until they click view full site link/?nomobi=true

This gives users a seamless user experience. Say if I saw a nice mobile site and I want to show it to someone and then it doesn't redirect me to mobile anymore unless I clear cookies and cache.

I have seen other plugins like mobilejoomla do it the way I mentioned.

Thanks

webadpro’s picture

well, i think the best way would be.. always stay on the mobile version site no mater what. and let the module create a switching block. Mobile to classic and visversa.

aaronott’s picture

webadpro,

I think you are looking for functionality that this module is not meant to provide. If you are looking for a module that will detect your device and display a mobile version of your site, you might look at http://drupal.org/project/mobile_tools

This module is meant to provide a quick way to detect the device you are on, and based on that device, redirect you to another page or a page on an external site. Use case for this would be for a promotion of a product for a given device. Perhaps you have an app for download at the Android Market place, it would not make sense to send an iPhone there but it would make sense to send an Android phone there.

webadpro’s picture

I see.

Well I finally wrote the code to do what i needed using the mobile switch module. You can view the patch here: http://drupal.org/node/1691304#comment-6370002

Greg J. Smith’s picture

As I see it, redirects could fall into a couple different categories:
* Once - one time only redirect
* Once per session - each time the user comes back to the site
* Once per cookie_expire - A variable timeout that would be set in the cookie expire field.

Yes – please implement this as that would make this module much more useful (for me anyways).

jnicola’s picture

I'm running into a similar issue in D6.

Reviewing the code, it looks like it firsts looks to see if the user has been redirected before by checking cookies (line 65). If they have, don't redirect. This avoids looping as your comment states... but... it also means once they've ever been redirected once, they aren't redirected again, so at that point the return to the mobile home page having a cookie to not return them to mobile seems redundant... since they've already got a cookie saying not to redirect.

I wonder what it would take to run this based on session versus instead of based entirely on cookies... hmmm...

I think I'm going to spend my night working on the code for this. I got nothing else going on!

jnicola’s picture

Welp, I tried... I've spent a ton of hours on this, and I finally stalled out. i thought i had a solution... but then I realized I had just mimic'd your cookie results. This is all a bit mind boggling... but here goes...

$_SESSION relies on cookies. HOOK_BOOT occurs before a session is started and HEADER(LOCATION:) is sent out before cookies get brought in... yet through setcookie and getcookie() in PHP you can still get the values... so you exist in a grey area in which $_SESSION can't exist because cookies are created yet, however you have PHP get cookies for you...

Drupal 7 has drupal_session_start(), which will forcefully start the Drupal session. I see no equivalent in 6 to force start the session. Even more fun, there are limited functions that can be called from inside of _BOOT, and none of them include anything from _session.inc, at least not per my reading.

So... I tried, and I have no solution in Drupal 6 after 3-4 hours of effort. I'll give this another hour as the solution is theoretically in sight... but my google-fu isn't helping my find a way to start a session in _BOOT.

jnicola’s picture

StatusFileSize
new10.86 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in addSessionFunctionality-1619732-10.patch.
[ View ]

Switching from _BOOT to _INIT now has $_SESSION existing and DPM() available to me and I can still forward and move around, but I can't create persistent mobile forwarding based on $_SESSION here either, despite my variables stick around. I'm rather stumped...

For fun I attached a patch based on what I've done. Don't expect this to solve much though... it doesn't do the trick.

This has been a fun exercise in learning more about Drupal though, so not entirely a night lost... just not a lot of ground gained on this module or my current projects issues!

EDIT: Don't use this patch. I took a very different direction below, as you can't really make this work at INIT effectively, and in boot $_SESSION simply doesn't exist. What is in that patch doesn't work anyways it was just for people to check out and maybe help me construct into something functional!

jnicola’s picture

Status:Active» Needs review
StatusFileSize
new9.32 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in oneTimeRedirectFix-1619732-12.patch.
[ View ]

WOOHOO! I got it!

The problem (as I view it): Visitors to the site are only redirected internally to the mobile site ONCE. Subsequent visits don't take them to the internal site.

The Cause of the Problem as I see it: The cookie smr_redirect wasn't passed an expiration value, so in theory as long as the cookie exists, this would always evaluate as true. This value is used to prevent looping, but in the end it prevented users from subsequently getting forwarded ever again on later visits to the website.

The Solution I created: I added a module setting that can be adjusted in the modules admin page for how many seconds the smr_redirect cookie should expire at. A little bit of code adjusting here, some logic adjustments there, and it worked! I set my value to 60 seconds, and sure enough it works.

Why make this an adjustable setting? Because it's ultimately your call on what works and doesn't work for your clients and average visitation, etc etc. Here's why: With 60 seconds, I could in theory visit my a website on my cell, hang up to get a call from a friend, get back on my phone and navigate to the website (50 seconds or so now having passed) and since 60 seconds isn't up I'd not be redirected...

This introduced another problem, which I resolve as follows: Creating the above solution created a new problem however... users to your mobile site would be redirected again to the mobile site in 60 seconds, even if they persisted in the mobile site. Rut row!

I came up with a solution, and it is based on the presumption you will be sending your users to a subdirectory such as "mobile" and in theory that's where your mobile theme/content exists and will reside. Therefore, I wrote code to check the $_GET['Q'] (requested URL) against the simple mobile redirect URL, and if the current URL contains the simple redirect URL, do not forward as we can safely assume the visitor is already in the mobile specific section.

Added Bonus While working on this: All of the $user_agent Case seemed really out of place in the logic for when to redirect and what not. It has it's place, but it's a pretty significant chunk, so I broke it out into it's own function (getMobileRedirect()) which really cleans up the code for redirecting, and instead of setting $simplemobileredirect, it just returns the same value it would have set before, so what used to be 60 ish lines, now just reads $simplemobileredirect = getMobileRedirect();

typhonius’s picture

Title:Only Redirecting Once» Only Redirecting Once [Patch attached]
Version:6.x-1.4» 7.x-1.1
Component:Miscellaneous» Code
StatusFileSize
new19.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch redirect_rewrite-1619732-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Hi - myself and rli have taken a look at this and created a fairly major patch that addresses the issue of redirecting users more than once. I was tempted to recategorise this as a bug report as one could argue that having the module work once every 30 days per device couldn't be classed as 'working'.

The patch applies against 7.x-1.x-dev and the following now works:

  • Mobile users will get redirected to the mobile site regardless of how many times they visit the non-mobile site.
  • Non-mobile users will get redirected to the non-mobile site regardless of how many times they visit the mobile site
  • The patch respects the settings in the admin interface relating to how the user should be redirected:
    • Redirect each time will do such
    • Redirect per session (whilst the same browser window is open) is yet to be written by the module maintainer
    • Redirect per timeframe sets the user's cookie to the timeframe specified (1 day, 1 week etc)
  • Mobile users are allowed to view the desktop site through the use of the nomobi=true query parameter
  • Mobile users who desire to go back to the mobile site after being on the non-mobile site can browse to /clearsimplemobileredirect as before to have their non-mobile cookie removed and be re-evaluated by the module as a mobile user.

Caveats

To make this work smoothly and without redirect loops I have engaged the use of the $cookie_domain global. I can't envisage a situation where this would cause issues with the module as all mobile sites I've used have been on the same cookie domain as the non-mobile site.
ie m.example.com or mobile.example.com instead of example.com.

I am confident enough with this patch to place it here for review and potential incorporation into the simple mobile redirect module, however if the maintainer feels strongly that this patch has no place in a simple mobile redirect module then I believe this patch can be used to fork the module into another module - less simple mobile redirect.

jnicola’s picture

Hmmm... we've got two different patches to solve the same problem.

Anyways, I'm in need of this functionality on a Drupal 7 site I am working on, so I'm going to roll your patch and see how it works.

I don't see a dev version of this module though... so not sure what version you checked this against. I'll go against 7.x-1.1 and see how it goes.

jnicola’s picture

Off hand observations thus far:

Your description in admin settings says the following:

" Once per timeframe - Currently once per 60 days.
Once per session - Once until the user closes the browser.
Every Time - Like it says, each and every visit to your site, this user will be redirected. (Use with Caution may cause looping)"

With the patch I wrote for 6.x-1.1, you can custom set the timeframe, it might be nice to add that functionality!

Once per session is nice functionality. I couldn't pull it off since you can't start session from _boot in D6 but i saw that it may be possible in D7. Did you start the session or move to _init ?

Every time possibly resulting in looping... I wrote functionality to avoid this in the patch I submitted for 6.x. , although I forget the specific. I believe I checked if the redirect path is included in the RequestURI, and if so, don't redirect. This presumes people sending users into /mobile or /ipad however, and may contain flaws... but i feel it's ballpark ish :)

I'll continue testing now using "once per session".

typhonius’s picture

StatusFileSize
new19.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch redirect_rewrite-1619732-16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Once per session isn't actually in use - it's just dev code written by the module maintainer. I found the dev version of the module with building blocks of this functionality in the git repo for the module.

I've since written the updated patch (attached) that allows users to be redirected with the correct path too. Still no per session functionality but I'm sure it can be done.

jnicola’s picture

So what exact functionality did you add and what exactly doesn't work so I know what I'm dealing with in regards to your patch. I don't have a clear understanding.

typhonius’s picture

Essentially what I wrote in #13.

Two sites: example.com and m.example.com. (Or whatever sites specified)

Per 'time period' setting:
On first visit users are set with a cookie.
mobile user will keep getting redirected from example.com to m.example.com regardless of which site they try to navigate to - this was the issue this patch tries to address. Similarly non-mobile user will be redirected from whichever site to the non-mobile site.

The two paths from the original module are still in effect and mobile users can use those paths to alter their cookie such that they are able to access the non-mobile site and then go back again.

Per session setting:
Not in use but skeleton code provided by the module maintainer in the dev release

Each time setting
No cookies set. Users evaluated on each page load and sent to relevant site.

I filled in code for 'each time' because it was pretty easy although the best option to use would be 'per time period' which sets the cookie for mobile/non mobile for 30 days. It would be trivial to add the option for the admin user to set the cookie period but I have not done that as yet since it was unnecessary for the task I needed it for.

If you have further questions I'm on the IRC often and can answer q's there!

Rory’s picture

Status:Needs review» Needs work

@typhonius - your patch shouldn't contain white-space changes. I'd advise you submit all your white space changes in a separate patch for review. Distinguish your functionality changes for this issue.

From general patch guidelines: http://drupal.org/node/707484

To help reviewers understand the scope of changes, separate each change type into its own patch. For example, bug fixes, performance enhancements, code style fixes, and whitespace fixes all should be in different patches. Each separate change type (and patch) should be associated with a different issue in the queue on Drupal.org.

typhonius’s picture

Status:Needs work» Needs review

Thanks for your review Rory. I think minor whitespace changes that happened as a result of creating this patch are inconsequential to the focus of the patch and the review process. If people (and especially the module maintainer) are accepting of the logic of this patch then I (or another contributor) can reroll with whitespace changes removed prior to it being committed..

There'll need to be another change to this patch from personal experience - so I'll note it here for personal reference:
If the schema changes it can lead to redirect loops.

Rory’s picture

Firstly, sorry to quote the Drupal documentation, but I'm only trying to help. It's not exactly clear what you meant by the last sentence, and you shouldn't note something for personal reference in an issue.

Secondly I don't mean to harp on a point but a module maintainer's life should be made easy.

In an initial assessment of your patch, I couldn't separate what was whitespace and what was functionality. When a module maintainer or anyone else intending to review this patch goes to apply your patch and wants to monitor what specifically changed and what didn't, it's difficult for them otherwise.

It needs to be apparent what in your patch is the result of a whitespace fix and what is specific as a change appropriate to this issue.

Feel free to change the issue status back (I'm not seeking any kind of personal victory here) but I recommend you take my advice, as then both would probably be more speedily processed.

typhonius’s picture

I understand it's all about making the maintainer's life easy and agree with you there :)

Perhaps I am confused when you talk about whitespace changes - are you talking about the odd extra line that has been added eg

+      $location = $base_url . '/' . $location;
+    }
+  }
+  // << Extra line here
   if ($persistence == REDIRECT_ONCE_PER_TIMEFRAME) {
-    $expire = REQUEST_TIME + 60 * 60 * 24 * 30;
+    // If the module is set to have a cookie then set it to expire per the admin menu
+    $expire = is_null($location) ? REQUEST_TIME - 3600 : REQUEST_TIME + variable_get('simple_mobile_redirect_expire', 2592000);

The personal note was something I felt useful to either myself when I get back to the issue or if another user wishes to have a go at rerolling or has issues with the patch. Perhaps I could have phrased it better though so it didn't seem like I was using the issue queue to 'note to self' :)

Steve -cc’s picture

Hi
I just installed this module on a site and have found what I think is the same problem that this issue is addressing: That the module works perfectly once and redirects a mobile device visitor to the mobile site specified but that subsequently if the same visitor on the same device reloads the site the regular site is loaded and the visitor is not automatically redirected to the mobile site.

In my settings I have:
for non-mobile site: / - (the default setting)
and for the various Redirect Device: http://m.mysite.com

Is this correct?

I see the instructions at the top of the settings page:
Link back to /?nomobi=true to set a cookie to stay on the full version of the site.
Link to /clearsimplemobileredirect to clear the cookie and redirect to Mobile sites.

But I am not sure where to use these instructions - in the non-mobile site: settings? or?

I have not tried the patches listed above - yet
Any help in moving forward on this would be greatly appreciated.

Steve

Status:Needs review» Needs work

The last submitted patch, redirect_rewrite-1619732-16.patch, failed testing.

typhonius’s picture

Steve -cc you're absolutely correct. The issue you're experiencing is what motivated me to write the patch in this thread!

I see the instructions at the top of the settings page:
Link back to /?nomobi=true to set a cookie to stay on the full version of the site.
Link to /clearsimplemobileredirect to clear the cookie and redirect to Mobile sites.

But I am not sure where to use these instructions - in the non-mobile site: settings? or?

You don't alter these, you navigate to http://mysite.com/clearsimplemobileredirect or http://mysite.com?nomobi=true to have the above effects on the browser.

Steve -cc’s picture

Thanks for the reply typhonius

So I think you are saying that if I apply a patch that this problem will go away? or give me some more settings to adjust?

Which of the patches above are you referring to? Does the patch work for the D6 module?

Steve

Steve -cc’s picture

Thanks for the reply typhonius

About the Link back and link to instructions at the top of the settings page:

You say: You don't alter these, you navigate to http://mysite.com/clearsimplemobileredirect or http://mysite.com?nomobi=true to have the above effects on the browser.

Sorry about being a bit dense but . . . does a site visitor need to click one of these links? or?

Steve

typhonius’s picture

My latest patch in #16 should be applied against the 7.x branch of the module.

It should mitigate the problem so the user will be evaluated on each page rather than just once per 30 days.

Yes, the visitor should click the link (in the same way that some mobile sites have a 'View Full Site' link.

mforbes’s picture

Status:Needs work» Needs review

#13: redirect_rewrite-1619732-13.patch queued for re-testing.

typhonius’s picture

If you're going to use a patch - test and use #16 as it's more up to date.

Shane Birley’s picture

@typhonius & @aaronott -- I believe this module is very powerful and seems to fill a void where Themekey, Browscap, Mobile Detect all seem to fall down. They can do the theme loading bit but all lack a method of switching back and forth without freaking out the cache system, causing loops, or finding one's self sitting in a bar, drinking beer and eating tasteless peanuts wondering how to solve this very problem.

Is there a chance of a newer release with Mobile Detect/Browscap integration?

typhonius’s picture

Shane Birley: I feel the most important thing is to get this patch (or a revised one to include the improved functionality this patch provides) into the simple mobile redirect module prior to actually getting additional integration.

Shane Birley’s picture

@typhonius -- Agreed. Who is available on this project to commit/review the patch?

Shane Birley’s picture

I must be attempting to patch the wrong set.

patching file simple_mobile_redirect.admin.inc
Hunk #1 succeeded at 7 with fuzz 2 (offset -1 lines).
Hunk #2 FAILED at 19.
1 out of 2 hunks FAILED -- saving rejects to file simple_mobile_redirect.admin.inc.rej
patching file simple_mobile_redirect.info
Hunk #1 succeeded at 2 with fuzz 1.
patching file simple_mobile_redirect.module
Hunk #1 FAILED at 45.
Hunk #2 FAILED at 81.
2 out of 2 hunks FAILED -- saving rejects to file simple_mobile_redirect.module.rej
M

EDIT: Yah, I did. Forgot to patch it against 7.x-dev -- duh. So, kids, don't do what I did above or the mail man will turn out to be your father.

Shane Birley’s picture

I have successfully attempted the patch and everything was patched fine but now I am getting issues with caching and unending loops.

https://drupal.org/node/974526

typhonius’s picture

Is it doing an http <-> https redirect? I didn't build schema observance into the patch yet - perhaps I will do that at some point?

It may also be an option to clear all your cookies and try again.

Shane Birley’s picture

@typhonius --

Hi! I don't believe it was bouncing off https:// but I will double check. Caching and cookies were dumped a few times and I tried it in a couple of other browsers and same result. I am going to review my set up and then try again.

smscotten’s picture

Status:Needs review» Needs work

There is no -dev release listed at https://drupal.org/node/910180/release or https://drupal.org/project/simple_mobile_redirect/git-instructions and the patches provided in this thread are not against any version that seems to be available by web or git. I'd be willing to help work on this but it would be bad for me to duplicate the work that has been done and I also can't work from the work that has already been done.

Options?

typhonius’s picture

This patch should be based off of HEAD so try and apply the patch to the latest version downloaded using
git clone --branch 7.x-1.x http://git.drupal.org/project/simple_mobile_redirect.git

kazaa’s picture

Issue summary:View changes

Is it any new solution to fix this problem? If someone refresh or open again in mobile device, drupal site appear in normal mode - not mobile link.