False/true not converted to 0/1 during browscap.ini import

The patch fixed the problem for me!

Related I believe: #1861832: browscap_ctools_browscap_access_ctools_access_check returns false on mobile-devices

Patched against 7.x-2.0, uninstalled , reinstalled and panels are now detecting mobile browsers properly with browscap_ctools module (important - full uninstall rather than disable/enable required to trigger hook_schema; otherwise, not certain if simple browscap DB update will cause old data to be properly replaced in the browscap table).

EDIT: Rather than uninstalling/reinstalling browscap, the module itself can be used to import a new dataset:
Modules>Browscap>Refresh Browscap Data File then Save Configuration.

Thanks to IEFBR14 for the tip (here).

@mondrake: Thanks for the patch; it worked exactly as described.

Does anyone know if this will be committed to the next update of the module?

Andrew

Patch #1 fixed the issue for me as well.

#1 implemented and fixed my issues

+1 for the patch in #1

Please note that this impacts other modules that work together with Browscap. I'm using the Statistics Filter module, which has a Browscap integration but without this patch the statistics_filter_is_crawler function considers all traffic to be a crawler.

I tried to repeat the problem but I don't see the bug. It would be super helpful to have a simpletest for this problem so that people can confirm the problem is fixed and doesn't come back.

I did create a regexpal test with current code and the proposed pattern.

It does seem like we should make this change, but it would be good to know specifically an example bug this causes so I can see before/after. I am working with php 5.3.

Since this is still present in the dev version that is the best version for the issue.

Since it's not 100% clear what bug this fixes marking back to needs review.

OK, so I tested this some more and think it is a critical bug that is indeed RTBC. I updated the original post to make it more clear.

I think it's worth committing this fix now to get it into the dev package at least. http://drupalcode.org/project/browscap.git/commit/b09a91f

Backports to 7.x-1.x and 6.x-2.x and 6.x-1.x are welcome.

Did you intend to change the version from 7.x-2.x to 7.x-1.x?

@jlea I did because the code is committed and I believe the bug is now fixed in 7.x-2.x-dev. It's now "to be ported" to the previous branch (I don't plan to work on that, but someone else could and I would commit it).

providing more clarity on the problem.

There is no supported release for the 7.x-1.x branch, so a port to it is no longer needed. Please correct if wrong.

Reproduced problem in 6.x-2.x and confirmed patch in #1 fixes. Patch applies as is.

Thanks for the research, AohRveTPV - committed with credit to you and mondrake.

FYI: I had to modify the code on my patch after updating from browscap 6.x-2.0 to 6.x-2.1. I was updating a site which I had previously patched successfully with this patch on 6.x-2.0, but browscap wasn't working properly.

It's worth noting that my import.inc also has a few other changes to compensate for our firewall, but I don't think that is relevant to the problem I was having. The problem I had on my site was that all browsers were being detected as mobile browsers.

After comparing the databases on my live site and my testing site, I believe the schema changed when transitioning from tempdownloads.browserscap.com to www.browscap.org. True and False are now reported with quotes around them. The previous regular expression was only good for unquoted True and False after an equal sign.
I changed the following lines of the patch to import.inc to account for optional quotes. Maybe someone more familiar with this can convert it to a proper patch of the aforementioned patch:
Change old:

    $browscap_data = preg_replace(
      array(
        "/=\s*true\s*\n/",
        "/=\s*false\s*\n/",
      ),
      array(
        "=1\n",
        "=0\n",
      ),
      $browscap_data
    );

to

    $browscap_data = preg_replace(
      array(
        "/=[\s\"]*true[\s\"]*\n/",
        "/=[\s\"]*false[\s\"]*\n/",
      ),
      array(
        "=1\n",
        "=0\n",
      ),
      $browscap_data
    );

It's rudimentary, but it worked for me. I also had to temporarily decrement local_version after line 15 to force it to download the same file again ($local_version--; #uncomment this line to force download)

Re-opening per #19. I have not tried to reproduce the problem yet.

Also changing to normal severity because I do not immediately see how this would be critical.

For anyone who is wondering, I would say that it's only as critical as how you use true/false data from browscap. In essence, the data format change puts us back to the original problem that started this thread (which was normal severity as well). Sites will be impacted to various degrees from zero to critical.

In my case, it was semi-critical because it completely altered our site's appearance for visitors using non-mobile browsers, reverting to a very basic theme.

Thanks for contributing your time and checking on this.

"Critical" does not seem appropriate to me per the descriptions of the priority levels:
https://www.drupal.org/node/45111

However I see how it could be considered critical for a given site if some important functionality depends on the testing a TRUE/FALSE property. Upgrading to "Major".

Looks like this also affects 7.x-2.x.

7.x-2.x patch I have tested and it seems to convert all "true" and "false". 6.x-2.x is currently untested.

I changed the regular expression a bit versus suggested in #19 to be more specific. I don't think we need to handle multiple consecutive double quotes, spaces within the double quotes, etc.

Please review/test.

Made a minor simplification to the parameters to preg_replace().

Patches in #23 newly used single quotes for one preg_replace() parameter and double quotes for the other. These patches consistently use single quotes for both.

#24 works for me on 7.x.

#24 works for me on 7x too.

Was wrong to set this RTBC. You're not supposed to set your own patch to RTBC.

#24 didn't work for me. Seems to be possibly removing all the EOLs. I believe you need to maintain the \n characters, which require the double-quote. Here is the modified code from #24 that works for me:

    $browscap_data = preg_replace(
      array(
        "/=\s*\"?true\"?\s*$/m",
        "/=\s*\"?false\"?\s*$/m",
      ),
      array(
        "=1\n",
        "=0\n",
      ),
      $browscap_data
    );