[D7] PDFer

Hi.
In function pdfer_help please use only one return operator.
like this.

function pdfer_help($path, $arg) {
  $info = '';
  switch ($path) {
    case 'admin/help#pdfer':
      // Return a line-break version of the module README.txt
      $info = check_markup(file_get_contents( dirname(__FILE__) . "/README.txt") );
      break;

    case 'admin/config/system/pdf-settings':
      $info = '<p>' . t('Page where all pdf-template are listed, you can edit existing template, check this template with "Test Download" button or add your new template') . '</p>';
      break;

    case 'admin/config/system/pdf-settings/add-template':
      $info = '<p>' . t('Page with form where you can add your template and choose settings for it.') . '</p>';
      break;

    case 'admin/config/system/pdf-settings/%/edit':
      $info = '<p>' . t('Page with form where you can edit your existing template and choose new settings for it.') . '</p>';
      break;
  }
  
  return $info;
}

Thanks.

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

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

List of my remarks:

1. Settings menu item should be placed in the proper position and not directly at admin/config/system sub-section.
2. Better to move hook_menu implementation as the first hook of your module.
3. 'test-download' naming is not very good choice.
4. File 'includes/pdfer.admin.inc' should not be directly included. Use 'file' option for appropriated menu item.
5. Why here 'check_markup(file_get_contents(dirname(__FILE__) . "/README.txt"))' drupal_get_path is not used?
6. From what i see on edit page, you are allowing edition of ID. While in submit handler db_merge is used. It means if i change ID and save — record will be duplicated.
7. Better to move direct queries usage from form builder functions — it will be easier to read such code and don't mix form api definitions with data retrieving.
8. It's bad practice to write html within form builders (see pdfer_admin_page). In addition, i guess something like theme('item_list') could be used instead. Probably you should really use action links system for this link.
9. Why HTML allowed for the link from point above?
10. Message 'Please add one or more templates.' is not translated.

Hello!
For me everything is ok)
Good job.

Hi There,

There are few things that needs to be fixed:

pdfer.admin.php
- $header = array('Template ID', 'Language', 'Description', 'Operations'); - Make sure those headers titles are translatable. Use t() function.

pdfer.module
- Please add logic that would nicely display an error message if the WkHtmlToPdf library is missing. Consider using Libraries API module.

Thanks!

I think there will be better to use something like a

  $items[PDFER_DEFAULT_SETTINGS_PATH . '/%pdfer/test-download'] = array(
    'file' => 'includes/pdfer.admin.inc',
    'page callback' => 'pdfer_test_download',
    'page arguments' => array(4),
    'title' => 'PDF templates',
    'access arguments' => array('administer pdf templates'),
  );

and rename function pdfer_load_existing_template_by_template_id() to pdfer_load() to increase usability of this module for developers.
Also I think there could be few hooks.

But module is looking good, I hope you will improve it during upcoming development.

Removing review bonus tag, you have not done all manual reviews, you just pointed out coding standards docs. Make sure to read through the source code of the other projects, as requested on the review bonus page.

Git errors:

• Git default branch is not set, see the documentation on setting a default branch.

manual review:

1. project page is too short. What is the use case of the module what problem does it solve? Does it work on nodes? Or custom entities? How do I use it? Can you make a screenshot what the end result looks like and add that to the project page? See also https://www.drupal.org/node/997024
2. "fix review" is not a useful git commit message, you should desribe what you did. See also https://www.drupal.org/node/52287
3. pdfer_wkhtmltopdf(): @param doc block is missing, see https://www.drupal.org/coding-standards/docs#param

There is a security vulnerability in this module and as part of our git admin training I'm assigning this to th_tushar so that he can take a look. If he does not find anything I'm going to reveal the vulnerability details in one week. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Automated Review

