LongTail Video [longtailvideo]

On the whole this looks good. There are a few points though:

• You can't include the swfobject.js file as this has an MIT license. Non-GPL code cannot be included in Drupal CVS (http://drupal.org/node/422996). Also, is the JW Player version you are including distributed under GPL 2? If not, it can't be included in Drupal CVS either.
• You are using variable_set() but you have no install file with the uninstall hook implemented to remove entries that get written to the variables table. You need to make sure you clean up after yourself.

Some minor issues to point out:

• There is some inconsistency with the Drupal coding standards for variable names, or rather inconsistent adherence to the standard. Variables such as $fVarArray and $newName should be consistent with your usage in the rest of the code.
• I am seeing '‚Ñ¢' after references to JW Player. I'm assuming this is supposed to be the trademark symbol? It has got mangled in encoding - try using the HTML entity ™ instead.

The licensing issue is major and needs to be resolved. I would also say implementing an uninstall to clean up what you write to the variables table is also necessary. For example, I'm not even going to install this to see how well it works until I know I can remove it cleanly.

The license restrictions apply to what can be hosted on cvs.drupal.org. Provided the actual licensed files are not hosted by Drupal you can include them. I think using the Google CDN would be acceptable, though I would suggest making this configurable to allow users to download their own local copy if they want/need to. That's just a suggestion for something that could be added later.

For your player, trying to do an automated download and install would trigger some concerns about security and also filesystem permissions on shared hosting environments. Allowing the web server access to modify module code opens up a whole can of little security worms.

I'd want someone a little more heavyweight than me to assess any attempt at downloading and installing code from a module installer. If you attempted that I think there would need to be very close scrutiny for security issues. I might suggest looking at the Plugin Manager module: http://drupal.org/project/plugin_manager - perhaps doing something in collaboration with this project would be the right approach for streamlining the install process and in a way that other similar modules could leverage.

On the coding standard issues, it's obvious that you have used variable naming standards in jwplayer.module except for a couple that got missed. For external library files that will be used in other contexts outside of Drupal there is no Drupal coding standard requirements, obviously, but they should be treated as external libraries. The recommended way of integrating Drupal modules with such libraries is to install them in a separate 'libraries' directory on the same directory level as the contributed 'modules' directory. E.g. 'sites/all/libraries'. That way other modules can easily share your framework files even if the rest of your module is not installed, without having their own copy local to their module.

With the framework libraries all licensing requirements for hosted files would obviously apply. If the framework libraries are GPL 2 even if the player itself isn't it can be hosted on cvs.drupal.org.

On the positive side. I really like your player and the additional capabilities that it gives webmasters. The fact that you are including a free version of the player and trying to make things easy for your users indicate to me that this would be a very well maintained and supported project.

atheneus is correct in what he says; I will just add a note about coding standards.

