[D7] Indicators

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

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.

http://pareview.sh gives really bad result for this project. Fix it first.

http://pareview.sh/pareview/httpgitdrupalorgsandboxec-entr2304245git

Line number 524 has defined PHP generic class as $source_file = new StdClass() instead of $source_file = new stdClass()(Mixed capital case) in the file indicators_import.data.admin.inc

Please first fix the errors as recommended by @zaporylie and remove any debugging code, i.e. dpm().

http://pareview.sh/pareview/httpgitdrupalorgsandboxec-entr2304245git

EDIT: removed long pareview.sh dump.

Hi EC-ENTR,

- At line 1368 of indicators_query.module you have to use placeholder and remove variables inside the queries. Look at db_query page for more informations

- You should use dpm only for testing your module and remove all system messages from the sites for security reasons.

- All constant and variables defined by your module should be prefixed with a module name to avoid conflict with other modules

- Please remove the EC_LICENSE.txt file. Drupal will add the appropriate version automatically during packaging so your repository should not include it. Read the licencing requirements

- In info file you need a specific version of php?

- Is better using arg() than $_GET in indicators_query.module

- In line 706 you catch a database exception error and print the error with drupal_set_message.
You should register this error on watchdog and remove drupal_set_message that expose part of your db to the frontend/backend users.

First of all - it seems you are using a non-individual account. User accounts are intended for personal use by an individual.
Please note that organization accounts cannot be approved for git commit access. See https://www.drupal.org/terms for details on what is/isn't allowed.

If you have question about licence - please ask licensing working group https://www.drupal.org/governance/licensing-working-group, although I believe you cannot publish code on drupal.org (or create any drupal-related code) with non-GPL licence.

dpm() is called only at design phase when the programmer activates the debug mode. At all other times dpm() is never called. Is there any other way to display debug info to the programmer?

Before you enter the project application process, you should first make sure that your project is a release candidate (rc). That means you shouldn't release code which is still in "active development mode". And if you think your code is release-ready just remove all debug-related code.

If you want to speed-up reviewing process, please join review bonus program.

The answer to #2510036: Possibility of using EUPL v1.1 or later is no. You cannot license module code as EUPL in a sandbox or promoted module.

I've opened #2514084: Remove EC_LICENSE.txt.

So not only is including EUPL code blocking, you risk having your git access revoked if you don't fix the licensing.

If you want to change the project's structure so that some part of it is considered a 3rd party library maintained on GitHub or another repo using EUPL, you could ask the LWG to confirm the license compatibility for the library. We'd then have to review the GPLv3 quirks in that license.

But for module code, this is very clear. The license for what is committed MUST be GPLv2 and later. What is packaged for downloaded MUST be GPLv2.

I would say you should still remove all instances of dpm() from codebase or, at least, move it to separate module with devel as a dependency. Check commerce_devel for best practices.

And please remember that you cannot commit as EC-ENTR. You have to use your own, personal account instead.

As I wrote before, if you want to get review from security team you should join review bonus program.

Hi EC-GROW,

1) Lots of coder issue in your project.
Install coder module, check your module with coder and fix those errors.

2) In README.txt line no. 41
It should be /admin/modules instead of /admin/build/modules

Could it be that this application has been off the radar lately for some reason?

Thank you in advance for looking once more into it or telling us what would be missing!

Thank you for your time!

I'm out of date on project application requirements, but noticed this when doing a quick review:

_indicators_term_find_code()
_indicators_term_find_tid()

Both of these functions are using db_select() and joining directly to the field_data_* table(s).

Instead they should use EntityFieldQuery - this isn't tied to the specific database schema for field storage.

Some things realized during the review. Most of them are just general comments and improvements which could be implemented later I guess. To be honest I also
don't really have any clue about the current review process.

• indicators_install() calls out to _indicators_import_create_voc() during install, which though is just provided by indicators_import.module.
  Maybe some of that install code should be moved to indicators_import module itself?
• The module requires the features module so we could maybe skip parts of the install file, like creating the fields.
• _indicators_term_find_code () could convert the machine_name to a vid first. Then an EFQ should be usable,
  same with _indicators_term_find_tid .
• Is there a reason the indicators_query.module is not part of the indicators_query folder?
• function indicators_query_preprocess_html():

  IMHO we could replace that by using some Drupal.behaviours.foo call inside the javascript.

• indicators_import_datasets_list()
  Ideally you would use drupal_static_reset() instead of the simple static.
• Another suggestion: indicators_query_data_field_options : This could use trigger_error(), so normal users don't see that message.

• function indicators_geo_filter_html_form($form, &$form_state, $inner_html, $help) {
    $form = array();
    if ($help) {
      $form['help'] = array(
        '#markup' => '<div class="geo-picker-map-help">' . t($help) . '</div>',
      );
    }
  

  In order to make it possible to parse the t() strings using potx

  ideally you would use t('the-help-string'), but I see, this help text is coming from configuration, so ideally you would rely on https://www.drupal.org/project/i18n for translation those help texts.

• In an ideal world functions like _indicators_import_data_save_line_eurostat() should use db_insert() instead of raw sql queries, because its on the one
  hand more reusable (sqlite, pgsql) and on the other hand potentially more secure.
• Is there a reason why the indicators_devel modules still exists? This module seems quite empty and its function could be easily replaced by the devel module.

Automated Review

I ran your code against auto code sniffer at 'http://pareview.sh/pareview/httpgitdrupalorgsandboxec-entr2304245git'.

Review of the 7.x-1.x branch (commit 93c2f91):

EDIT: removed long pareview.sh dump.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Causes module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*) Major finding, needs work
2. (+) Remove empty comment line at line no. 1026 in indicators_query.module file.
3. Suggestion
4. Would be good, if you use block comment instead of line comment.
5. Instead of strtolower, substr function use drupal_strtolower(), drupal_substr().
6. For db_query second argument must be an array, verify in indicators_query.module file.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

@Rahul Seth: please don't post long pareview.sh dumps, either attach it as text file or just link to pareview.sh.

Is this application now RTBC or does it need work from the applicant?

There are some things (like the location of the indicators_query.module which would be nice to fix). Not using db_insert() seems to be maybe
critical from a security point of view, not entirely sure.

In general though I mostly found things which can be improved over time, there should be some more room for that. On the other hand, again, I've no idea whether things like codestyle stuff should really block a project.

Hi EC-GROW,

Please fix the some basic issues suggested by paraview (http://pareview.sh/pareview/httpgitdrupalorgsandboxec-entr2304245git) in "indicator_pareview_result.txt" file attached.

Thanks,

Yeah, the file encoding should be fixed. Also:

PAReview: Individual user account

It seems you are using a non-individual account.
All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered (see Get a Drupal.org account).
Please note that organization accounts cannot be approved for git commit access. See https://drupal.org/node/1966218 and https://drupal.org/node/1863498 for details on what is/isn't allowed. Please update your user profile so that we don't have to assume that this is a group account.

Hello,

• Majors coding standards issues are corrected (#26 #30)
• Files encoding are corrected (#31)
• db_query() is use for performance reason to insert/import big datasets (+100.000). As mentioned in https://www.drupal.org/node/310079 db_query not call hooks (#28)

Thanks!

It doesn't seem the point about the individual account has been fixed.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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