[D6] Filefield Audio Insert

Can you elaborate on the differences between this module and the Better formats module, which seems to provide similar functionalities (based on your brief description).

Hello, and thank you for applying for a CVS account.

The main reason that I really would like a CSV account is for the NPR module that I have developed.

In that case, you should use that module for the CVS application.

Please decide which of the 2 submitted modules should be up for review, so we can focus on one in particular.

1.   require_once dirname(__FILE__) . '/filefield_audio_insert_widget.inc';
   

   There is a Drupal function to use in such cases.
   It's not a good idea to unconditionatelly include a file; it's better to include the file when necessary, or use the Drupal features that allow to report to Drupal which files need to be loaded in some cases (before a menu callback or a theme implementation is invoked, for example).

2. Check you are using the correct indentation character; the currently set one is the tab character, which is not the one suggested in http://drupal.org/coding-standards.
3. Some of the files use the short PHP tag <?, when it is suggested to use the standard one (<?php).

4. 		$form['audio_insert']['audio_insert_image_fieldset']['default_image_thumbnail'] = array(
   			'#type' => 'markup',
   			'#value' => t('<img src="/'.$widget['default_image']['filepath'].'" width=150 />'),
   		);
   

   The first argument of t() is a literal string, not a dynamic value like the value returned from a function, or the concatenation of two strings.
   In that case is not even necessary to pass the string to t(). The purpose of the function is to translate strings, and HTML tags are not translated.
   Since the argument passed to the function is not the correct one, the argument will not translated, though.

5. See http://drupal.org/coding-standards to understand how a module should be written; in particular see how the code should be formatted (control structures and functions).

Hi,
I installed your module on a fresh Drupal 6.20 installation.

This is not a complete review, however I did find some issues that are easy to fix.

1. I ran you source code through Coder and found that you have a number of formatting issues with the file listing filefield_audio_insert_widget.inc
2. These are couple "take or leave them" suggestions :) -- Consider including a small audio file that folks can use to test their installation. Most sites are going to have a very small php file upload limit and they might be scratching their heads on how to increase it. If you supply folks (or supply a link) to an audio file you know they can test with. My guess is you will save yourself a lot of installation support work. Along with that I'm implying it would really nice to give folks an extended installation and setup guide. I had to scramble to find an audio file that was under 2 megs. Another example, if you don't set the input filter the "full html" your player is not going to render. In testing, even I got confused for a moment. Again, all optional. Just a thought.
3. Going along with point 2, I would also consider implementing hook_help() to provide administrators with some guidelines.
4. On my installation, I was getting issues with paths. After much debugging I found that the path on both the server side and client script side needed the Drupal base path.
   • I used Firebug and found that you 2 javascript listing were producing http 404 errors. All I did was change
     

        '/filefield_audio_insert/embed/'
     

     to

       Drupal.settings.basePath + 'filefield_audio_insert/embed/'
     

     That was in filefield_audio_insert_all.js . And the in filefield_audio_insert.js I changed:

     'filefield_audio_insert/file_link/'
     

     to

     Drupal.settings.basePath + 'filefield_audio_insert/file_link/'
     

   • On the server side, same issue with the path. I changed in the file listing file_field_insert_audio.module method ilefield_audio_insert_embed(). Changed:
     

     $embed = theme('mp3player', $player, '/' . $file->filepath);
     

     to

     $embed = theme('mp3player', $player,  base_path() . $file->filepath);
     

   Those changes got the players to render and play the audio files.

5. I didn't realize at first that you were inserting the audio player in the body of the node. Therefore, if the user has entered text in the body before invoking the "Insert Audio" button, the player is inserted right after the last text character. Might be nice to prepend a   or something to give it some padding between the player and text.
6. This may be redundant with my comments about an extended setup. If you click on the "Insert Audio" button multiple times, your module inserts multiple players for the same audio file (in a single node body). I don't know if that is by design?, or a bug. If it's design. then I would let folks know the player is not going to be replaced when editing a node.
   

I hope this helps.

I live in Chicago myself. A couple of my friends have been featured on "vocala". Thus I really wanted to reach out and help a fellow Chicagoan who is doing great work!

I gave the rest of your code a cursory review, didn't see any other issues. Please see if any of the changes I suggested make sense. The folks who review the Druapal cvs application truly have a daunting task. I'm also in the application queue. My recommendation is, help the folks out on Drupal.org by reviewing some of the other applications. I know that they are trying to recruit more folks to do the reviews:) Therefore, why not help out and review some of your fellow applicants : )

