search

silverstripe-archive / silverstripe-mobile

Mobile support module for SilverStripe CMS
http://silverstripe.org/mobile/
BSD 3-Clause "New" or "Revised" License
35 stars 36 forks source link

Cookies aren't working as intended. #35

Closed kinglozzer closed 10 years ago

kinglozzer commented 11 years ago

In MobileSiteControllerExtension.php, where the cookies are being set, the third argument being passed to Cookie::set() is time() + self::$cookie_expire_time.

Cookie::set() takes it's parameters in days. This means that the default $cookie_expire_time of 1800 actually attempts to set a cookie to expire in around 3.7 million years. I'd like to think my site will still be around then, but for now it's causing a warning about setting an expiry year that's higher than 9999.

As far as I can see, the expiry date for Cookie::set() has always been in days, so I don't understand how this hasn't been spotted & addressed before now.

Am I being incredibly stupid?

wilr commented 11 years ago

As the message on the static says, 1800 is seconds so probably just an assumption that was wrong about Cookie::set() so patches welcome. Would need to clarify days in the comment.

kinglozzer commented 11 years ago

I'm happy to submit a patch for it. The existing 30 minutes would be hard to accomplish when specifying the amount in days. The nearest I can reasonably get is 0.03125 days, which would give a 45 minute expiry time. Unless you're happy for the static to be changed to, for example, 1 day instead of 30 minutes.

wilr commented 11 years ago

1 day seems reasonable.

On 23/01/2013, at 10:16 PM, Loz Calver notifications@github.com wrote:

I'm happy to submit a patch for it. The existing 30 minutes would be hard to accomplish when specifying the amount in days. The nearest I can reasonably get is 0.03125 days, which would give a 45 minute expiry time. Unless you're happy for the static to be changed to, for example, 1 day instead of 30 minutes.

— Reply to this email directly or view it on GitHub.

kinglozzer commented 11 years ago

Have submitted a pull request for this. While investigating, I noticed that line 44 of MobileSiteControllerExtension.php, $domain = $config->FullSiteDomainNormalized;, can never actually return an empty variable, hence making the if statement that follows it redundant.

Will submit another pull request for it.

kinglozzer commented 10 years ago

Closed via 9ca250e1eba3778fe8c12471c4f4ac7785a4977f