dSero Anti AdBlock for Google AdSense

Hello ! =)

Manually review of your module.

I am not an expert, below is what i found:

- You have only a master branch. You have to create a new branch according to the Drupal version of your module (here, 7.x-1.x), then remove the master branch. See http://drupal.org/node/1659588 for more informations.
- You must always end each file with a new line http://drupal.org/coding-standards
- Do not use indent tabs. Indents must be 2 spaces. Also, you have to remove whitespaces at the end of each line of your code.
- Remove CVS $Id tag, there are not required any more
- Juste a notice: i am confused about the name of your module, it this dSero or dsero_anti_adblock_for_google_adsense ? Maybe, you need to rename your files according to the name of your project.
- You should use hook_init() instead of hook_install() for adding your script.
- Why there is an standalone php file ? I can't see the link between your module file and this php file.
- Line 212 & 216, i think that function_exists() is not required here: drupal_set_html_head() and drupal_add_js() are core functions.
- Secund notice: const is not required in Drupal 7 for constants (but it is in Drupal 8).
- You could use drupal_json_decode() instead of json_decode(). Same, for urlencode().
- I think you should separe the admin part from dsero.module into a new file like dsero.admin.inc
- I have the impression that the user can decide on which block ads appears, but the js script is added on all pages.

I run also an automated test which reveals some of the errors i reported.

Hope that helps :)

Hi,

I'm looking at the code and I see the following:

1. You added functions inside the .info file. Those functions should go inside .module or .install files

2. When you define a class the file should be referenced on the .info file

3. On line 154 of .module the title should be inside the t() function

4. You are executing code directly on the module file, from line 234 to the end of the file. I've worked with some modules until now and I've never seen this. I'm quite sure you can find hooks to execute that code at the right moment.

5. line 236. drupal_set_html_head is a Drupal 6 function.
Your module is Drupal 7, do you need to test if that function exists ?

I guess parts of this modules are the same as in the Wordpress module and still need to be adapted.

Nice module by the way.

forgot to change status

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Hi dSero, you should add a PAReview: review bonus tag if you apply for a review bonus. Otherwise the administrator won't find your issue.
You can read again the review bonus process for any question regarding the process:
http://drupal.org/node/1410826

1. Find the one on that doesn't belong to this list:

• Dashboard (View and customize your dashboard.)
• Content (Administer content and comments.)
• Structure (Administer blocks, content types, menus, etc.)
• Appearance (Select and configure your themes.)
• People (Manage user accounts, roles, and permissions.)
• Modules (Extend site functionality.)
• Configuration (Administer settings.)
• dSero (View AdBlock Analytics and enable dSero Ad Recovery.)
• Reports (View reports, updates, and errors.)
• Help

I think that it is too intrusive to put this module as a root item of the administration menu.
Imagine that everyone put its own modules at the root level...
I'm not sure if this can be a reason to block the release process but in any case I personnally found that quite annoying. Please, can you put this link a more adapted place like as child of configuration root item ?

2. Line 145. dsero_anti_adblock_for_google_adsense_get_dashboard_path()
This is even more intrusive than the previous one and I think this requires modifications.
This function sends the administrator email address as a $_GET param to an external website without its consent.
This is something I personally don't want to see on a GPL free CMS... there are a lot of privative software on the market already.
Why don't add a secured Drupal form with a submit button instead ?

Beside that the code is ok, it follows the Drupal standards.

changing status

Line 42, 92
drupal_http_request() is the preferred method over file_get_contents()

Also, I'm not sure I understand the reason behind using the DseroCache class to store all of the variables and constants.

Hi Moshe,

You have not answered to my review on #11.
What is your opinion about those the two points ?

#11: You associate the email address of the administrator with a hashed key and store it at dSero database automatically without any consent from the user. That's a privacy violation !

If this is not happening, can you give a proof of that ?

If you see other modules that work with 3th platforms as Mollom or Google Analytics, the user has to go the the main platform and give his email address to obtain the API key then introduce it on his module configuration page.

They don't use the CMS API to send your personal data without consent.
I know this is very tempting, but please don't.

What you do here is not legal on 90% of countries...
Changing status and added tag.

Concerning the # param.

Officially this is meant to stay on client side that's TRUE....

