[D7] Flexpaper

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

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.

Is a most needed module since my current project is handling with a lot of pdf publications. Happy to see the future release and contribute it.

Not sure hook_uninstall() for the variable "path_to_pdf2swf" and "path_to_pdf2json" is necessary or not since they are in system_settings_form.

Suggestion:

flexpaper.install from line 14, add the libraries module check.

Background:

This can avoid the drush error when using Drush pm-enable flexpaper

If the libraries is not installed, the above command can start the download but the libraries module is not enabled.

The error message are:

Fatal error: Call to undefined function libraries_get_libraries()
Drush command terminated abnormally due to an unrecoverable error.

The following codes are just like fuse. :)

  // Check libraries module.
  if (!module_exists('libraries') || !function_exists('libraries_get_libraries')) {
    $requirements['flexpaper'] = array(
      'title' => $t('Libraries module'),
      'description' => $t('Flexpaper module requires the <a href="@url">Libraries module</a> to be installed.', array('@url' => 'http://drupal.org/project/libraries')),
      'severity' => REQUIREMENT_ERROR,
    );
  }

@feolius you are right, it is not belong to issue but drush problem. There is no problem in web mode.

Another suggestion line 9 flexpaper-container.tpl.php

Wrap the text in t() function
<?php print t('File is downloading...'); ?>

Anyway, nice work!

flexpaper.module, line 28:
Use $form_state['values']['my_field'] instead of directly using $form_state['input'].

flexpaper.module, line 29, 46 and 53:
All user defined variables should start with the module name (eg. flexpaper_VARIABLE) to avoid collisions.

Beside from that, everything looks great. It's nice to see a well documented readme file.
Keep up the good work.

Don't forget to add the "PAReview: review bonus" tag next time as indicated in http://drupal.org/node/1975228 , otherwise you won't show up on my high priority list.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/flexpaper.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   28 | WARNING | Do not use the raw $form_state['input'], use
      |         | $form_state['values'] instead where possible
  --------------------------------------------------------------------------------
  

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. flexpaper_settings_page_form(): looks like you want to display that messages on validation, not in the form building function. Please move them to a form validation handler.
2. flexpaper_field_formatter_info(): I think the description should run through t() here, or is that done by field module?
3. flexpaper_field_formatter_view(): instead of calling drupal_add_js/css() you could use the #attached property of the render array?
4. "$context['sandbox']['progress'] . ' of ' . $context['sandbox']['max'] . ' files are processed';": all user facing text must run through t() for translation. Also elsewhere, please check all your strings.
5. template_preprocess_flexpaper_container(): do not call theme_file_link() directly, use theme('file_link', ...); instead.
6. _flexpaper_get_pdf_pages_number() suppressing erros with "@" is a bad idea, that could make debugging really hard. Why do you need this? Please add a comment.
7. flexpaper-container.tpl.php: "t(File is downloading...)" should be printed in PHP tags.

Although you should definitely fix those issues they are not critical application blockers, so I guess this is RTBC. You can add the review bonus tag if you have done another 3 reviews of other projects.

Assigning to Dave Reid as he might have time to take a final look at this.

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

Thanks for your contribution, feolius!

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.