Again, hope that helps, Lorin.

I am changing status as per previous comment.

Hi,
I reviewed the version posted in comment #23 on a Drupal 6.20 installation. Nice job, but still a few issues:

• Still getting the 404 errors as detailed in my comment #21. You added the Drupal base bath to the server side code, but not to the client side javascript JQuery. On my installation I again applied the changes detailed in comment #21 to the javascript listings. Bingo, that cleared the 404 errors and got the player to work.
• Please make sure you set Coder (Selection Form) to "minor" and "Internationalization". Once those options are set, Coder still reports formatting issues and strings not being passed to t(). Easy to fix, once identified.
• Your README.txt file has a typo in it. Please change "suer" to "sure"

Again, nice job, really close.
Hope that helps.
Lorin

Hi,
I reviewed the version of your module posted in comment #27. Please check the last line of your README.txt file (so minor, I'm giving this a go :) ).
.....
All issues fixed. Coder reports no issues. Module works as expected. This code passes my inspection.
....
Great job.
.....
Hope that helps.
Lorin

Please read all the following and the links provided as this is very important information about your CVS Application.

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:Migrating from CVS Applications to (Git) Full Project Applications.

• If your application has been "needs work" (or "postponed (maintainer needs more info)"), your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.
• if the status of this application is a different one, it will be changed to "postponed"; you will be able to reopen it by following the instructions in the above link.

pwaterz - Your audio insert module was exactly what I was looking for. But...can't get it to work.

I've installed your module and mp3player. If I print the audio insert filed on the node view set to display via mp3player, it works just fine. However, the actual file inserted into the node body just displays "You may need: Adobe Flash Player."

Any thoughts?

I tired your module from comment # 27 and I am having the same exact problems as Poieo. I tried rolling back my version of the mp3player module with no luck. I am also assuming the field insert is supposed to work when it is set to hidden via the content type display EG like the insert module.

Thanks!

Drupal.org just finished a redesign, the community just launch Drupal 7, they just migrated to Git, and this week is DrupalCon. So you may need some extra patience for them to respond to your request for your module review...they'll get to it.

Thanks again for the great contribution!

@pwaterz - Any timeline for this fix?

Obviously no pressure to do so. But I have to make a decision between this module or taking another route asap.

Works like a charm! Thanks for your work. Look forward to your contributions.

Working great here as well... Thank you pwaterz!

Hi.
I took a snapshot from git. Really minor:

• Coder reports one formatting isssue:
  • Line 49: Use an indent of 2 spaces, with no tabs
    $embed = str_replace('mp3player_1', 'mp3player_' . $delta, $embed);
• Please remove the CVS markers on your file listings (// $Id$).

I assume the CVS markers issue is the reason why the status on applications like yours was changed from "Reviewed and Tested by the Community" to "needs review" :) Thus please take care of formatting issues and I'll be happy to change the status back to "Reviewed and Tested by the Community" .

Hope that helps.
Lorin

Hi,
I inspected the latest snapshot of your git sandbox. You removed the CVS marker from one file listing (you main .module file). However, you did not remove the CVS markers from the rest of the file listings.

Hope that helps.

@pwaterz - Not sure it's appropriate to ask for a feature request when this isn't actually a module yet. But, if you could get this working with FileField Sources so you could attach files then insert them, that would round this thing out.

I'm happy you created this module, it's exactly what I needed. I have one problem. I followed the installation instructions and watched the video. All went fine, but when I load up the page, the player says "file not found". Is there a setting I'm missing?

Somehow this application fell off the radar and got marked "fixed" by the requester (pwaterz) but it never was even RTBC. So setting to "needs review" and perhaps this can get done.

The sandbox somehow became a project and is at http://drupal.org/project/filefield_audio_insert

This was RTBC after a reasonable review in #28, right before the git migration. As a result, should be RTBC.

Git vetted user role granted.

Thanks for your patience in this, and we welcome your contributions. Thanks so much.

And don't forget to come back and review somebody else's application every day :-) We have a terrible backlog.

Just correcting the category.

pwaterz , what happens to your 2 extensions to filefield_sources module?
Filefield sources taxonomy and filefield sources node title.

Please give more info. I need exactly this kind of extension

I am giving credits to the users who participated in this issue.