VladRM [vladrm]

1. version = 6.x-1.0
   

   That line should be removed from the file.

2. function videopublishing_init() {
     module_load_include('inc', 'videopublishing', 'videopublishing_api');
     module_load_include('inc', 'videopublishing', 'videopublishing_forms');
     module_load_include('inc', 'videopublishing', 'videopublishing_helpers');
   }
   

   hook_init() is called only on pages that are not cached; depending on the case, this could cause some problems, especially if the code always needs the code present in the included files.

3.   if (in_array($node->type, $vp_content_types)) {
       switch ($op) {
         case 'view':
           if (!$a3 || variable_get('videopublishing_show_in_teaser', 0)) {
             $nvs = videopublishing_get_node_videos($node->nid, $node->vid);
       	  if ($nvs) {
               $embed_xml = videopublishing_get_stream_embed_code($nvs[0]['video_id'], $nvs[0]['player_id']);
               $embed_code = '<div class="vp-node-video">'. (string)$embed_xml .'</div>';
               
               if (variable_get('videopublishing_show_video_title', FALSE)) {
                 $info_xml = videopublishing_get_video_info($nvs[0]['video_id']);
                 $vp_title = '<h3 class="vp-node-video-title">'. (string)$info_xml->title .'</h3>';
               } else {
                 $vp_title = '';
               }
               
               $node->content['videopublishing_nodeapi_field'] = array(
                 '#value' => $vp_title . $embed_code,
                 '#weight' => variable_get('videopublishing_field_weight', 10),
               );
             }
   		}
         break;
   

   See the Drupal coding standards to understand how a module code should be written.

4.       case 'presave':
         break;
   

   If the code is not doing anything when $op is equal to presave, then there is no need to write that code.

5. /**
    * Implementation of hook_perms().
    * 
    * Displays VideoPublishing specific input fields
    * in the node/add and node/<nid>/edit forms
    */
   function videopublishing_form_alter(&$form, $form_state, $form_id) {
     // ...
   }
   
   It isn't the implementation of hook_perms.
   </li>
   <li>
   <?php
     $schema['videopublishing'] = array(
       'description' => t('The base table for videopublishing.'),
       'fields' => array(
         'nid' => array(
           'description' => t('The primary identifier for a node'),
   

   Schema description should not be passed to t().

6.   try {
       $factory = new Api_Methods_Factory(API_USER, API_USER_KEY, Api_Methods_Factory::XML_RESPONSE);
       $method = $factory->factory(Api_Methods_Factory::PLAYERS);
       
       $xml_string = $method->getAll();
       $xml = new SimpleXMLElement($xml_string);
       return $xml;
     }
   

   If you are using feature specific of PHP5, then you should declare the dependency of the module from PHP5, as Drupal 6 still support PHP4.

7.   else {
       // do nothing
     }
   

   If it doesn't do anything, why is the ELSE-statement present?

8. 	$form['#validate'] []= 'videopublishing_upload_form_validate';
       $form['#submit'] []= 'videopublishing_upload_form_submit';
   

   The name of the functions are not the one defined from the module.

9.   $ct = explode(',', variable_get('videopublishing_content_types', NULL));
     foreach($ct as $v) {
       $options[$v] = $v;
     }
   

   It would be better to use variable_get('videopublishing_content_types', ''); I am not sure explode() is able to handle NULL.

10.   $form['videopublishing']['player_id'] = array(
        '#type' => 'select',
        '#title' => t('Select default player'),
        '#default_value' => $default_vp_player_id,
        '#options' => videopublishing_settings_form_get_player_options(),
    	'#description' => t('You can create your own custom players through the Video Publishing GUI. See http://admin.videopublishing.com/players/'),
      );
    

    The options used in a form field should present to the user a translatable string.

11.     '#description' => t('You can create your own custom players through the Video Publishing GUI. See <a href="http://admin.videopublishing.com/players/'" title="http://admin.videopublishing.com/players/'" rel="nofollow">http://admin.videopublishing.com/players/'</a>),
    

    Use the correct placeholders.

For the previous point #3, the code doesn't use the correct indentation.

1. If you need to include files, you could put the code that includes them outside any function; in that way, the code will be included when the module is loaded.
2. What I was trying to say is that the functions are present in a file that is not videopublishing.module; if that file is not being included (see my comment about hook_init()), then the functions are not found.
3. The coding standards report also which functions should be used from the module; the code is still not following that point.

1. The path used for require_once() is relative to Drupal root directory; therefore the files are not found as they are in the module directory.
   It should be better to use module_load_include().
2. The coding standards do suggest a category of functions that the modules need to use.
   To notice that the coding standards are not only a book page. At the bottom of the first page you find the links to the child pages; one of the those child pages reports which functions should be used (the page says you should use these replacements when you are programming for Drupal).

1.   t((string)$player->title)
   

   The first argument of t() should be a literal string; differently, the string will not be added in the translation template, and who translates the module would be able to translate that string. Using the Drupal translation system it is not possible to translate dynamically generated strings (if they don't use t() placeholders).

2. function videopublishing_stream_id_label_mapper($stream_id) {
     $stream_id_map = array(
       1 => t('Standard'),
   	2 => t('Enhanced'),
   	3 => t('HD'),
     );
   

   The indentation character is not correct.

3. function videopublishing_get_all_video_options($null_option=TRUE) {
     $vids = array();
   

   The code is not formatted as suggested by the coding standards.

4.   require_once 'modules/node/node.pages.inc';
   

5. All the content of the directory VideoPublising should be removed.

The library is available from a third-party site; in that case, it should not included in Drupal.org CVS.

Also, files from other projects should not be committed in Drupal.org CVS; if you would create (in example) a jQuery plugin, you should probably make it available from a different server.

1.   define('API_BASE_DIR', dirname(__FILE__) .'/VideoPublishing');
   

   Drupal modules normally use drupal_get_path().

2. Drupal.org CVS doesn't add directories that don't contain any files; if you want the directory VideoPublishing to appear in CVS (and then in the tarball archive generated by the packaging script), you need to add a file README.txt.
3. There are some commented code lines that should be removed.

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.

any updates on this module? I submitted some issues on this project. Also, are there any plans on extending this module? because right now it doesn't uses the full API of the videopublishing.com