[D7] ReachEdge

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

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.

Hey, there are some feedback from my end..

First of all please change your module folder name to match your rest of the module file's name.

Another thing is that you have used two variables here at
line 104: reachedge_pages
and at
line 103: reachedge_visibility_pages

but i don't see you have set them anywhere, then what is the need to store that fixed value in database.

Although this one is minor but still, at line 24: Title is not wrapped up in t()

I'm not positive what you mean by "change your module folder name to match the module's file name" can you explain?

Well by that I only meant that your project application git clone command should be like this: git clone --branch 7.x-1.x https://git.drupal.org/sandbox/REDrupalPlugin/2675668.git reachedge instead of git clone --branch 7.x-1.x https://git.drupal.org/sandbox/REDrupalPlugin/2675668.git

Also, If I try to use t on line 24, I get phpcs tells me not to use t() in hook_menu()

Yes its not required in hook_menu(), translation of title and decription in hook_menu are handled internally.

Well hello REDP! I used to work with Loni at ReachLocal. I will go ahead and review :D

OK, here's my review:

Code

The structure is very good overall. While the individual files are small enough to all fit into one .module file without being overly cluttered, I like how it's broken out with the admin.inc and variable.inc files. The functions also follow namespace conventions, and all functions have some Doxygen documentation. Great stuff!

Security

In reachedge.module I saw $_GET['q'] a few times; these should probably be wrapped in a check_plain() function or similar. EDIT: check_plain() is only necessary when data is printed to HTML, which is not the case here.

Also, I saw a call to php_eval(). Is it really needed? That said, I see that it evaluates a constant that is defined on an admin page.

Other Comments

Having worked for ReachLocal, I know that 'http://go.reachlocal.com/contact-us-edge.html' in README.txt is one of RL's marketing pages. Of course, I know that going through that page is the only way to get ReachEdge, but it would be nice to explain first what ReachEdge is, followed by explaining how the module is a bridge to install ReachEdge on Drupal sites. In other words, the wording in the first paragraph could be a little clearer, but otherwise the README file is well written.

Changing to 'needs work' until we can get an explanation or fix for the security items.

Ah, just caught this; In your hook_menu, the title and description should be wrapped in the t() function.

Hi @REDrupalPlugin!

Thanks for contributing your module.

I've just confirmed you as a new user - Welcome to the Drupal.org community :-)

Glad to see you've had a couple of reviews now - would you be willing to help some others get off the starting line too and review some other modules? Once you've reviewed three other projects you're eligible for the Project Application Review Bonus, which will put you in the fast track lane to approval.

Thanks again - and good luck!

cheers
Donna

@REDrupalPlugin - Have you done any other module reviews yet?

Make sure you link to them in the summary here and add the PARreview bonus tag when you have to get more eyeballs on your module.

More info on the review bonus system is here: https://www.drupal.org/node/1975228

Pareview reports that there are two README files and @author unnecessarily used in documentation.

Apparently one extra line (line 9 actually) was left behind in reachedge.module.

10 | ERROR | [x] Additional blank lines found at end of doc comment

What site id should I use for testing? I can get a free trial at http://go.reachlocal.com/naus-offer-edge-free-trial.html but they require my contact info including email. Is there a way to test this without giving my email address?

A couple of suggestions: on admin/config/system/reachedge you could use placeholder instead of default value, like so:
'#attributes' => array('placeholder' => 'XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX'),

Also increase the size slightly so the user can see the end of the placeholder. (The end doesn't really need to be seen, but it's nicer that way.) '#size' => 38,

After entering the site id and going to some other page, I get a long pop-up in German!: "Wir erheben, verarbeiten und nutzen bestimmte personenbezogene Daten von Ihnen...". Finnish could've been expected (I live in Finland) but German is highly unexpected ;)

089242925016 gets changed to 08994396959 (not the one you said) with a delay varying from a fraction of a second to a couple of seconds, which could be confusing to a visitor.

STRING REPLACEMENT was not changed.

I see stuff happening at developer tools, so I guess that part is working, and it doesn't depend on Drupal anyway.

@REDrupalPlugin - To speed up approval, you'll need to post your reviews of other projects to the issue summary.

The Git admins are only approving applications that have the review bonus.

For info on the review bonus please see: #1975228: Review bonus

Here's some new applications that need review:
* #2702713: [D7] Duplicate Images
* #2700917: [D7] Views Results Inject
* #2624458: [D8] Simple Analytics

@REDrupalPlugin, would you mind sharing why you chose not to use the placeholder?

I think you should revise the Description section of the README.txt file. You have now seven links to the same contact address. Also I'd prefer reading about how this Drupal module works, instead of what looks like a sales pitch for the service. A brief explanation about the service and a link to it would suffice in my opinion. Following that, a description of what this Drupal module does.

At the Requirements section of the README.txt you could mention how to get the ID.

Also at reachedge_menu and reachedge_variable_group_info I'd prefer having a description about those particular menu items instead of the boilerplate description about the service "ReachEdge offers lead & call tracking...".

@kari.kaariainen - that sounds like back to needs work?

What's the story with the placeholder? Do you really prefer a default value that the user has to delete?

Tested. Works as advertised. Found no problems. No errors from pareview.sh.

Removed 2 automated review links from the issue summary.

manual review:

1. reachedge_page_alter(): you are not altering the page, you are just adding stuff to it. Use hook_page_build() instead.
2. reachedge_page_alter(): Since you have $page available you should not use drupal_add_js() but rather #attached on the render array. See https://api.drupal.org/api/drupal/developer--topics--forms_api_reference...
3. _reachedge_visibility_pages(): @return do is missing, see https://www.drupal.org/coding-standards/docs#return
4. _reachedge_visibility_pages(): the check_plain() calls are useless here since you are not printing data to HTML. I think that was wrong advice from laboratory.mike above, sorry about that. You only need to sanitize data for XSS when printing to HTML. Make sure to read https://www.drupal.org/node/28984 again.

But otherwise looks good to me.

Thanks for your contribution, Mark!

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.