[D6] PDF Merger

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

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.

Hi teyser,

Please follow the instructions on http://drupal.org/node/1011698 #5 including linking to the project page, giving the Git URL (which is different from the code browsing URL) and specifying the Drupal version in the title. All of this makes it easier for people to review your project, which should speed up the process for you.

Download path added

You need to set the status to "needs review" if you want to get a review, see http://drupal.org/node/532400

Link to the project page is missing in the issue summary.

I added the project link to the issue description and I'm also adding the Drupal version to the title, both to make this easier to review.

Did you follow the commands in all steps, 1 through 6? step 6 deletes the master branch.

You should add the tag upon completing three reviews.

I would like to recommend that you produce a D7 version of this module. I have a working D7 version of this module if you would like to add it to your repository. I think this module may be better than my own module PDFTK, seeing as my module requires software installed on the server. I would also be willing to head up the D7 branch once this becomes contributed if you don't have a Drupal 7 environment.

Project code syntax is all clean, and module seems to do what developer says, The only problem I see as needing to be resolved is a better defined readme file explanation - the description / readme about what this module does are kind of hard to understand.

manual review:

1. "CSF" is not a useful git commit message, please follow http://drupal.org/node/52287
2. info file: core property appears twice? And the drupal.org packaging comment should be removed.
3. PDFMerger.php: appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
4. "require_once 'sites/all/libraries/fpdi/fpdi.php';": do not hardcode that library path since the library might be in sites/example.com/libraries as well.
5. pdf_merger_merge(): do not change the PHP memory limit from your module! That will fail on shared hosts where it is forbidden to change that setting. Just include a note in the README.txt that people might need to increase the memory. Same for set_time_limit(0).
6. pdf_merger_merge(): So there is always the same $pdf_file_name? That will cause ugly race conditions if more than one user try to merge PDFs at the same time. You should create a temporary file instead that has a unique name, right?
7. "Please make sue to configure the correct PDF located ...": all user facing text must run through t() for translation. Same for "drupal_set_message('No PDF files under the mentioned directory');", please check all your strings.
8. pdf_merger_menu(): why is the pdfmerge path only protected with "access content"? Shouldn't it use the same permission as the other admin pages?

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

manual review:

• "variable_set('pdf_merger_merged_directory', $pdf_merger_directory);": all variables defined by your module need to be removed in hook_uninstall(). Same for the other variables.

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

Trying to remove tag again.

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

Thanks for your contribution, teyser!

I updated your account to let you 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 get 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.