omissis [omissis]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

Hey omissis,

Were you going to provide your iso_639 module too? as your module can't be installed without it.

I've actually recently completed an epub exporter for the epublish module. It's very custom written for a clients needs at present, so its not ready for submitting to drupal.org yet. I was also tossing up, whether it would be better to have an epub like api, that everything can use, like book, epublish, and others.

Looking over your code you've done a lot of work, so well done. I notice though, that you're using additional php libraries that the user installing the module must download first - I've found it easier to create the epub file without the use of external libraries, using the ZipArchive features of php. I'll post back my notes on this in my spare time - perhaps I can help you co-maintain this module.

Anyway, when you post the iso_639 module, I'll be sure to check this out. Cheers.

@alexkb: Purpose of the review is to

1. make sure this doesn't duplicate existing functionality
2. make sure there are no license violations
3. check for security holes
4. ensure there's a general understanding of Drupal APIs in place (e.g. they're not printing out tags rather than using Form API)

For such cases, there isn't any reason to install the module.
omissis correctly uploaded a single module, as we review a single theme/module per applicant.

@omissis: If iso_639.module is included in the same archive, that is fine; if I review the code, I will report just what I find wrong in the main module. There is no reason to open a different issue for the other module (which is not possible, then); you can create the other project when you will have your CVS account.

Hey omissis, sorry for the delayed reply - I've whacked the crux of what I was doing into this post: http://www.akb.id.au/2010/epub-files-with-php/

Probably best to just go with what you've got though, and we can discuss things in the issue queue for your new module.

Was this module published as a project? I'm very interested in helping out. Some folks just came out with a wordpress plugin doing something similar to this: http://anthologize.org/

Hi,

I'm very interested in this module as well. What's the current status?

in the meanwhile I added some new features to the module

You should upload the new archive here too. It doesn't make sense to review code, when the code is already changed; there are chances reviewers would report something that has been already changed.

• The points reported in this review are not in order or importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
• Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

1.   drupal_set_message(st(
       "ePub settings are available here: !settings.",
       array(
         '!settings' => l(
           t('Administer > Content management > ePubs'),
           'admin/content/epub/settings'
         ),
       )
     ));
   

   l() is never used together t(). See the documentation for t(), where such code is reported to not be correct.
   The function t() is then available to hook_install(), and hook_uninstall().

2. Menu callback descriptions, and titles are not passed to t().

3.     db_rewrite_sql(
         "SELECT `b`.`nid` FROM {node} AS n
         INNER JOIN {book} AS b ON `b`.`nid` = `n`.`nid`
         WHERE `b`.`nid` = `b`.`bid` AND `n`.`title` = '%s'"
       ),
   

   The character ` is specific to MySQL; there is no need to use it, as the database API functions escape the field names when necessary.

4.   $result = db_query(
       "INSERT INTO {epub} (
         `nid`, `vid`, `bid`, `author_name`, `language_code`, `identifier`,
         `identifier_type`, `publisher_name`, `publisher_site`, `creation_date`,
         `rights`, `source_url`
       ) VALUES (%d, %d, %d, '%s', '%s', '%s', '%s', '%s', '%s', '%s', '%s', '%s')",
       $node->nid,
       $node->vid,
       $book->nid,
       $node->author_name,
       $node->language_code,
       $node->identifier,
       $node->identifier_type,
       $node->publisher_name,
       $node->publisher_site,
       $node->creation_date,
       $node->rights,
       $node->source_url
     );
   

   Why isn't the code simply using drupal_write_record().

5.   if (!$result) {
       node_delete($node->nid);
       drupal_set_message(t('Error occurred during the creation of the ePub.'), 'error');
       drupal_goto('node/add/epub');
     }
   

   I have never seen an implementation of hook_insert() calling drupal_goto().

6.   $result = db_query("SELECT * FROM {node} WHERE `type` = 'epub' ORDER BY `title`");
     $headers = array(t('ePub'), t('Operations'), '', '');
     $rows = array();
   
     while ($epub = db_fetch_object($result)) {
       $rows[] = array(
         l($epub->title, "node/$epub->nid"),
         l(t('Download'), "node/$epub->nid/epub"),
         l(t('Edit'), "node/$epub->nid/edit"),
         l(t('Delete'), "node/$epub->nid/delete"),
       );
     }
   
     return theme('table', $headers, $rows);
   
   

   The code is showing nodes data without to verify if the user has access to the node. This is considered a security issue.

7.     form_set_error('epub_custom_chapter_size', t('Chapter size can\' t be less than 1 kB'));
   

   Avoid to escape the string delimiter inside of strings, especially the ones that are passed as argument of t().

8. The module doesn't remove the Drupal variables that defines.

1. function epub_install() {
     // Create tables.
     drupal_install_schema('epub');
   
     drupal_set_message(st(
       'ePub settings are available here: <a href="!settings">Administer > Content management > ePubs</a>.',
       array('!settings' => url('admin/content/epub/settings'))
     ));
   
     drupal_set_message(st(
       'Also do not forget to check permissions for ePub contents: <a href="!permissions">Administer > User management > Permissions</a>.',
       array('!permissions' => url('admin/user/permissions'))
     ));
   }
   

   The code is still using st(); the placeholder for URLs is @.

2. function epub_uninstall() {
     db_query(db_rewrite_sql("DELETE FROM {menu_links} WHERE module = 'epub'"));
     db_query(db_rewrite_sql("DELETE FROM {node} WHERE type = 'epub'"));
     variable_del('epub_custom_chapter_size');
     drupal_uninstall_schema('epub');
     menu_cache_clear_all();
   }
   
   

   There is no need to use db_rewrite_sql() in hook_uninstall(). The module should not then delete the nodes that use the content type it defines; that is not done from any Drupal core modules.

3. The version line needs to be removed from the .info file.

I will review the code tomorrow morning, but I think it is good to go.

Remember to remove any debug code.

Thank you for your contribution! I am going to update your account.

These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.