daveeddydotcom [daveeddydotcom]

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/theme; for modules it should include also a comparison with the existing solutions, while for themes a screenshot is also required.

Thanks for the reply.

When I ask for more information is because the requirements speak of few paragraphs instead of sentences.
You effectively added more information, as you didn't before reported which data are hosted on Ampache, and how the module grabs those data in.

• The points reported in this review are not in order or importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
• Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

1. Schema descriptions are not passed to t().
2. Deleting Drupal variables using a query that matches any Drupal variable with a name that starts with the module name would remove also the Drupal variables of other modules.
3. Menu descriptions, and titles are not passed to t(), as that is already done by Drupal core code.
4. The permission access administration pages is too generic to be used in this case; if the code should use an existing permission, then it should probably use administer blocks.

5. 			$blocks[0]['info'] = t("$title");
   

   The first argument of t() is a literal string, not a concatenation of strings. The script used to create the translation template is not able to handle any dynamic value, even in the case of code similar to t($variable); this means that if the argument of the function is not a literal string, it will not appear in the translation template.

6. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted; how Drupal variables, global variables, constants, and functions defined from the module should be named.
   Check also you are not using tabs to indent the code.
7. Comments should not be longer than 80 characters per line.

8. 	if ($artwork_url && $show_album_art) { // show the album art
   		$google_link = 'http://www.google.com/search?q=' . _add_pluses($song_info['title']);
   		$msg .= "<div style=\"float:left;\"><p>
   			 <a href=\"$google_link\" target=\"new\">
   			 <img style=\"padding:4px;border-style:solid;
   				border-color:#ddd;border-width:1px;\" 
   				height=\"150px\" width=\"150px\" src=\"$artwork_url\" />
   			 </a>
   			 </p></div>";
   	}
   	$msg .= '<div style="float:left;">';
   
   

   The code should use a theme function to output HTML.

9. 	drupal_set_message(t('Ampache album art cache cleared.  ' . $i . ' files deleted.'));
   
   

   Use t()-placeholders.

10. /** 
     * Implementation of hook_validate() for the "account" page
     */
    
    

    It's not the implementation of hook_validate().

11. function _ampachenowplaying_valid_url($url) {
    	return preg_match('|^http(s)?://[a-z0-9-]+(.[a-z0-9-]+)*(:[0-9]+)?(/.*)?$|i', $url);
    }
    
    

    Why isn't the code using valid_url()?

Please read the following links as this is very important information about CVS applications.

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for these applications. Please read Migrating from CVS Applications to (Git) Full Project Applications and Applying for permission to opt into security advisory coverage on how this affects and benefits you and the application process. In short, every user has now the permissions necessary to create new projects, but they need to apply for opt into security advisory coverage. Without applying, the projects will have a warning on projects that says:

This project is not covered by Drupal’s security advisory policy.