[D7] PDF using mPDF

Manual Review :

1) Do not use html tag within t() function.
2) Use t() function for defining description at line no 46 in pdf_using_mpdf.module file.
3) Declaration of all themes should be in a single array in hook_theme() . You can apply your theme rendering conditions as where needed.
4) Please keep pdf_using_mpdf_config_submit() function in pdf_using_mpdf.admin.inc file.
5) Please do not use $_REQUEST variable in pdf_using_mpdf_config_submit() function.
5) Use system_settings_form() function for saving admin settings information.
6) Please don't add LICENSE.txt, it will be added by drupal community after completing review process and full project release.

Thanks....

Manual Review :

1) Do not use html tag within t() function.
2) Use t() function for defining description at line no 46 in pdf_using_mpdf.module file.
3) Declaration of all themes should be in a single array in hook_theme() . You can apply your theme rendering conditions as where needed.
4) Please keep pdf_using_mpdf_config_submit() function in pdf_using_mpdf.admin.inc file.
5) Please do not use $_REQUEST variable in pdf_using_mpdf_config_submit() function.
5) Use system_settings_form() function for saving admin settings information.
6) Please don't add LICENSE.txt, it will be added by drupal community after completing review process and full project release.

Thanks....

This issue is not fixed? See http://drupal.org/node/532400

Priority is normal right now, see http://drupal.org/node/894256

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/1823698
Project 2: http://drupal.org/node/1822638
Project 3: http://drupal.org/node/1823696

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

1). Anonymous user can not access admin/config/user-interface/mpdf, Please check it.
2).Use t() function for defining description at line no 152, 159 in pdf_using_mpdf.admin.inc file.
3).Generate PDF document title show to anonymous user, please manage it by permission.

I'd suggest removing a CREDITS.txt file in favor of simply adding the text from that file into the bottom of README.txt.

I believe this module should be dependent upon the Libraries API and use the module's API functions for loading the mPDF library. Currently a constant MPDF_LIB_PATH is defined which dictates where the mPDF library resides. Unfortunately, hard-coding the path this way will pose problems for multisite installations and is difficult for other modules/themes to customize.

function pdf_using_mpdf_help($path, $arg) {
  if ($path == 'admin/help#pdf_using_mpdf') {

    $data = '<p>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;' . t('This module is used Conversion of HTML page to PDF using mPDF PHP Library. This module allows you to generate the pdf documents of any node:') . '<br /><br /><strong>' . t('PDF creation (at www.example.com/show_pdf/nid)') . '</strong><br /><br /> ' . t('where nid is the node id of content to render.') . '<br /><br />' . t('By creating your own CSS file and/or the pdf--node.tpl.php files, it is possible to change the look of the output page to suit your taste. For a more fine-grained customization, it is possible to use a template file named:') . '<br /><br /><strong>' . t('Drupal 7: pdf--node--[node-type|node-id].tpl.php') . '</strong><br />' . t('located in the same module directory i.e., "pdf_using_mpdf".') . '<br />' . t('Where node-type & node-id are Drupal node type (e.g. page, story, etc.) and node-id (eg: 12) respectively.') . '<br /><br /><strong>' . t('API Function : pdf_using_mpdf_api()') . '</strong><br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;' . t('This API function is available to content developers that prefer  to generate a pdf file of custom path. The function takes two parameters, first a rendered html content and an optional second parameter, name of the pdf file. E.g:   pdf_using_mpdf_api($html) where $html is any html content.') . '<br /><br />' . t('You must install the following third-party tools to generate PDFs:') . '<br /><br />' . l(t('mPDF'), 'http://www.mpdf1.com') . '<br /><br />' . t('Please follow the instructions in the README.txt files carefully.') . '<br /><br />' . t('Developed By :') . ' ' . l(t('OSSCube'), 'http://www.osscube.com/') . '<br /><br /></p>';
    return $data;
  }
}

The non-breaking space should be removed as well as the line breaks. Use multiple paragraphs and variable substitution and required.

  $items['show_pdf/%'] = array(
    'page callback' => 'drupal_get_form',
    'page arguments' => array('pdf_using_mpdf_generate_pdf'),
    'access callback' => TRUE,
    'type' => MENU_CALLBACK,
    'file' => 'pdf_using_mpdf.pages.inc',
  );

The access callback should not be TRUE, it should instead use an access arguments which requires the "administer custom mpdf" permission. All access-related code should be removed from pdf_using_mpdf_generate_pdf().

/* ********************* End of page.inc  ************************ */

There's no need for the comment.

    $form['pdf']['radio_btn'] = array(
      '#type' => 'markup',
      '#title' => 'MPDF library',
      '#markup' => 'Add mPDF library to "' . MPDF_LIB_PATH . '/' . MPDF_API_FILE . '" or "' . MPDF_MODULE_PATH . '/mpdf/' . MPDF_API_FILE . '".',
    );

