omissis [omissis]

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.

First of all thank you for checking the module :)
I am not sure about the issue you' re having with the iso_639 module: it' s included in the zip file, but if I got it well from kiamlaluno's comment that' s not the correct way of submitting a module.
So should I open another Issue for iso_639 itself?

@alexkb: it would be great to get the ideas you were talking about into code, perhaps we can kinda merge the two modules or talk about future developments.

ok, I will wait for news then. thx :)

I am going to read it asap, thanks :)
Btw I just sent you an email so we can discuss privately about it without generating too much noise here.

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. <?php
  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. <?php
    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. <?php
  $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. <?php
  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. <?php
  $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. <?php
    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. <?php
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. <?php
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.

done, thanks. Here it is the path of the project: http://drupal.org/project/epub