D7 port of browscap

Can this be ported? My module: Browser Theme Settings requires this module. I cannot port my module to D7 until this happens.

Yes, please make a Drupal 7 port of this module.

Adding one more request for a D7 version.

Subscribing.

Small patch against the downloadable version in #1

Sign for an official Version for Drupal 7

Better patch. Resolves an "Unknown function" fatal error and an "Undefined index" notice.

Subscribing

My Mobile Tools needs a D7 version of this module in order to work correctly. When is the new Drupal 7 version of this module expected to be ready?

@devildogmrk -- have you tested the patch by the original submitter and in #8?

• If you have tested the patch, and find that it fails to work for you, please describe the failure in detail. Better yet, provide an improved patch.
• If you have tested the patch and it works, change the status to "Reviewed and Tested By the Community".
• If you have not tested the patch, do not change the status.

Removing useless tags.

Here is a re-rolled patch against the 7.x-1.x branch at git://git.drupalcode.org/project/browscap.git

The patch in #13 works for me... (remember that was a checkout from git, and NOT the 6.x branch in CVS)

cd sites/all/modules
git clone git://git.drupalcode.org/project/browscap.git browscap
cd browscap
git checkout 7.x-1.x
patch -p0 browscap_2.patch

My next item will be to test this by plugging the 7.x version into context so that I can have a context based on browser versions (iphone/android specifically)

@pillarsdotnet - please don't RTBC your own code.

@himerus, thanks for the test and report. Would you say you tested/reviewed all the changes or just that it works for your use case?

It is a pretty simple module so if it mostly works then we could probably commit and make a dev release to aid further testing, but I would like to have some confidence in it. The site where I use browscap is on 6.x for a long time in the future so it's hard for me to test this out.

Now, a review:

This looks to me like it is both a port and some fixups, which makes it harder to commit.

I don't understand your goal with the change of browscap_update_6000 and browscap_update_7000. Why would we need to set a version? Especially if it is going to mean people will skip a change made in update 6000?

+/**
+ * Implements hook_uninstall().
+ */

That should be in both versions of the module.

-      $browserstring = substr(trim($browser['parent']), 0, 255);
+      $browserstring = empty($browser['parent']) ? '' : substr(trim($browser['parent']), 0, 255);

This looks like a notice fix, so it should be in 6.x first.

I didn't review the changes of moving the page callbacks to includes/admin.inc b/c that's somewhat hard to do and with these reviews I've run out of time. I'd prefer if you could actually straight port those leaving them in the .module file and then after we upgrade it we can create a new issue for breaking it out which would make it all much easier to review.

Seems close, but marking needs work for now.

@greggles

My review was a quick patch and install on my D7 dev site for Omega... My goal is using it for selecting mobile browsers, and using alternative theme settings (Delta module) via context for those mobile browers.

My initial test of this patch/D7 version was just installing, and seeing that everything seems to be working properly on all the reporting pages. It wasn't really in depth, and I'm looking into the update for mobile_tools to see if it can integrate w/context (#1046244: Integrate with Context conditions & reactions)

That is the main reason I didn't mark as RTBC... but I do think it could be "ready" for the D7 dev branch at the minimum.

Right ... so git d7 branch is CVS HEAD?

@sime -- no; it's a separate branch. CVS HEAD is the same as git master.

Re-rolled patch. The reports are working for me, now.

Another re-roll, backing out the addition of includes/admin.inc and restoring some of the comments that were deleted.

Restored another bit of code that had been inadvertently removed...

More comment change reversal. This is probably as small a patch I can make without adversely affecting functionality.

Found a few more...

By the way, I also had to patch core to make this work. See #1055276: Configurable chunk size in drupal_http_request()

@pillarsdotnet - OK, so the branch doesn't exist in CVS at all. I thought git was still being nuked periodically and replaced with CVS but I guess we've moved on from that!

subscribe

Updated patch fixes handling of is_crawler field. MySQL chokes when storing "true" or "false" in an integer field.

Also corrects a poorly-worded comment.

Reroll after the Great Git Migration.

Is there any way to make a 7.x-dev release, patched, on drupal.org ? This is getting fuzzy with all the patches...

I agree with creating a branch for 7.x. Unfortunately this patch still modifies more than I would like it to.

This changes some required permissions. We should note that in README.txt installation notes both for completeness and as a helper to people who are upgrading.

That's the only thing that really "needs work." The code seems to work properly.

These changes are relevant bugfixes for the 6.x branch as well.

-    'title' => t('Browsers'),
+    'title' => 'Browsers',

I've created that as a separate patch in #1077366: Don't translate menu titles nor descriptions - could someone review it?

Moving this to a form seems like a great idea for both branches.
- '!refresh' => url('admin/settings/browscap/refresh'),

I don't have the time for that now and don't think it should hold up creating the patch, but it's an example of how not to port a module.

Removing the unused functions browscap_boolean and browscap_unmonitor seem like great ideas to discuss in their own issues.

@greggles:

So if I understand you correctly, you are unwilling to create a D7 version that incorporates any advantages over the D6 version.

We would have to submit issues to patch the D6 version to bring it up to par with the proposed D7 port, and only then submit an issue to port to D7.

Is that correct?

I mean no offense nor disagreement. I only ask for clarification because core Drupal development happens in exactly the opposite manner.

Sorry for not being more clear.

The only thing that needs to be fixed before I commit this is adding documentation to the README.txt about the proper permissions for D7.

The rest of these things frustrate me as a maintainer who wants two high quality and similar branches but I'm not going to hold back a d7 release on them. I'm happy to get improvements however they come, but happier if they are done in a way that makes it easy to update both branches.

It also makes the review job for this upgrade patch easier since there is less code to review specifically for the upgrade.

The attached patch is as minimal as I can get, without breaking code.

It will not apply as is; but requires and depends on patches in these issues:

#1077366: Don't translate menu titles nor descriptions
#1077622: Correct errors in the downloaded browscap file
#1077650: Move refresh function to settings page.

EDIT: Note that the README.txt is modified to document the permissions change.

And once again, a combined patch against the contents of git branch 7.x-1.x

Bah. Typo.
Trying again:

Awesome. Thanks pillarsdotnet and pasqualle - http://drupal.org/commitlog/commit/890/7c0931e5703bd28fc46779214948b87e7...

As soon as packaging is done it will be published at http://drupal.org/node/1077948

Reopening... What's the status? Can we get a dev release ?

There already is a dev release for the commit greggles did in #37!