[Best practice issues identified by pareview.sh / drupalcs / coder. Please don't copy/paste all of the results unless they are short. If there are a lot, then post a link to the automated review and mention that problems should be addressed.]

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

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

No: List of security issues identified.

Coding style & Drupal API usage

* Not able to download the Test Download pdf. The path 'admin/config/content/pdf-settings/%/test-download' is not working.

* Use 'id' field from database instead of user entered value for simple wildcard (%) in 'admin/config/content/pdf-settings/%/test-download'. It is expected to be auto-increment database field value and not a user entered value.

* /admin/config/content/pdf-settings is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as Template ID and Template description then I will get two nasty javascript popups on this page. You need to sanitize user provided text before printing, make sure to read https://www.drupal.org/node/28984.

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.

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.

Fixing tags.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Hello andrey.troeglazov,

My findings for your module as follows:

pdfer.admin.inc

Line 38: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
$operations = $edit_link . '|' . $download_link . '|' . $review_as_html;

Thank you,

Hello andrey.troeglazov

My findings for your module as follows:

File: pdfer\includes\pdfer.admin.inc

Line 38 : Concat operator must be surrounded by a single space
Line 54 : Expected 1 blank line after function; 2 found

File: pdfer\pdfer.module

Line 75 : Inline comments must end in full-stops, exclamation marks, colons, question marks, or closing parentheses
Line 97 : There must be exactly one blank line before the tags in a doc comment
Line 97 : Missing parameter type Parameter comment indentation must be 3 spaces,found 2 spaces
Line 100 : Parameter comment indentation must be 3 spaces,found 2 spaces
Line 102 : Type hint "null" missing for $path_to_save

Thank you,

Looks like you forgot to add your additional reviews to the issue summary? Doing that now.

Git errors:

• Git default branch is not set, see the documentation on setting a default branch.

Review of the develop branch (commit c1b86c4):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• 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. make to push all recent changes to the 7.x-1.x branch, this is your mainline branch people will review and use, so it should of course have all security fixes (I read XSS somewhere in your git history).
2. project page is too short. What is the use case of the module what problem does it solve? Does it work on nodes? Or custom entities? How do I use it? Can you make a screenshot what the end result looks like and add that to the project page? See also https://www.drupal.org/node/997024
3. pdfer_wkhtmltopdf(): do not use "print" here, that will print to the page output in some random place. Use drupal_set_message() instead.
4. pdfer_admin_page(): do not call theme() here, just return a nested render array. Drupal core will render everything later for you. See https://www.drupal.org/node/930760
5. pdfer_add_edit_template_form_submit(): the check_plain() is wrong here: "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database" from https://www.drupal.org/node/28984
6. pdfer_add_edit_template_form_submit(): why is the htmlspecialchars_decode() call needed here? Please add a comment.
7. pdfer_add_edit_template_form(): the check_plain() calls are wrong here, #default_value is already escaped by the Form API and double escaping is bad. Make sure to read https://www.drupal.org/node/28984 again.
8. pdfer_review_as_html(): that callback prints out the HTML as is, which means an admin that can create those PDF templates can easily insert Javscript tags and perform XSS. Therefore I think you should mark your "administer pdf templates" permission as "restrict access" => TRUE to indicate that only fully trusted admins should get that permission.

The wrong check_plain() usage and the permission restriction is a blocker right now.

Looks good to me apart from the remaining points from #37 that are not application blockers.

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

Automated Review

Git errors:

• Git default branch is not set, see the documentation on setting a default branch.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• 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.

FILE: /Users/matt/PAR/pareview_temp/includes/pdfer.admin.inc
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one blank
   |       |     line
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------


FILE: /Users/matt/PAR/pareview_temp/includes/pdfer.form.inc
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one blank
   |       |     line
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------


FILE: /Users/matt/PAR/pareview_temp/pdfer.install
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one blank
   |       |     line
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------


FILE: /Users/matt/PAR/pareview_temp/pdfer.module
--------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
--------------------------------------------------------------------------
   1 | ERROR | [x] The PHP open tag must be followed by exactly one blank
     |       |     line
  99 | ERROR | [ ] Missing parameter type
 104 | ERROR | [ ] Type hint "null" missing for $path_to_save
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

Time: 212ms; Memory: 6Mb

Manual Review

Don't quite see why the htmlspecialchars_decode() is needed in pdfer_add_edit_template_form(), but since #default_value is sanitized, this isn't a problem.

Why the explicit redirect code in pdfer_add_edit_template_form_submit? Comment needed (or just get rid of it).

Not seething anything else that hasn't been mentioned, nor any blockers.

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.

Thanks for your contribution, andrey.troeglazov!

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.