[D7] RePEc

Git clone command for the sandbox is missing in the issue summary, please add it.

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.

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.

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

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

There are a lot of errors reported by the automated review. I think you should start there.

Also, you should have a 7.x-1.x branch for your code, and not a master branch. Given the number of errors, it will be better to fix them before attempting a manual review.

Automated Review

The automated review still returns a number of errors. Please visit http://pareview.sh and enter http://git.drupal.org/sandbox/EversonDaSilva/2333885.git as the branch to test.

You also have an extra directory in your repository (e.g. "repec/repec/repec.module") that could be throwing off some of the tests. It certainly seems to interfere with the check for a README.txt file.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

No: Does not follow the guidelines for master branch. You have set the 7.x-1.x branch as your default, but Drupal also recommends you remove the master branch as well.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code. I found no such assets in my review.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template. I recommend that you break the README file out into separate files, one of which could deal with the installation and configuration of the module (which looks like it would be quite short, but you should assume that the person installing your module has never installed a module before), an explanation of your motivation, and then perhaps a separate document for your format samples.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Please let me know when pareview.sh does not return any errors, and I'll happily perform a manual security review.

Coding style & Drupal API usage

PAreview.sh currently returns a number of errors.

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.

Also, please make sure your most up to date code is reflected in the 7.x-1.x branch of the drupal.org git repository. While I appreciate that you've attached zip files, I've based my review on the git repository, which is what Drupal considers authoritative. I'll happily continue the review when the issues detected by the automated tests are ameliorated.

No need to use quotes in *.info file.
You *.info file looks like

name = RePEc
description = Integrating Working Papers with RePEc.
core = 7.x
package = RePEc
configure = admin/config/services/repec

Your git clone command required password. Please update the description of your project following the example https://www.drupal.org/node/2532406

Nice module, Everson. Undoubtedly, a lot of effort has been put into it.

Because the module is this elaborate, I wasn't able to do a complete review, but below are some potential issues I found after checking some parts of it:

• The CSS selectors the module uses in the stylesheets are too generic, and may conflict with themes. Consider applying a wrapper element with a selector that contains the module's name in it.
• The module configuration form is farily big. Consider separating it into a .inc file to decrease memory consumption.
• In repec_fields_association(), you pass NULL to hook_repec_paper_mapping(), but the implementation of that hook produces an error when the argument is NULL.
• In repec_generate_paper_template(), when the directory cannot be created, the function still proceeds to try to save the file. I think you may need to add a return statement when you check that.
• repec_settings_form_submit() expects $form_state['values']['repec_check_delimiters'] to be set. However, this is not always the case, which results in an error.
• Hook implementations should be in the .module file so that they are always available.

Hi Everson,

Please commit your code to the repository instead of attaching your module to the post.
Once you make the change and commit your code respond here and change the issue status to "Needs review".

Thanks,

Minnur

I have reviewed the module and all other related materials and found some issues as followings:

1. You should not upload your module as zip file in module page. I understand that there is no download links for sandbox page. I don't think you should make the download links by yourself.

2. All git commit comment is lack of information. I see there are many comment like "Refine module", "Update repec", "Fix code". Commit comment is not one of the item we are asked to check, but you should avoid doing that again.

3. There are quite a lot variable_get() in the form. Why not just combine them and use one large array?

@EversonDaSilva

I may make you confused. I don't mean that variable_get() does affect the performance. It won't because variable_get() and variable_set() are the helper functions to read and save data from global $conf array. This module keep repeating variable_get() and variable_set() which is not really necessary. There are 2 ways to help you simplify your coding.

1. using system_settings_form() to replace your own repec_settings_form_submit().

system_settings_form() is provided by Drupal core and it provide a generic way to make form element value into variable with proper keys. Please check the example in https://www.drupal.org/node/1111260

2. using #tree in fieldset

for example, in below snippet:

$form['repec_provider'] = array(
    '#type' => 'fieldset',
    '#title' => t('Provider'),
  );

  // Provider Name.
  $form['repec_provider']['repec_provider_name'] = array(
    '#type' => 'textfield',
    '#title' => t('Provider Name'),
  );

  // Provider Homepage.
  $form['repec_provider']['repec_provider_homepage'] = array(
    '#type' => 'textfield',
    '#title' => t('Provider Homepage'),
  );

  // Provider Institution.
  $form['repec_provider']['repec_provider_institution'] = array(
    '#type' => 'textfield',
    '#title' => t('Provider Institution'),
  );

You will get 3 variable keys at the end. They are repec_provider_name, repec_provider_homepage and repec_provider_institution. You need 3 variable_get() to retrieve data.

<?php
  variable_get('repec_provider_name'); // value of name
  variable_get('repec_provider_homepage'); // value of homepage
  variable_get('repec_provider_institution'); // value of institution
?>

After you used #tree:

