at_get_browser broken [#1898936]

Hi,

The function at_get_browser(), which the mobile blocks/regions feature depends on, does not seem to be compatible with the current return format of the browscap_get_browser() function it relies on.

As it is now, in adaptivetheme 7.x-3.1, it does:

$browser = browscap_get_browser();
if ($browser['ismobiledevice'] == 1) {
  $is_mobile = TRUE;
}

But browscap 7.x-2.0 seem to set $browser['ismobiledevice'] to a string value 'true' rather than an actual boolean value.

Since you are checking if it's equal to 1, not if it's true, the expression returns false.

Proposed solution will be attached as patch.

Comment	File	Size	Author
#2	adaptivetheme-at_get_browser_broken-1898936-2.patch	511 bytes	sigveio

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

sigveio CreditAttribution: sigveio commented 25 January 2013 at 15:32

Status:

Active

» Closed (fixed)

Actually, never mind. Upon preparing the patch, and grabbing the latest git version, I see you've already changed it to:

if ($browscap['ismobiledevice'] == TRUE) {
  // ...
}

Which would have been my proposed solution. In other words; if anyone else experience 7.x-3.1 mobile functionality being borked... download the dev version.

Comment #2

sigveio CreditAttribution: sigveio commented 25 January 2013 at 16:36

Status:

Closed (fixed)

» Needs review

File	Size
adaptivetheme-at_get_browser_broken-1898936-2.patch	511 bytes

On a closer look (I should have done this sooner, but... well... it's been a long week) browscap 7.x-2.0 also seems to return 'false' as a string when it is not a mobile browser. Unfortunately this means that the updated expression in your dev version would also behave unexpectedly... because PHP does not cast 'false' to the boolean false. Unlike for an example the string '0'.

There's a few ways this could be handled if you need to keep backwards compatibility for an earlier version which returned 1 or 0 instead of the current string format. The following method would achieve that, while making it more robust against changes made to browscap in the future:

if (filter_var($browscap['ismobiledevice'], FILTER_VALIDATE_BOOLEAN) == TRUE){
  // ...
}

The FILTER_VALIDATE_BOOLEAN filter means that it'll return TRUE for "1", "true", "on" and "yes". It'll return FALSE for all other values.

If you wanted to check more explicitly for TRUE/FALSE you could add the flag FILTER_NULL_ON_FAILURE to have it only return FALSE on "0", "false", "off", "no", and "" - and NULL for all "non-boolean" values.

php > var_dump(filter_var('true', FILTER_VALIDATE_BOOLEAN));
bool(true)

php > var_dump(filter_var('false', FILTER_VALIDATE_BOOLEAN));
bool(false)

php > var_dump(filter_var(1, FILTER_VALIDATE_BOOLEAN));
bool(true)

php > var_dump(filter_var(0, FILTER_VALIDATE_BOOLEAN));
bool(false)

php > var_dump(filter_var(true, FILTER_VALIDATE_BOOLEAN));
bool(true)

php > var_dump(filter_var(false, FILTER_VALIDATE_BOOLEAN));
bool(false)

To my knowledge there are no significant performance hits in using filter_var this way, but it will mean a dependency of PHP 5.2 or later.

Patch attached.

Comment #2.0

sigveio CreditAttribution: sigveio commented 25 January 2013 at 16:36

Issue summary:

View changes

Removed junk

Comment #3

Jeff Burnz CreditAttribution: Jeff Burnz commented 28 January 2013 at 12:19

Version:

7.x-3.1

» 7.x-3.x-dev

I'm going to test with this solution, I have no issues depending on PHP 5.2.

Comment #4

Jeff Burnz CreditAttribution: Jeff Burnz commented 13 March 2013 at 19:45

Status:

Needs review

» Fixed

OK, I committed this, hooray, I will call this fixed.

Some of my clients urgently required this so I have kind of run out of patience waiting for Browscap to fix its own issues: #1868808: False/true not converted to 0/1 during browscap.ini import

Comment #5

sigveio CreditAttribution: sigveio commented 13 March 2013 at 20:36

Great! :)

(PS: It's no biggie, but git author attribution would have been nice. :P Every patch, even as small as this one, helps.. and people like being given credit where credit is due. You can find a ready formated command for the commit in peoples profiles, in case you were not aware. :-))