BUT: your iframe is executing a function called qs() that parses the url params (also the #) and send it with an Ajax call to a php script called adblocker.statistics.php !
With this code you are giving yourself the temptation to store private information and it's as easy as adding this line on your javascript file (hosted outside of drupal repository) :
data.e = qsParm['e'];

See loadPage() function line 30 of js/adblock-report.js

1. From my point of view sending private information as URL params is a very bad practice.
2. I would never use a module that send my information away without asking me.
3. The basic philosophy behind GNU/GPL project is to keep software free and fight against privative software, which between other practices, stores users information without permission.
4. Drupal has a grate Form API that can build the form you show inside that iframe without risk of sending the administrator email to an external server.

This is the script on your server that receives the # params:
adblocker.statistics.php

If you have no plans to store administration emails, why don't add this form insde the drupal module without calling an iframe ?

Concerning the menu item on the tree root

I don't think this is a reason to block the review of this module.
I just think is a wrong choice for your project as people usually like to have the items in a good semantic order.

Concerning Wordpress

Wordpress has no validation for contributed plugins/themes submissions and is quite common to find bugs, intrusive code, base64 encrypted code and many other surprises inside...

I'm asking to a git administrator what should be done on this case.
I think my task is done here as I analysed the code and pointed the facts so I will stop following this issue and go to other projects so I don't block this process more.

As you have a review bonus she/he should answer to this point.

Regards,
Sergio

I understand you are really impatient to validate your work but please don't set the RTBC status by your own.

At comment #17 point 2 there was an obvious try to hide the way your code works.

Looking at the JS file that this module loads inside the iframe we can see that you are parsing the # params and sending values (the administrator email address?) to your server without asking first. See comment #18

You have a PAReview: review bonus and I've sent an email to a git administrator so he can check your module and decide by himself.

If it was my decision I wouldn't approve this module without removing the iframe and moving the module from the admin menu root level.

Dear Moshe,

Your code is not the problem here.
I reviewed this project three times and I'm just trying to advice you the best that I can.

I don't think I would be the only person that will find aggressive to discover that the User1 email is sent using an iframe and parsed with a js code without the user consent.

This could not only be disappointing for some users, but also become a legal problem to your company.

As said before, is my advice.

Regards,

Sergio

Hm, there seems to be no GPL licensing violation here, all files appear to be genuine. The javascript file logicdesign was referring to seems to be gone as well, so I see no problem continuing this.

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

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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:

1. project shortname "dsero_anti_adblock_for_google_adsense" is too long, is that really necessary? Why not simply "dsero"?
2. project page is too long. Once this gets promoted to a full project you should move information to dedicated documentation pages.
3. @file doc block is the same in all files. Please replace it with actual useful documentation what the specific file is about.
4. I agree that the class DseroCache is quite useless and does not make sense. Please remove it and just use standard constants.
5. "!defined('_DSERO') or die;": never use die() in the global scope. Why do you need that line?
6. "const _DSERO = TRUE;": const can only be used since PHP 5.3 as replacement for define(). As Drupal 7 should also work on PHP 5.2 I recommend to use define() or add the PHP 5.3 dependency to the info file.
7. dsero_anti_adblock_for_google_adsense_generate_site_code(): why should there be an exception during variable_set() or drupal_http_request()? Both functions do not throw exceptions, so your try/catch blocks are useless.
8. dsero_anti_adblock_for_google_adsense_menu(): do not use t() in hook_menu(), the items will be translated later.
9. dsero_anti_adblock_for_google_adsense_block(): $form is undefined.
10. "t('Failed to update API key - ') . t($status)": do not concatenate translatable strings, use placeholders with t() instead.
11. dsero_anti_adblock_for_google_adsense_init(): why do you need hook_init()? This will be executed on every single page request and is considered expensive for performance. You should only add the javascript when your ad is actually displayed.
12. dsero_anti_adblock_for_google_adsense_block(): hook_block() does not exist in Drupal 7.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

manual review:

1. dsero_anti_adblock_for_google_adsense.module: "$_dsero_anti_adblock_for_google_adsense_blocking_path = '';": do not define standard variables in the global scope like that, they have no effect.
2. dsero_anti_adblock_for_google_adsense_should_refresh(): why do you need global variables? variable_get() is cached in memory anyway.
3. dsero_anti_adblock_for_google_adsense_refresh_code_from_remote(): "dsero_anti_adblock_for_google_adsense_should_refresh() == FALSE": that function never returns FALSE?
4. I'm heavily confused by the global variables and the usage of variable_set(). Why do you set those system variables when they are overwritten on any page request anyway?
5. dsero_anti_adblock_for_google_adsense_should_refresh(): does nothing more than generating a random number. dsero_anti_adblock_for_google_adsense_refresh_code_from_remote() does nothing more than issuing a drupal_http_request(). That could all be just one function that is called from hook_init(). No need for globals, no need for variable_set().
6. $_dsero_anti_adblock_for_google_adsense_site_private_code is also not necessary is global variable, it is saved as system variable anyway.
7. You are installing the site code on hook_install(). No need to do it again in hook_init() and hook_enable().
8. Overall there is a lot of verbose, confusing code that does not do much. I think this project is too short to approve you as git vetted user. However, we can promote this single project manually to a full project for you.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

No need to call me "Mr. Purer", makes me feel old. klausi is fine :-)

Looks a lot cleaner now and seems RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

No objections for more than a week, so ...

Thanks for your contribution, dSero!

I promoted this to a full project: http://drupal.org/project/dsero_anti_adblock_for_google_adsense

Now that this experimental project has been promoted, you'll need to update the URL of your remote repository or reclone it.

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