D7 port of browscap

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.

Login or register to post comments

#12

Posted by pillarsdotnet on January 19, 2011 at 4:02pm

Removing useless tags.

Login or register to post comments

Attachment	Size
browscap.patch	27.13 KB

#14

Posted by himerus on January 31, 2011 at 1:09pm

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)

Login or register to post comments

#15

Posted by pillarsdotnet on January 31, 2011 at 2:01pm

Status:

needs review

» reviewed & tested by the community

Login or register to post comments

#16

Posted by greggles on January 31, 2011 at 2:57pm

Status:

reviewed & tested by the community

» needs work

@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?

<?php
+/**
+ * Implements hook_uninstall().
+ */
?>

That should be in both versions of the module.

<?php
-      $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.

Login or register to post comments

#17

Posted by himerus on January 31, 2011 at 3:02pm

@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 (D7))

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.

Login or register to post comments

#18

Posted by Pasqualle on January 31, 2011 at 5:54pm

re: browscap_update_7000()
see #431940: Update errors
sometimes you need to know the user's db schema to apply your updates correctly. I found this as a good solution for this problem. If you later realize that you need to create an upgrade path from D6 to D7 then you can change browscap_update_7000(). New D7 installs, users who are using the D7 dev version already will not be affected by the change.

It is not possible to upgrade from D5 to D7 directly, so browscap_update_6000() is useless in D7 release.

Login or register to post comments

#19

Posted by sime on February 8, 2011 at 1:07am

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

Login or register to post comments

#20

Posted by pillarsdotnet on February 8, 2011 at 5:11pm

Status:

needs work

» needs review

@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.

Attachment	Size
browscap.patch	27.63 KB

Login or register to post comments

#21

Posted by pillarsdotnet on February 8, 2011 at 5:38pm

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

Attachment	Size
browscap.patch	23.54 KB

Login or register to post comments

#22

Posted by pillarsdotnet on February 8, 2011 at 5:41pm

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

Attachment	Size
browscap.patch	23.21 KB

Login or register to post comments

#23

Posted by pillarsdotnet on February 8, 2011 at 5:48pm

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

Attachment	Size
browscap.patch	22.86 KB

Login or register to post comments

#24

Posted by pillarsdotnet on February 8, 2011 at 6:25pm

Found a few more...

Attachment	Size
browscap.patch	22.42 KB

Login or register to post comments

#25

Posted by pillarsdotnet on February 8, 2011 at 6:34pm

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

Login or register to post comments

#26

Posted by sime on February 8, 2011 at 10:32pm

@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!

Login or register to post comments

#27

Posted by nunof on February 9, 2011 at 4:03pm

subscribe

Login or register to post comments

#28

Posted by pillarsdotnet on February 20, 2011 at 3:31pm

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.

Attachment	Size
browscap.patch	22.61 KB

Login or register to post comments

#29

Posted by pillarsdotnet on February 26, 2011 at 12:59am

Reroll after the Great Git Migration.

Attachment	Size
modules_browscap.patch	22.6 KB

Login or register to post comments

#30

Posted by SebCorbin on March 1, 2011 at 11:13am

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

Login or register to post comments

#31

Posted by greggles on March 1, 2011 at 3:28pm

Category:	support request	» task
Priority:	critical	» normal
Status:	needs review	» needs work

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.

<?php
-    '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.

<?php
-                     '!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.

Login or register to post comments

#32

Posted by pillarsdotnet on March 1, 2011 at 5:44pm

@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.

Login or register to post comments

#33

Posted by greggles on March 1, 2011 at 6:26pm

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.