Both the title and the markup text should be run through the t() function. You'll also probably want to enclose the markup in <p> </p> as it is displaying a paragraph of text.

'#description' => t('use') . ' {PAGENO} ' . t('for page numbering or') . " ' {DATE j-m-Y} ' " . t('for current date.'),

You should use variable substitution here to make the text easier to translate.

'#markup' => '<br />' . t('Password : ******** is already set.') . '<br /><br />',

This should probably be a paragraph (which would also eliminate the need for so many line breaks).

'#markup' => '<p>No mPDF Library Found in"' . ' ' . MPDF_LIB_PATH . '/' . MPDF_API_FILE . ' or ' . MPDF_MODULE_PATH . '/' . MPDF_API_FILE . '".<br />Please dowload mPDF supported PHP PDF generation tool from ' . l(t('MPDF Library.'), 'http://www.mpdf1.com/') . '</p>',

The text should be run through t() with appropriate variable substitution and the <p> should be concatenated with the translated text just like </p> is.

/* ********************* End of admin.inc  ************************ */

There's no need for the comment.

README.txt

This module requires MPDF >= 5.9.012. The current version of mpdf is 5.4 ...

you should move the MPDF support section inside the installation part

you could create a requirement section as well

if you give the possibility to copy mpdf in the module folder, the library module should not be marked as dependent in the .info file

add the allowed paths to mpdf.php in the installation section

project page

set php_mbstring extension as required

Master Branch

you should delete it and just keep the 7.x-1.x

Major coding standards / best practice issues

see errors http://ventral.org/pareview/httpgitdrupalorgsandboxosscube1819680git

Code

You should allow the library to be in the libraries folder of the site in multisite mode ( sites/mysitename/libraries/mpdf/mpdf.php ) define('MPDF_LIB_MODULE_PATH', 'sites/' . $_SERVER['SERVER_NAME'] . '/libraries/mpdf');

problem with system_settings_form() in pdf_using_mpdf.admin.inc, no buttons displayed... ? so i can't check if the options are working or not...

... and i change the status

Hi,

1). No description of MPDF Libraries storage directory overview into README.txt file. Please check it.
2). Add mPDF library to "sites/all/libraries/mpdf/mpdf.php" or "sites/all/modules/custom/pdf_using_mpdf/mpdf/mpdf.php". message show after configure mpdf, please check it.

This kind of translation are not correct, since grammar structure couldn't be respected in some languages:

'#description' => t('If no name given to PDF file, Default filename : "') . PDF_DEFAULT_FILENAME . t('" will be used.'),

That one is in pdf_using_mpdf.admin.inc line 165, but there are a lot in the whole project.

Use instead the form:

'#description' => t('If no name given to PDF file, Default filename : !default_filename will be used.', array('!default_filename' => PDF_DEFAULT_FILENAME)),

This other t() use is critical, since it generates a new translatable string every time a variable/constant like MPDF_LIB_PATH changes, and it uses t() function inside string for another t() function (lines 171-176 in pdf_using_mpdf.admin.inc):

$message = '<p>No mPDF Library Found in"' . ' ' . MPDF_LIB_PATH . '/' . MPDF_API_FILE . ' or ' . MPDF_MODULE_PATH . '/' . MPDF_API_FILE . '".<p>Please dowload mPDF supported PHP PDF generation tool from ' . l(t('MPDF Library.'), 'http://www.mpdf1.com/') . '</p>';
...
'#markup' => t($message),

Please, fix all wrong t() uses.

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

There are some more t() bad uses not fixed:

pdf_using_mpdf.pages.inc, line 300
pdf_using_mpdf.admin.inc, line 103
pdf_using_mpdf.admin.inc, line 158
pdf_using_mpdf.admin.inc, line 175
pdf_using_mpdf.module, line 24

Generally, it's a bad use:

• To insert variables into t string:
  • t('My name is ' . $user->name);
  • You will need to translate that phrase for every user (and every time a new user is created). So, potentially infinite translations.
• to concatenate t(...) . SOMETHING . t(...), because order in other languages, the grammar can change:
  • Peter's house -> La casa de Peter
• to put inside t( SOMETHING ... t( SOMETHING) ), because it generate lot of translatable strings:
  • t('My name is ' . t('Peter'))
  • String to translate 'My name is Peter' will be created, and needed to translate.
  • If I translate 'Peter' to 'Pedro', string to translate 'My name is Pedro' will be also created', and the last one won't be used (so in the site it may appear in wrong language)

Please, check all t(), maybe I lost some others.

Hey,

