[D7] Browscap UABlock

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi Breidert,

I reviewed your module

Could you please remove the unwanted images from module.(This image already present in the your sandbox project link).

When install this module, It throws the warning.Due to missed the browsecap.ini file.we have to handle even in the warning itself.

Available OS filter is empty in the module configuration.

Please refer the attached image for your reference.

Regards,
-Raj.

Hi,

I reviewed your module:

Installation works fine
Removal too, everything gets cleaned up
Automated code review passed (http://ventral.org/pareview/httpgitdrupalorgsandboxbreidert2004078git)

Some improvement suggestions:
- I think it should be possible to default to none on the filter configuration page
- it would be nice if the filters would be checked against the avaliable filters on the configuration page (hint or warning), like its done on the block configuration (there only filters with a match show up).

best,
netkaiser

Hi

I just did a cursory review
- in the .module file, at line 47

if (!empty($cache_browsers) && ($cache_browsers->created > REQUEST_TIME - 60 * 60 * 24) &&
      !empty($cache_oss) && ($cache_oss->created > REQUEST_TIME - 60 * 60 * 24)) {

but at line 168

if (!empty($cache_browser_options) && ($cache_browser_options->created > REQUEST_TIME - 60 * 60 * 24) &&
      !empty($cache_browser_options) && ($cache_browser_options->created > REQUEST_TIME - 60 * 60 * 24)) {

Ok for me about the line 47, but how about line 168? I think it's the same as line 47
And they're duplicating so I think you should refactor the code, create a new function and put your condition in the new function. and you should assign the "REQUEST_TIME - 60 * 60 * 24" to a variable so you don't have to calculate it multiple times


Just one more small question, do you think that we should use the cache to store the list of the passed blocks instead of calculating all the possibilities in every request?


Thanks for your contribution

Hello,
Great module! Worked fine for me.
Only there is still a master branch, according to pareview.sw online tool,

http://ventral.org/pareview/httpgitdrupalorgsandboxbreidert2004078git

here https://drupal.org/node/1127732 follow step 6.
cheers!

Part 1: Manual Code Review

/**
 * Implements hook_init().
 */
function browscap_uablock_init() {
  // Add css for hiding blocks and styling the config interface.
  drupal_add_css(drupal_get_path('module', 'browscap_uablock') . '/browscap_uablock.css');
}

You are add css file on all pages, including those for which it is not used. Add css only when it is actually used.

    if (empty($result)) {
      drupal_set_message("Module browscap did not return any 'user detection information'.
        Please check installation and configuration of <a href='/admin/config/system/browscap'>browscap</a>.", 'error');
      watchdog('browscap_uablock', "Module browscap did not return any 'user detection information'.
        Please check installation and configuration of <a href='/admin/config/system/browscap'>browscap</a>.", NULL, WATCHDOG_ERROR);
    }

Use t() function for messages.

Part 2: Manual Testing

I created a new block and added filters. Block disappeared from the page, but he stayed in the markup of the page. Hide blocks means the css is not correct, you have to hide block pattern during the rendering of the page

Sorry for my english

Code is clean after automatic review indeed [http://ventral.org/pareview/httpgitdrupalorgsandboxbreidert2004078git].
Didn't find any errors/issues in manual review.
Regarding @zestaio's review:
1) I think it's ok to add css to every page
2) minor issue, but you should really use t()

As for testing, works fine, tested on different OS's and browsers.
A few opinions - on setting form:
- it would be nice to have some hierarchy in available OS/browser listing
- in filters an autocomplete option would help

Because of only minor issues, I'm setting status to reviewed and tested.

First off, I like the idea of this module!

You should remove the .gitignore file and the master branch from the repo.
Don't create new globals if at all possible, any bare PHP code like that will be executed on every Drupal page request.
You seem to be using hook_init() to load css. These will be loaded on every Drupal page request, and they should be moved to only load where they are actually needed.
The doc comment for browscap_uablock_settings_form() isn't right, you're not implementing hook_form() (which is form defining a node edit form for a content type your module has declared), it's just defining a new settings form.

Where you are retrieving cached data do you need the time checks? I think cache_get() will already do that for you. Or it will if you add the expire parameter to your cache_set() call.

if (!empty($cache_browsers) && ($cache_browsers->created > $_request_time) &&
    !empty($cache_oss) && ($cache_oss->created > $_request_time)) {
      $browser_filters = $cache_browsers->data;
      $os_filters = $cache_oss->data;

I'm not sure I agree with the general approach. Instead of 2 additional tables, can you use hook_schema_alter() to add a new column to the blocks table? That seems like a much more logical place to store the visibility settings.

For showing or not showing a particular block, wouldn't it be better to use one of the hook_block_xxx_alter() functions to prevent the block from being rendered instead of just css hiding it?

If you feel strongly about the approach, I will review just the code again and lay off :) But some of the initial problems should be fixed.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

Removed breidert@ from git clone snippet