DropIn

Hi. 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

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

Hi DWB,

I see a few issues with your code. First, you should install http://drupal.org/project/coder and fix any issues it reports. Most of what I see are whitespace issues, but there might be more. In addition to what coder will tell you, some other feedback:

• You don't need allcss_install() at all; Drupal core already sets a message when modules are enabled.
• The nbproject directory should be removed from the git repository, or at least any stable release. That's cruft for end users.
• You should remove the // $id$ everywhere. Those were important for CVS, but unneeded on Git.
• _allcss_settings_form should not start with an underscore. The underscore indicates a private method, and anything using form API can't really be private.
• hook_html_head_alter() seems like the wrong place to be adding the CSS and JS, since you're not altering the $head_elements at all. I'd probably do that in hook_init(), though someone else might know of a better place.

Hi DWB,

Sorry this has been waiting so long. We have a huge backlog on reviews and not enough volunteer hours to look at them quickly. I just looked at this again and see some issues:

• You changed the name of allcss_settings_form(), but allcss_menu() still references that function by the old name.
• allcss_init() has a parameter, which isn't used anywhere and should be removed. Also because hook_init() takes no parameters.
• Minor issue: the indentation is inconsistant. For example, the returns at the end of both allcss_settings_form() and allcss_help() are indented much further than the rest of those functions.
• allcss_uninstall() is deleting allcss_subdir, which is never set, but not deleting allcss_css_subdir nor allcss_js_subdir, which are set.

A few more (trivial) points:

1. I'd suggest adding add a README.txt file to the project repository ... See http://drupal.org/node/447604
2. The project page description could use a bit more elaboration as well, IMO. It left me scratching my head as to what this did. Then I read your application text (from above), which does a much better job of explaining the module.
3. Missing translation call on the second part of the output:
   

   $output .= '<strong>' . t('news_articles.css') . '</strong>' . ' - styles all pages under news/articles/<br />';
   $output .= '<strong>' . t('news_articles_issue-1.css') . '</strong>' . ' - only styles the page issue-1</p>';

Interesting concept, and looks well executed ... as a feature suggestion, it would be nice to have an admin setting where you define specific paths where this check is invoked (similar to the 'show this block on:' path listing on block config pages), instead of having the hook_init script run on every single page.

Small text modification

• It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
• lines in README.txt should not exceed 80 characters, see also http://drupal.org/node/447604
• allcss_uninstall(): All functions should have doc blocks, especially hook implementations. see http://drupal.org/node/1354#hookimpl
• install file: @file doc block missing, see http://drupal.org/node/1354#files
• install file: the "?>" at the end is discouraged, see http://drupal.org/node/318#phptags
• "Implementation of hook_init" should be "Implements of hook_init()."
• "// Seperate current path into dirs and file name": comments should end with a "."
• "foreach($path_parts as $file_name) {": there should be a space before "(". Run coder to check your style http://drupal.org/project/coder
• "'#required' => false" should be "'#required' => FALSE"
• allcss_settings_form(): array indentation errors, see here for formatting: http://drupal.org/node/318#array

After reviewing your module, I have to say I did not find many things that could be an issue. Here is one, though:

In line 20, why do you use DIRECTORY_SEPARATOR to split a Drupal path? On Win servers it is set to backslash, although the Drupal path is always separated with forward slashes. Also, the PHP function split() is deprecated; you could use explode() instead.

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/allcss.module:
   +58: [normal] Menu item titles and descriptions should NOT be enclosed within t().
   +61: [minor] There should be no trailing spaces
   +85: [normal] Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
   +93: [normal] Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
   +109: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +110: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
  
  Status Messages:
   Coder found 1 projects, 2 files, 5 normal warnings, 1 minor warnings, 0 warnings were flagged to be ignored
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

Hi DWetc,

Review of 7.x-1.x branch:
The issues pointed in #22 are fixed. The module passed coder.

Some suggestions:

• You mentioned that you changed the project name to DropIn. The name of the module file should be dropin.module and not allcss.module. The install file, info file names and the function names also need to be changed.
• line 24: removed extra space after "head"
• line 30 and line 40: file_exists can check for relative paths. There is no need for dirname function.
• line 34 and line 44: defined a constant 'JS_DROPIN'. It is better to use a constant here.
• line 50: removed the extra line between two functions.
• line 61: removed extra space from the end of the line.
• line 78,87,95:removed '#required => FALSE' as false is the default value.
• line 112: replaced website.com with the standard example.com

Attaching the patch.

Looks good to me.

Hi DWetc,

Using a constant here is not a big deal. It just makes the program easier to read and modify IMO. The module looks really good to me.

Please take a moment to make your project page follow tips for a great project page.

Thanks for your contribution, DWetc! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in minde: Commit messages - providing history and credit and Release naming conventions.