First glance:
Could be a promising module, however the print module does accomplish this task as well, and implements flexible pdf libraries as well. It does not use mPdf however.
I would advocate you spend your efforts contributing an mPdf option to the print module, rather then developing a separate module.

Automated review using PAReview:
There is still a master branch in your repository.
There is version information in the info file, which should be removed as it is maintained by drupal.org.

Documentation:
The README.txt states you require Drupal 7.0 . I would assume this is a minimum requirement, but changing to "Drupal 7.0 or later" goes a long way.
CREDITS should go into README.txt as an earlier reviewer mentioned.

Code:
This function is completely redundant:

function pdf_using_mpdf_user_access() {
  if (user_access('administer custom mpdf')) {
    return TRUE;
  }
  else {
    return FALSE;
  }
}

you would change in your hook_menu:

'access callback' => 'pdf_using_mpdf_user_access',

to

'access arguments' => array('administer custom mpdf'),

and remove the above mention function.

The following access callback should be refactored to use permissions rather then admin role. user_access already has this exception anyway, so this function is not only redundant, but restricts the use of the admin pages to the admin user..

function pdf_using_mpdf_admin_config_access() {
  global $user;
  if ($user->uid == 0 || $user->uid != 1) {
    return FALSE;
  }
  else {
    return TRUE;
  }
}

I stopped here. There is alot of work to complete this, and I do consider this module a duplicate of the print module.

This sounds like a feature that should live in the existing print project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the print issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

Ok, so I think we can continue with this application.

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:

• Remove "version" from the ./pdf_using_mpdf.info file, it will be added by drupal.org packaging automatically.

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. pdf_using_mpdf_uninstall(): all variables that your module defines need to be prefixed with your module's name to avoid name clashes.
2. "define('MPDF_LIB_PATH', 'sites/all/libraries/mpdf');": all constants that your module defines need to be prefixed with your module's name to avoid name clashes.
3. pdf_using_mpdf.module: why do you need to load pdf_using_mpdf.pages.inc on every single page request? Please remove the include call in the global context.
4. pdf_using_mpdf_help(): Why do you need "!default_help"? Only dynamic variables should be passed to t() with placeholders. Static strings as here should be directly in t(). Otherwise they cannot be found by automatic translation extraction tools.
5. pdf_using_mpdf_admin_config_access(): Feedback from comment #22 has not been addressed?

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

1. Look at the function pdf_using_mpdf_user_access():
   

   function pdf_using_mpdf_user_access() {
     return user_access('administer custom mpdf');
   }
   

   Is really needed?

2. Similar in function pdf_using_mpdf_admin_config_access(), could be implemented as:
   

   function pdf_using_mpdf_admin_config_access() {
     global $user;
     return $user->uid === 1;
   }

3. I'm not sure you need to define constants like PDF_USING_MPDF_MPDF_MODULE_PATH. Others like PDF_USING_MPDF_PDF_PAGE_SIZE should be better if the user can customize them using the UI.

manual review:

1. pdf_using_mpdf_generate_pdf(): the user_access() check is not necessary, you specified the permission already in hook_menu().
2. when I create a node and go to /show_pdf/1 it always returns access denied. "'access callback' => user_access('administer custom mpdf')," is wrong, please read the documentation of hook_menu() again and test your module before setting this back to needs review.
3. While testing I wanted to exploit a security issue in your code: I do not see where you respect node access grants when loading the node with _pdf_using_mpdf_node_generator(). So when you fix the page callback make sure to run node_access().
4. pdf_using_mpdf_menu(): why do you include pdf_using_mpdf.pages.inc here if you do not use any code from it?
5. pdf_using_mpdf_admin_config_access(): that is a bad access callback because it limits the settings page to one single admin user. Better use a permission instead.
6. "module_load_include('inc', 'pdf_using_mpdf', 'pdf_using_mpdf.pages');": no need to use module_load_include as you are including files from your own module which you already know where they are. Use something like require_once 'mymodule.inc';
7. pdf_using_mpdf_theme(): that is not how hook_theme() works. You cannot use the dynamic node object from the current context, because the return value of this function is cached in the theme registry. See http://drupal.org/node/933976
8. pdf_using_mpdf_node_view_alter(): better use hook_node_view() instead?
9. _pdf_using_mpdf_generator() should be defined in the module file if it is used that often.

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

manual review:

1. pdf_using_mpdf_user_access(): this function is useless and can be removed. Just use the permission in hook_menu() with "access arguments". user_access() is the default access callback. Same for pdf_using_mpdf_admin_config_access().
2. "administer custom mpdf": that is a confusing permission name, better use "generate pdf using mpdf" instead?
3. "variable_get('PDF_USING_MPDF_PDF_PAGE_SIZE', PDF_USING_MPDF_PDF_PAGE_SIZE);": usually variable names are all lower case. And are you sure that this variable is removed in the uninstall function because of the different casing?
4. "drupal_set_message(check_plain(t('mPDF library is...": no user provided text is involved here, so check_plain() is useless and should be removed. Please read http://drupal.org/node/28984 again.
5. pdf_using_mpdf_generate_pdf(): "$node = node_load($node->nid);": what is that good for? $node should be loaded already?
6. "@return mpdf": mpdf is not a valid data type? See http://drupal.org/node/1354#functions
7. pdf_using_mpdf_generate_pdf(): this page callback does not respect node access control. If grant anonymous users the permission to generate PDFs and unpublish a node, then the PDF is still accessible for anonymous users and the content can be retrieved. As I said: you need to check node_access() before outputting the PDF. This is a security issue.

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

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

manual review:

1. pdf_using_mpdf_config(): the check if the library exists should be done in hook_requirements().
2. "define('PDF_USING_MPDF_MPDF_LIB_PATH', 'sites/all/libraries/mpdf');": that will not work if I have my libraries folder in profiles/recruiter/libraries. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

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.

Now actually removing tag.

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

Thanks for your contribution, osscube!

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.

Hello friends.

i have download and install the mpdf module also install the mpdf library.

after that i go to the admin/config/user-interface/ mpfd

then message display like this...
Add mPDF library to "sites/all/libraries/mpdf/mpdf.php" or "sites/all/modules/custom/pdf_using_mpdf/mpdf/mpdf.php". message show after configure mpdf, please check it.

so how can i add the libraries ?? how can i solve my problem???

This project application is closed. Please post bugs to the module's issue queue at https://www.drupal.org/project/issues/pdf_using_mpdf?categories=All

Dear Vineet,

I am using this module to print html data into pdf using pdf_using_mpdf_api() function.

When I am sending html to pdf using this function, It always gives me "504 Gateway Time-out" . I have also checked my server max_execution_time.

Could you please help me over the issue.

Thanks in advance.

There is some coding standard errors to fix :

Review of the 7.x-2.x branch (commit a61a87c):

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

  
  FILE: /var/www/drupal-7-pareview/pareview_temp/pdf_using_mpdf.module
  ----------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  ----------------------------------------------------------------------
   155 | WARNING | Unused variable $root_path.
  ----------------------------------------------------------------------
  
  
  FILE: /var/www/drupal-7-pareview/pareview_temp/pdf_using_mpdf.install
  --------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  --------------------------------------------------------------------------
   39 | WARNING | Do not use DELETE queries with db_query(), use
      |         | db_delete() instead
  --------------------------------------------------------------------------
  
  
  FILE: /var/www/drupal-7-pareview/pareview_temp/pdf_using_mpdf.admin.inc
  --------------------------------------------------------------------------
  FOUND 0 ERRORS AND 10 WARNINGS AFFECTING 3 LINES
  --------------------------------------------------------------------------
   127 | WARNING | #options values usually have to run through t() for
       |         | translation
   127 | WARNING | #options values usually have to run through t() for
       |         | translation
   127 | WARNING | #options values usually have to run through t() for
       |         | translation
   127 | WARNING | #options values usually have to run through t() for
       |         | translation
   140 | WARNING | #options values usually have to run through t() for
       |         | translation
   140 | WARNING | #options values usually have to run through t() for
       |         | translation
   140 | WARNING | #options values usually have to run through t() for
       |         | translation
   140 | WARNING | #options values usually have to run through t() for
       |         | translation
   140 | WARNING | #options values usually have to run through t() for
       |         | translation
   271 | WARNING | Variable $type_name is undefined.
  --------------------------------------------------------------------------
  
  Time: 95ms; Memory: 8Mb
  

• Codespell has found some spelling errors in your code.
  

  ./CREDITS.txt:0: sponsered  ==> sponsored
  

• 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.

Source: http://pareview.sh/ - PAReview.sh online service

Below is the basic example of 'PDF using mPDF'

<?php
$html = '
<h1><a name="top">This is a demo pdf file.</a>mPDF</h1>
<h2>This file demonstrates most of the HTML elements.</h2>

<h3>Heading 1</h3>
<h4>Heading 2</h4>
<h5>Heading 3</h5>
';
include("MPDF57/mpdf.php");
$mpdf=new mPDF();
$mpdf->WriteHTML($html);
$mpdf->Output();
?>

@Chetan Sharma this project application was approved 4 years ago.

The full module can be found here:
https://www.drupal.org/project/pdf_using_mpdf

You may wish to repost your support request there.

Hello i'm looking for a custom of this module.
print $content; if i need to see more on the content type generated by