audio.js

Cool module, this will a big help to people building websites with MP3 audio (I hope audio.js will support ogg soon).

Code looks good after a quick manual review, and the module works well.

A few minor comments:

• Typo in audio_js_help(): 'theme' should be 'themed'.
• Module names in the .info file usually start with a capital (so Audio.js).
• A suggestion (but not a blocker to becoming a full project): since the audio.js library is required for this field formatter to work, you may want to consider displaying a message after the module was installed, directing the user to download and install the library in case the library wasn't found.

Other than that, this looks good!

Hi,

Another manual code review:

• General

I would put the js files in a /js folder, to be consistent with a bunch of other Drupal modules, but that's just a suggestion.

• audio_js.info

Your module uses the File field, you should add a dependency to the file module.

• audio_js.install

l5: the comment mentions the colorbox module, shoul be yours I think :)

• audio_js.module

l15: This doesn't look like help to me, merely a description. If like me you're lazy you can implement the hook_help like this:

/**
 * Implements hook_help().
 */
function audio_js_help_help($path, $arg) {
  switch ($path) {
    case 'admin/help#audio_js':
      // Return a line-break version of the module README.txt.
      return check_markup(file_get_contents(dirname(__FILE__) . "/README.txt"));
  }
}

l127: Typo


Otherwise, you should use the renderable arrays and theme functions available in d7, instead of using a big $output string.
I'm thinking about theme_item_list(), l(), and '#prefix' and '#suffix'.

Some resources about that:
http://drupal.org/node/930760
http://randyfay.com/content/drupal-7-render-arrays-and-new-render-example
http://drupal.stackexchange.com/questions/10788/rendering-theme-item-lis...

• audio_js_playlist.js

Comments should follow the same conventions than php comments, i.e should be proper phrases with a capitalized letter and an ending dot. And the file's purpose should be commented at the beginning of the file.
Some empty lines are useless.

• audio_js_init.js

The file's purpose should be commented at the beginning of the file. Useless empty lines too.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

I confirm my review has been taken in account, I do not have more remarks on the code :)

I ran the Project application checklist a second time, and for me the project is ok.

Ensure that the application contains a repository and project page link.
=> OK

Ensure it´s not a duplication.
=> already existing audio.js project has been submitted to project application, but never finished, so OK

Ensure the user does not have multiple applications.
=> User has submitted an application in the past but the amount of code was too short, the project has been promoted but user is not a vetted git user, so OK

Ensure the repository actually contains code.
=> OK

Ensure the applicant is working in a version specific branch.
=> OK

Ensure branches and tags are following naming conventions.
=> OK

Ensure the repository does not contain a ‘LICENSE.txt’ file.
=> OK

Ensure the repository does not contain any 3rd party (non-GPL) code.
=> OK

Ensure the project page contains detailed information.
=> Informative enough.

Ensure the repository contains a detailed README.txt.
=> OK

Ensure the code contains a well-balanced amount of inline-comments.
=> OK

If not done yet run an automated review and ensure there are no major issues.
=> OK (http://ventral.org/pareview/httpgitdrupalorgsandboxjtphelan1899818git)

Ensure the applicant is using Drupals API correctly.
=> OK

Ensure the project does not contain any security issues.
=> OK

I tested this with a fresh pull of the audio.js library.

Manual review:
1. The path in audio_js_init doesn't seem to be correct. I believe it should be:
$audio_library = libraries_get_path('audiojs') . "/audiojs/audio.min.js";
2. If I understand the Drupal docs on hook_init, you shouldn't use it to include js on every page. Instead either add a declaration to your .info file (scripts[] = sites/all/libraries/audiojs/audiojs/audo.min.js), or better yet add it in audo_js_field_formatter_view() so it's really only added if it is needed.
3. In audio_js_field_formatter_settings_summary(), you put 'Preload', 'Autoplay' and 'Loop' in t(), but not the actual settings for each of those. I think you want to use a placeholder within t() for the setting, and then add the setting like so:

// Set a variable to the setting value.
$setting = $settings['audio_js_preload'] ? 'true' : 'false';
// Output the text.
$summary = t('Preload: @setting', array('@setting' => $setting)) . '<br />';

4. In audio_js_field_formatter_view(), I believe you want to use check_plain() on the variables in your #markup form elements and t() for your $label variable used as the title of your l() function.

On 4. I don't think you need to use check_plain on the url since file_create_url is sanitizing its output already. And, yes, I think that is how you would treat $label in t(). It looks a little weird, but I'm not sure how else you would deal with it. BIG DISCLAIMER: I am not a security expert, so you might want to hit up others in the community to get their take ;-)

Nice module, on the whole couldn't find a lot to fault.

One issue I found is in the implementation of hook_requirements. libraries_get_path returns a path not an array, so the following test always returns false and the library is reported as not found on the status page, even if it is present:

$library = libraries_get_path('audiojs');
if (empty($library['installed'])) {

Nice to haves:

Was a little confused in the README.txt notes under INSTALLATION that the Drupal libraries folder should be in the root of the site. It'd be a bit clearer if you specify the path from the site root. e.g. sites/all/libraries/audiojs/audio.min.js

Using the playlist mode the track list is currently output as a ordered list which is nice; though if I was theming I'd prefer it to have a suitably named css class on the list element (audiojs-playlist?)

Also in playlist mode, the playlist loops any there doesn't seem to be a way to disable it though this may be a limitation of the library.

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/audio_js.module
  --------------------------------------------------------------------------------
  FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
  --------------------------------------------------------------------------------
   106 | ERROR | Whitespace found at end of line
   108 | ERROR | Line indented incorrectly; expected 2 spaces, found 3
   111 | ERROR | Closing brace indented incorrectly; expected 3 spaces, found 2
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. "theme_item_list(array(...": do not call theme_item_list() directly, use theme('item_list', ...) instead.
2. audio_js_field_formatter_view(): That should not contain any actual markup, theme functions for the two types would be nice, so that themes can easily override the markup.

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

no objections for more than a week, so ...

Thanks for your contribution, jphelan!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here 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 lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.

hi everyone

i am having an issue in which i am not able 2 play sound on my webpage using audio.js....
i have enabled the module + pasted js files from http://kolber.github.io/audiojs/ in library....i am still not getting any audio player display on my webpage

this module just allowed me to add FILE field in content type

if anyone having brief knowledge about this ....he or she can reply !!!!!!!!!!!!!

thanks buddies.....