lucuhb [lucuhb]

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.

As per http://drupal.org/cvs-application/requirements, the motivation message should be expanded to contain more details about the features of the proposed module, and it should include also a comparison with the existing solutions.

I will make a review tomorrow morning (which means not before 10 hours from now).

1. Remove the code already hosted in third-party sites, or that are not part of other projects.

2. function node_to_odt_access ($node) {
     global $user;
     // Check basic permissions first.
     
     $access = (user_access('export node to odt') );
     // Make sure the user can view the original node content.
     $access = $access && node_access('view', $node);
     // Check additional conditions
     $access = $access && (node_to_odt_is_permitted($node->type) && filter_access($node->format) );
     // Let other modules alter this - for exmple to only allow some users
     // to export specific nodes or types.
     
    
     drupal_alter("node_to_odt_access", $access, $node);
     return $access;
   }
   
   /**
    * Check menu access to export a node.
    */
   function _book_node_to_odt_access ($node) {
     global $user;
     // Check basic permissions first.
     
     $access = (user_access('export book to odt') );
     // Make sure the user can view the original node content.
     $access = $access && node_access('view', $node);
     // Check additional conditions
     $access = $access && (node_to_odt_is_permitted($node->type) && filter_access($node->format) );
     // Let other modules alter this - for exmple to only allow some users
     // to export specific nodes or types.
     $access = $access && isset($node->book) && ($node->nid == $node->book['bid']) && $node->book['has_children'];
   
     return $access;
   }
   

   The code can be better written, and optimized. Also, the comment Let other modules alter this - for exmple to only allow some users is probably not correct (or it's not correct the code following that comment).

3. function node_to_odt_token_list($type = 'all') {
     if ($type == 'Node to odt filename') {
       $tokens['Node to odt filename']['title']      = t("Title of the node (for book, title of the node which is the book parent).");
        $tokens['Node to odt filename']['timestamp']     = t("The timestamp when the file was generated.");
         $tokens['Node to odt filename']['nid']     = t("The node id.");
       return $tokens;
     } 
     
    
   }
   

   In general, avoid using extra spaces where not necessary.

4. function node_to_odt_node_type($op, $type_obj) {
   
     switch ($op) {
       case 'delete':
         variable_del('node_to_odt_reset_'. $type_obj->type);
         break;
       case 'update':
         if (!empty($type_obj->old_type) && $type_obj->old_type != $type_obj->type) {
           if (variable_get('node_to_odt_reset_'. $type_obj->old_type, FALSE)) {
             variable_del('node_to_odt_reset_'. $type_obj->old_type);
             variable_set('node_to_odt_reset_'. $type_obj->type, TRUE);
           }
         }
         break;
     }
   }
   

   The code is not updated with the latest development in Drupal; see hook_node_type().

5. Strings used in the user interface should be in sentence case.

1. require_once() should be replaced with a call to module_load_include().
2. Any calls to debug functions should be removed (see error_log()).

3. 		$searchTitle=array(
   				'<h1>',
   				'</h1>',
   				'<h2>',
   				'</h2>',		
   				'<h3>',
   				'</h3>',
   				'<h4>',
   				'</h4>',		
   		);
   

   The code is probably using the wrong indentation (it should be 2 spaces), and the reported code can be better formatted (there is no reason to write a single string per line).

4. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted.
5. The code calls node_load(), and it uses the properties of the node object without to verify if the user has access to the node (in example, the node could be unpublished, and the user should not see the content (which means not even the title) of that node. This could be a security issue.

1. The code should use a single assignment instruction, rather than using three.
2. "Book Filename pattern", and "Node To Odt" (but not "Odt") are not in sentence case.

3.     '#title' => t($label),
   

   The first argument of t() must be a literal string; differently, the script that extracts the string to translate to create the translation template will not be able to extract the string, which would not be translatable (if not in the case another module uses the same exact string, but it's rather difficult it happens, when the string is dynamically changed).

I'd like to bring to your attention a project of mine : XHTML2ODT : http://gitorious.org/xhtml2odt

It's an XSLT-based conversion from (X)HTML to ODT XML that I've used on already two ODT export plugins : one for Dotclear (blog engine in PHP) and one for Trac (project manager in Python). Since this Drupal export plugin seems to be a similar case, you'll probably be interested.

All the conversion is done in XSLT, in a very generic way, but it's still possible to customize it. A template ODT file is used for styling. The only thing to implement is the integration with the application you want to build an export plugin for, in our case Drupal.

I'd be very happy to help you with it should you choose to use my project, but unfortunately I don't know Drupal. So you'll have to do the Drupal specific bits, but I can handle the basic XHTML to ODT XML conversion.
It's even possible to do Drupal-specific conversions in the XSL if needed (for example to convert known CSS classes to equivalent formatting).

I hope I'll be able to help you.

Interesting module. I'm wondering whether it could be made to build on top of http://drupal.org/project/nd, by treating the doc export as another type of node display.

@lucuhb: would you mind if I take this code forward, giving you credit for your work obviously.