All PHP code hosted in Drupal.org CVS should be written following Drupal coding standards; if there would be two different standards, then a module could not respect its namespace (simply because the other standard doesn't contemplate such thing that is clearly contemplated by Drupal), and that is not something we want for module / themes hosted in Drupal.org.

With regard to the security of file management practices applicants for a CVS account are expected to be aware of all security best practices and to follow them. A couple of handbook pages that you should refer to are:

https://drupal.org/node/117054 - File management security best practices
https://drupal.org/node/244924 - Securing file permissions & ownership

With regard to the management of external libraries, there is a project currently in active development to handle just that:

http://drupal.org/project/libraries

Provided your code is secure and you are handling external libraries sensibly with a view to working well with other modules, then there's no real problem.

One thing to point out with regard to file permissions, if you place a binary in a directory where PHP code will be stored, this could be a problem if you require that directory to be writable by the web server, it would make it difficult for the site owner to secure their site in accord with the recommended best practices. Please be sensitive to that issue, not just the security of your own code, but the overall security of a complete site installation.

Any interest in taking over http://drupal.org/project/flvmediaplayer code? I wrote this to provide support for the JW player. There is quite a bit there that could be reused I think. There are also hooks to allow other modules to provide content directly to the player.

oh, and on the swfobject question, you can use the jquery plugin module (http://drupal.org/project/jquery_plugin) and use flashembed. I did an implementation of this for the JW player like this:

/**
 * Provides display formatting of a flash player.
 */
function theme_my_module_flash_player($variables = array()) {
  if (! isset($variables['params'])) {
    $variables['params'] = array();
  }
  if (! isset($variables['flashvars'])) {
    $variables['flashvars'] = array();
  }

  // Add the flash embed library
  drupal_add_library('jquery_plugin', 'flashembed');

  // The filepath has to be absolute
  if (! strpos('://', $variables['flashvars']['file'])) {
    $variables['flashvars']['file'] = file_create_url($variables['flashvars']['file']);
  }
  if (! strpos('://', $variables['flashvars']['image'])) {
    $variables['flashvars']['image'] = file_create_url($variables['flashvars']['image']);
  }

  // Build the default flashvars
  $variables['flashvars'] += array(
    'width' => variable_get('my_module_flash_player_width', 320),
    'height' => variable_get('my_module_flash_player_width', 240),
    'src' => file_create_url(drupal_get_path('module', 'my_module') . '/flash/player.swf', array('absolute' => TRUE)),
  );

  // Build the defeault params
  $variables['params'] += array(
    'src' => file_create_url(drupal_get_path('module', 'my_module') . '/flash/player.swf', array('absolute' => TRUE)),
    'width' => variable_get('my_module_player_width', 320),
    'height' => variable_get('my_module_player_height', 240),
  );

  // Set a unique ID
  $id = microtime();

  // Add this player's variables to the page.
  drupal_add_js(array('my_module' => array($id => $variables)), 'setting');
  // Add the behavior JS
  drupal_add_js(drupal_get_path('module', 'my_module') . '/js/my_module.js');

  // Return the formated div
  return '<div class="my_module_player" id="' . $id . '"></div>';
}

Javascript is simple:

(function ($) {

Drupal.behaviors.MyModule = {
  attach: function (context) {
    /**
     * Check the settings array and embed SWF objects.
     */
    $('.my_module_player:not(.proccessed)').addClass('proccessed').each(function() {
      // Get the configuration from this id
      var config = Drupal.settings.my_module[$(this).attr('id')];
      // Load the flash
      $(this).flashembed(config.params, config.flashvars );
    });
  }
};

})(jQuery);

I'd like to see the other changes you've made. Also, looks like the doxygen could use some cleanup, at least by adding @file sections and wrapping comments to 80 characters. See http://drupal.org/node/1354. Additionally, we don't end php files with ?> (as you've done, for instance, in LongTailFramework.php).

Much of that module could probably be broken into smaller include files to make it lighter to load. At the very least, hook_theme allows 'file' parameters for auto-loading. Same with hook_menu(). You can also make use of http://api.drupal.org/api/function/module_load_include/6 in a few judiciously chosen places, although just moving the admin screen, admin helper, and theme functions out should do wonders.

//Drupal automatically convers URLs into <a> tags. Must strip this out.

It automatically converts URLs into a tags ONLY when going through the URL filter, which although enabled initially, may not actually happen. As written, your module appears not to honor the filter settings, which means an editor would have no recourse to override the behavior if desired. (Normally, one would simply choose another filter, if they wanted to override that.)

I'm a little concerned about the current usage of hook_nodeapi for its filter; that would normally happen through hook_filter, which would have the benefit of working automatically with comments as well, and for text fields. It would also allow admins to disable the filter for certain node types, which may be necessary for security or layout issues. However, you would need to do some trickiness to add the JS as needed; perhaps by simply adding it automatically with hook_init, which many modules take the liberty of doing. As far as the issue w/ the URL filter processing would be easily solved by adding your filter above that, so the tags are not removed inadvertently.

I think for the module to be more widely accepted and used by the developer community, it needs to expose its API a bit more. I know that's substantially more work, and may not be necessary for an initial code review. However, I would hope to at least see that in the planned road map.

I really appreciate your offering to come on board with Drupal integration; there are a lot of Drupal sites that use your player. I hope you don't take my suggestions as criticism; it's tricky to write code reviews without sounding too critical. I also recognize that you presumably come from another coding background, and I applaud your efforts: Drupal is tricky enough when its API changes every year and a half. I'm impressed by the general level of the code; you've made an obvious effort to adhere to current Drupal standards, for instance by wrapping outputted text with t() and by using Drupal behaviors in the js (although I note that it should really be namespaced as in Arthur's example above).

Finally, I'm concerned about code duplication: there are already a few modules integrating the JW Flash Player, some of which are more mature and stable. I think you might be better off taking up Arthur's offer of co-maintainership; firstly, that module is used by a large number of sites anyway, and secondly, it might help it be of more use to other modules (for instance, the Embedded Media Module already utilizes its API for embedding YouTube modules w/ the JW player).

Thanks,
Aaron Winborn

yes, that's a difficult limitation of hook_filter. there might not be a better way around it in d6. if you keep things in hook_nodeapi, you might at least configure it to use some node type settings to toggle functionality. for instance, one might want to allow videos in blog posts but not in articles. this can be even trickier with user permissions. however, if this is working off uploads, then perhaps that will be somewhat self-limiting, as file extensions and user perms are already fine-tuned w/ that. otoh, if your filter allows embedding content from anywhere (regardless of whether it's attached to a node) there might be more security issues to be dealt with.

re. the bar for approval: my personal opinion is your coding standards are up to par with what's expected from module contributors. However, I'd like to see another posting of your current progress before I'd feel comfortable marking the issue RTBC, just to see some closure on some of the discussed items. I think that the large majority of modules in the wild are perpetual works in progress, and if we required each module to be absolutely bug free and polished before release, we wouldn't have any modules available (or even a Drupal core for that matter)...

Thanks,
Aaron

Totally understood that you'd want to roll your own given what's out there. There are a few things that I'd like to see from your module from my own selfish perspective (well largely because I want to convert all the flvmediaplayer users to use your module)

* some mechanism for a "use file" option- perhaps this is on the node object, which is done on node load. I do something (albeit quite poorly) with hook_flvmediaplayer_file_use() which allows modules to declare files accessible to the player. The reason why I want to do this is that I use xspf_playlist to deliver content to the player the majority of the time (so it implements hook_flvmediaplayer_file_use()) but sometimes I want this to be overridden. Having a hook that does this is handy and makes your module very extensible- think views, cck, etc

* support external configuration - I use node/X/flvmediaplayer/PROFILE to populate config data into the player. While this is another page load, it's extremely useful for postfacto changes- especially for embeded players. I think this is one of the best features of the JW player- one that is often overlooked.

I'm happy to contribute code or hand over flvmediaplayer to do this.

I don't see anything glaring. I would suggest you utilize spl_autoload_register for autoloading classes rather than forcing the files on every page load, and I'm still wary about doing your magic in hook_nodeapi rather than as a filter or without any configuration for node types or content fields. The second would be a blocker for me using or recommending the module as it currently stands, but other users might not be so discerning, and there's nothing verboten about your method that I'm aware of.