$form['repec_provider'] = array(
    '#type' => 'fieldset',
    '#title' => t('Provider'),
    '#tree' => true,
  );

  // Provider Name.
  $form['repec_provider']['name'] = array(
    '#type' => 'textfield',
    '#title' => t('Provider Name'),
  );

  // Provider Homepage.
  $form['repec_provider']['homepage'] = array(
    '#type' => 'textfield',
    '#title' => t('Provider Homepage'),
  );

  // Provider Institution.
  $form['repec_provider']['institution'] = array(
    '#type' => 'textfield',
    '#title' => t('Provider Institution'),
  );

You will get a variable with key repec_provider which saved a associative array with keys name, homepage and institution.

  variable_get('repec_provider'); // it returns array{'name' => '', 'homepage' => '', 'institution'=> ''}

---------------------------------- it is a separate line -----------------------------------

There are some other issues in your module.

Never call module_load_include() globally

Just remove the first module_load_include() in the .module file.

Global_variables() is not necessary

I has mentioned above. And please make sure every function in module has name as prefix.

Incorrect comment format

/**
 * Implements hook_repec_paper_mapping().
 */
/**
 * It maps the series fields with the node fields to generate the template file.
 */

Change the comment similar to above into below format.

/**
 * Implements hook_repec_paper_mapping().
 *
 * It maps the series fields with the node fields to generate the template file.
 */

Hi Everson,

I have done another round of review. Please see my feedback below:

- The repec_create_settings_form() function you created for hook_menu is not necessary. You can specify the form name (repec_settings_form) directly in the menu item by using drupal_get_form as the page callback. You’ll need to use the 'file' attribute to specify which file needs to be loaded.

- The variable $content_type is undefined in repect_form.inc on line 175

    '#description' => t($content_type . 'Please, associate fields from the selected content type to the series template. This will automatically be persisted.'),

In addition, you should not use variables inside t(), because that prevents proper translation of the string.

- repec_repec_paper_mapping() produces an error when the argument is NULL, and it does get called with a NULL in repec_fields_association(). Perhaps update repec_repec_paper_mapping() so that it gracefully handles NULLs.

Warning: get_object_vars() expects parameter 1 to be object, null given in repec_repec_paper_mapping() (line 78 of sites/all/modules/repec/repec.module).

- I’m getting the following error when submitting the settings form:

Notice: Trying to get property of non-object in repec_get_field_content_type() (line 277 of sites/all/modules/repec/repec_aux.inc).

- You are not supposed to set the default values in $form_state in the form callback. The following lines are unnecessary in repec_settings_form(). The default values will be populated automatically based on the #default_value attributes in the form elements.

  $form_state['values']['repec_series']['name']                        = empty($form_state['values']['repec_series']['name']) ? $series['name'] : $form_state['values']['repec_series']['name'];
  $form_state['values']['repec_series']['repec_paper']['name']         = empty($form_state['values']['repec_series']['repec_paper']['name']) ? $series['repec_paper']['name'] : $form_state['values']['repec_series']['repec_paper']['name'];
  $form_state['values']['repec_series']['repec_paper']['content_type'] = empty($form_state['values']['repec_series']['repec_paper']['content_type']) ? $series['repec_paper']['content_type'] : $form_state['values']['repec_series']['repec_paper']['content_type'];

- In repec_form_alter(), there is no need to alter your own form. You can just move everything from this hook to the form callback. Also there is no need to manually declare the repec_settings_form_submit and repec_settings_form_validate submit/validation handlers because those are the default ones.

I see your point about the form handlers. I missed that.

Great work on this module! I think promotion to a full project would be in order.

Review of the 7.x-1.x branch (commit 4590c41):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/repec.module
  ----------------------------------------------------------------------
  FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 2 LINES
  ----------------------------------------------------------------------
   271 | WARNING | Unused variable $repec_series.
   272 | WARNING | Variable $series is undefined.
   272 | WARNING | Variable $series is undefined.
  ----------------------------------------------------------------------
  
  Time: 43ms; Memory: 8Mb
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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 page: what does this module do? Does it create a content type where the content is then exported to some RePec site? Or does it import content from RePec? What is the use case? Does it provide a View or a block to display RePec articles? How does it work in the Drupal context? Can you improve the project page according to https://www.drupal.org/node/997024 ?
2. repec_schema(): descriptions for the table columns are missing, see https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
3. t('This module helps you create a repository for') . ' <a href="http://repec.org" target="_blank">' . t('Research Papers in Economics'): do not concatenate translatable strings like that, use placeholders for URLs. See https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7.x
4. repec_associate_fields(): double doc block over the function, one should be removed?
5. repec_create_directory(): us drupal_mkdir() and drupal_unlink() here. Also elsewhere.
6. repec_create_directory(): this will not scale if you have thousands of nodes that are loaded all at once with node_load_multiple() => PHP out of memory or timeout errors.

But otherwise looks good to me.

Thanks for your contribution, Everson!

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.