[D7] Openlayers Heatmaps

welcome

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• README.txt is missing, see the guidelines for in-project documentation.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See http://ventral.org/pareview/httpgitdrupalorgsandboxpol1381108git.

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. Go and review some other project applications, so we can get back to yours sooner.

I'm not shure about the .make file, should it be pushed or gitignored?

switch ($api) {
      case 'openlayers_layers':
        return array('version' => 1);

      case 'openlayers_behaviors':
        return array('version' => 1);
    }

Without break; it will fall through anyway, so you can change it to

switch ($api) {
      case 'openlayers_layers':
      case 'openlayers_behaviors':
        return array('version' => 1);
    }

please don't implement hooks if they're not doing anything

/**
 * Implements hook_uninstall().
 */
function openlayers_heatmaps_uninstall() {
}

    // TODO: Use library module.

it would be great if you implement this now ;)

Your module has not many lines of code, but regarding your ablility to use ctools, library (soon ;) ) and openlayers I think you really taking care of what your doing..

I like it and I think this will get through pretty fast

regards

ah, okay I thought .make files are only for profiles but this sounds interesting

You still got some few coding standart issues http://ventral.org/pareview/httpgitdrupalorgsandboxpol1381108git
you can use this site to re-test your self

It appears that there have been multiple project applications opened under your username:

Openlayers Heatmaps: http://drupal.org/node/1381494
I18N Selection Page: http://drupal.org/node/1273954

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue.

With this in mind, I have marked your secondary applications as 'closed(duplicate)', and left one application open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one I that I have left open, then please feel free to close the 'open' application as a duplicate, and re-open one of the other project applications which I had closed.

Thanks in advance for your patience and understanding!

You have to close two of the applications. After that you could get a review as soon as possible of one application. Now you opened two of them for need review. Shall we keep this one open and close the two others?

Sorry, one more application. I18N Selection Page is now closed.

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732

Automatic review, some small errors left:

FOUND 7 ERROR(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
24 | ERROR | Class name must begin with a capital letter
24 | ERROR | Class name must use UpperCamel naming without underscores
28 | ERROR | Method name "openlayers_heatmaps_behavior::options_init" is not
| | in lowerCamel format, it must not contain underscores
37 | ERROR | Method name "openlayers_heatmaps_behavior::options_form" is not
| | in lowerCamel format, it must not contain underscores
70 | ERROR | Array indentation error, expected 8 spaces but found 12
77 | ERROR | Array indentation error, expected 8 spaces but found 12
84 | ERROR | Array indentation error, expected 8 spaces but found 12

If the Openlayers project uses it, I see no problem in using it in modules that are built upon Openlayers. It should be strange to change that. Somebody else that have an opinion on this?

Hi, I'm one of the OL maintainers. His class and methods are named how they should be.

OL code can be a bit dated so thoses conventions were not enforced from the start. Everyone making OL behaviors (as well as layers and styles) write class and methods lowercase, geofield and openlayers_plus to name a few.

Looking really good actually :)

Thanks for your input @nod_ - it should be great if you have to opportunity to test the this module out - because of your deep knowledge on the Openlayers module, and report back here.

openlayer-wise it's fine. It's the JS that need a bit more cleanup.

• undeclared var heatmap leaking into global scope
• missing {} for one if ()

other than that it's fine :)

It is.

A manual review is also needed, hopefully me or somebody else could do this as soon as possible.

Did a quick manual review, in openlayers_plugin-folder you have a js-file callede openlayers_heatmap_behavior.js, I think it should be better if you called it openlayers_heatmaps_behavior.js. Else I could not find anything that should stop this for becoming a full project.

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732

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. Get a review bonus and we will come back to your application sooner.

manual review:

• openlayers_heatmaps_requirements(): no need to check for the libraries module, as you already depend on it in the info file.
• README.txt should mention a URL where to download heatmap.js
• "'<br/>(' . $layer->name . ': ' . $layer->description . ')',": you should sanitize those variables before printing them to avoid XSS attacks.

Looks good to me.

blockers

project page

Please take a moment to make your project page follow tips for a great project page.

form element validation

The form in openlayers_heatmaps_behavior.inc, options_form, does not filter or validate the values entering into the textfields. They look like they want to be restricted to integers but are currently open to any input ( . If there is a full form validation function called, it can be done that way, or the #element_validate should be able to be used otherwise. There is already an integer validation function around somewhere. The checkbox doesn't need additional validation.

non-blockers

project package

The Open Layers module and most other things related to it have the package set to "OpenLayers" in the .info file so that they are grouped together on the modules page. At present, it will be in the default "Other" section.

git commit messages and attribution

Please refer to Commit messages - providing history and credit about giving yourself some credit and properly formatting commit messages.

Those bits are fixed and it looks good.

Don't forget about Commit messages - providing history and credit

Hi Pol,

Thanks for your contribution and welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects, at your discretion.

Now that you've experienced the full review process, please consider reviewing other projects that are still awaiting review. Anyone can help with reviews, following the guidelines.