[D7] Browser Based Targeting

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxajesh2061283git

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, ajesh!

I've noticed that you have requested that I review your module. I am already reviewing 3 other modules. It is a lot of work to review modules!

As you may have noticed, no one has reviewed my module yet.

Given our similar predicament I decided to suggest that we help each other. I could review yet another module if you are kind enough to returned the favor.

My module is located at:
https://drupal.org/node/2059227

I will be waiting for your response.

I have already downloaded your project and it seems to be working fine.
I will need more time, though, to make sure that everything is fine.

With regards to my module being upgraded to version 7, I agree with you. But I will leave this for after my first project is approves. As you know, the approval of my project will depend on people like you. :)

You will hear from me tomorrow. Till then!

I tested your module in two Drupal installations and everything worked fine.

The only suggestion I would like to make is that you review the wording on the configuration tab. It is a little ambiguous. The interface is also somewhat counter intuitive.

The title says:

Show this block for these Browser

You could say, for example:

Show current block for the selected Browsers

You could set the Checkboxes as selected by default.

If you agree with my suggestion to set the Checkboxes as selected by default, you could change what is currently there at the bottom:

If no browser is selected, block will show regardless of selection.

You could type the following:

The current block will show for all the above selected browsers.

This is not a bug or error. It is just a suggestion that I think might improve usability.

Tell me what you think!

Hi, I tested this module in the five browsers provided, and it works splendidly!

The only criticism I can offer is that it would be nice to make the list of browsers configurable (say I want to add some specific mobile user-agents for example), and perhaps add some common mobile browsers to the list. With mobile targeting in-place, I could see this module being a major asset to site owners who want to tailor their mobile experience.

Code Review (through the coder module) detected the following minor, pedantic errors:

custom_detect.module

severity: minorreview: sniffer_commenting_filecomment_descriptionmissingLine 5: The third line in the file doc comment must contain a description and must not be indented [sniffer_commenting_filecomment_descriptionmissing]
*
severity: minorreview: sniffer_commenting_filecomment_spacingafterLine 6: File doc comments must be followed by a blank line. [sniffer_commenting_filecomment_spacingafter]
*/
severity: minorreview: sniffer_array_arrayLine 90: A comma should follow the last multiline array item. Found: 'delta' [sniffer_array_array]
'delta'
severity: minorreview: sniffer_whitespace_scopeindent_incorrectexactLine 94: Line indented incorrectly; expected 6 spaces, found 4 [sniffer_whitespace_scopeindent_incorrectexact]
$query->values(
severity: minorreview: sniffer_whitespace_scopeclosingbrace_indentLine 101: Closing brace indented incorrectly; expected 4 spaces, found 2 [sniffer_whitespace_scopeclosingbrace_indent]
}
severity: minorreview: sniffer_squiz_php_lowercasephpfunctions_calluppercaseLine 119: Calls to inbuilt PHP functions must be lowercase; expected "_custom_detetct_getbrowser" but found "_custom_detetct_getBrowser" [sniffer_squiz_php_lowercasephpfunctions_calluppercase]
$browser_data = _custom_detetct_getBrowser();

Overall, great module! I don't see any reason why this should not become a full project.

As I said before, the suggestions I made were only cosmetic.
I understand that you have a different point of view with regards to your module's interface.

In any other aspect your module is fine by me.

I will set is as OK.

Good luck with your full project application!

I'll look at this now in the Project applications sprint.

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

Possible Duplication

It seems that Browscap Block provides very similar functionality. Could you describe how your module differs? It seems like your module could be added as a feature to that module. Also, you are duplicating the browser detection code in Browscap.

We prefer collaboration over competition, therefore we want to prevent having duplicating modules on drupal.org. If the differences between these modules are not too fundamental for patching the existing one, we would love to see you joining forces and concentrate all power on enhancing one module. (If the existing module is abandoned, please think about taking it over).

If that fails for whatever reason please get back to us and set this back to "needs review".

That is the most serious problem, but there is still a full review below.

Master Branch

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

• Your function name has a typo, _custom_detetct_getbrowser() should be _custom_detect_getbrowser().
• You have some unused variables and other issues found here: http://pareview.sh/pareview/httpgitdrupalorgsandboxajesh2061283git.
• You have an extraneous 2061283 file in your repo (I'm not sure what file type m is).
• Your readme could be cleaned up and formatted a little nicer, and I don't think you need WARNING: DO READ THE INSTALL FILE when the user is already reading that file :)
• Remove ;Information added by Ajesh Prakash packaging script on 09/08/2013. from the .info file, Drupal will handle that for you.
• custom_detect_install() must be rewritten instead as hook_schema(). Then you can remove custom_detect_uninstall(). And we use braces {} around table names, not backticks `.

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

Don't forget to add the "PAReview: review bonus" tag as indicated in http://drupal.org/node/1975228 , otherwise you won't show up on my high priority list. (When you have completed 3 manual reviews and linked them in the issue summary)

Hi,

You still seems to be working on master branch in git repository; if not, remove it.
Moving from a master branch to a version branch .

I can also see some of the unused variables:
1) $description = ''; line #47 - file: custom_detect.module
2) $module line #48 - file: custom_detect.module
3) $delta line #49 - file: custom_detect.module
4) $block line #50 - file: custom_detect.module
5) $bname line #152 - file: custom_detect.module

Thanks

Hi,

Suggestion : Please do not mention your name everywhere in the module.

PLease view and remove these syntax errors :
http://pareview.sh/pareview/httpgitdrupalorgsandboxajesh2061283git

.module file
It would be better to use db_select() instead of db_query() so that queries would become compatible with different databases.

Thanks.

Correction: Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075

Hi Ajesh,
Nice work done.

Hi Ajesh

Same option on line no 62. Please remove.
Otherwise nice module.

Master branch still exist for this project and it is selected as default branch. Please delete that. Otherwise pareview.sh will provide review comments for master branch.

Also, I cloned 7.x-1.x-dev branch and found following issues after running coder module:

Line 55: Use an indent of 2 spaces, with no tabs [style_no_tabs]
	t("Opera") => t("Opera"),
severity: normalreview: style_no_tabsLine 57: Use an indent of 2 spaces, with no tabs [style_no_tabs]
	t("mobile")=>t("Display only in Mobile")
severity: normalreview: style_array_spacingLine 57: Arrays should be formatted with a space separating each element and assignment operator [style_array_spacing]
	t("mobile")=>t("Display only in Mobile")
severity: normalreview: style_no_tabsLine 116: Use an indent of 2 spaces, with no tabs [style_no_tabs]
	if($record->browser_detect == t('mobile')){
severity: normalreview: style_control_spacingLine 116: Control statements should have one space between the control keyword and opening parenthesis [style_control_spacing]
	if($record->browser_detect == t('mobile')){
severity: normalreview: style_paren_spacingLine 116: use a space between the closing parenthesis and the open bracket [style_paren_spacing]
	if($record->browser_detect == t('mobile')){
severity: normalreview: style_no_tabsLine 117: Use an indent of 2 spaces, with no tabs [style_no_tabs]
	 $mobile_detect_block = TRUE;
severity: normalreview: style_no_tabsLine 118: Use an indent of 2 spaces, with no tabs [style_no_tabs]
	}
severity: normalreview: style_comma_spacingLine 122: missing space after comma [style_comma_spacing]
    $mobile_detect = custom_mobile_device_detect(true,true,true,true,true,true,false,false,false);
severity: normalreview: style_function_spacingLine 223: Functions should be called with no spaces between the function name and opening parentheses [style_function_spacing]
function custom_mobile_device_detect ($iphone=true,$ipad=true,$android=true,$opera=true,$blackberry=true,$palm=true,$windows=true,$mobileredirect=false,$desktopredirect=false) { 
severity: normalreview: style_comma_spacingLine 223: missing space after comma [style_comma_spacing]
function custom_mobile_device_detect ($iphone=true,$ipad=true,$android=true,$opera=true,$blackberry=true,$palm=true,$windows=true,$mobileredirect=false,$desktopredirect=false) { 
severity: normalreview: style_uppercase_constantsLine 223: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE [style_uppercase_constants]
function custom_mobile_device_detect ($iphone=true,$ipad=true,$android=true,$opera=true,$blackberry=true,$palm=true,$windows=true,$mobileredirect=false,$desktopredirect=false) { 
severity: normalreview: style_uppercase_constantsLine 224: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE [style_uppercase_constants]
  $mobile_browser = false;

modify project description

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

I removed the automated review from the issue summary.

Removing review bonus tag, there are only 2 review comments listed in the issues summary. Please complete it with a third review.

Found permission which is never used.
Suggestion: You can add option Show this block for browser

• Is in
• Not in

IE
FF
Safari
etc

PHP have a function get_browser() to detect browsers. In addition Drupal have a module Browscap, with which you can obtain an improved version of the function.

In custom_detect_block_list_alter() is better to use the function get_browser() or browscap_get_browser() if you use Browscap. Then the function custom_detect_getbrowser() is not needed.

In addition, there may be a good idea to add an administrative page with the ability to add their own types of browsers? But I'm not sure about this idea.

@ajesh
On this side, you're right. Thanks for the explanation.

Hi Aajesh,

Thanks for this Module.

Hi Ajesh,

This module save my lots of works. We have some requirement like this.

Thanks
Debraj

review comment added

It appears you are working in the "7.x-1.x-dev" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
The following git branches do not match the release branch pattern, you should remove/rename them. See http://drupal.org/node/1015226

* 7.x-1.x-dev
  remotes/origin/7.x-1.x-dev
  remotes/origin/HEAD -> origin/7.x-1.x-dev

Review of the 7.x-1.x-dev branch:

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

• custom_detect_mobile_device(): the switch() statement does not match our coding standards, you should use ":" after cases.

This sounds like a feature that should live in the existing browscap project, or at least it should be based/dependent on the existing browscap code. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the PROJECT_NAME issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

fixed link

I think you are missing the point - your module *should* depend on browscap. Why? because browscap is already really good at figuring out which browser someone is using.

So instead of duplicating the same code in both modules, you can build on top of browscap.

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

Do not RTBC your own application, see https://drupal.org/node/532400

manual review:

1. if you must duplicate the code then you should add similar modules to the project page and describe the differences to them, so that users can make an educated choice.
2. The problem is that custom_detect_getbrowser() has a lot of custom detection code that is now duplicated. So if a bug is found it has to be fixed in two modules instead of one. The number of modules can be high on a site, but that fact alone does not make it slow. It depends on how heavy those modules are. The browsecap module for example is very light weight and does not add any heavy hook implementations at all, so it will not slow your site down at all.
3. The project title should be "Custom Detect", so that it matches to module machine name custom_detect

However, it is your decision to duplicate code, even if it is against common community interest. Since this was RTBC already and I don't see any other blockers ...

Thanks for your contribution, ajesh!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.