Ubuntu LoCo team info (drupal 6)

• 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.
• info file: make sure to list all your (core) dependcies here. Do you require the block module?
• ubuntu_loco_team_info_uninstall(): all hook implementations need to have proper doc blocks, see http://drupal.org/node/1354#hookimpl
• module file: @file doc block missing, see http://drupal.org/node/1354#files
• _ubuntu_loco_team_info_url_exists(): do not use "//" for function documentation. Use the doxygen standards instead.
• _ubuntu_loco_team_info_url_exists(): indentation errors, the function body should be indented 2 spaces. Please run coder to check your code style http://drupal.org/project/coder
• "@param path which path of the site we're displaying help": the parameter description should be on a new line, see http://drupal.org/node/1354#functions
• '#type' => "$type",: no need to wrap $type in "" here
• "// function onthisdate_help": what is this? Remove it.
• All functions should have doc blocks, especially hook implementations.

Review of the 6.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/ubuntu_loco_team_info.module:
   +35: [normal] curly braces { should end a line, not start one
   +37: [normal] curly braces { should end a line, not start one
   +43: [minor] There should be no trailing spaces
   +170: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +378: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +398: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +414: [minor] Comment should be read "Implements hook_foo()."
   +464: [minor] Comment should be read "Implements hook_foo()."
   +583: [minor] Comment should be read "Implements hook_foo()."
   +599: [minor] Comment should be read "Implements hook_foo()."
  
  Status Messages:
   Coder found 1 projects, 2 files, 5 normal warnings, 5 minor warnings, 0 warnings were flagged to be ignored
  

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• Comments: there should be a space after "//".
  

  ubuntu_loco_team_info.module:473:  //define module path
  

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

Review of the 6.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/ubuntu_loco_team_info.module:
   +174: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +382: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +402: [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, 3 normal warnings, 0 warnings were flagged to be ignored
  

• Comments: there should be a space after "//".
  

  ubuntu_loco_team_info.module:211:  //date_default_timezone_set('Europe/Amsterdam');
  ubuntu_loco_team_info.module:307:  //date_default_timezone_set('Europe/Amsterdam');
  

• ./ubuntu_loco_team_info.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

      // check if there is a link between [] in the launchpad team description, if so, use that as a link to the team's homepage
  

• Assignments should hava a space before and after the operator, see http://drupal.org/node/318#operators
  

  ./ubuntu_loco_team_info.module:74:  $content='';
  ./ubuntu_loco_team_info.module:127:  $content='';
  ./ubuntu_loco_team_info.module:297:  $content='';
  

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

manual review:

• _ubuntu_loco_team_info_memberscontent(): be careful when embedding user generated variables or third party retrieved data. Although you have a validation function for ubuntu_loco_team_info_defaultlogolink i would suggest to use a check_url() here for $img_url.
• same in _ubuntu_loco_team_info_memberscontent()
• _ubuntu_loco_team_info_memberscontent(): if you pass HTML to l() you must use at least filter_xss() for security reasons.
• "$localepoch = (date('I', $epoch)+1)*3600+$epoch ;": there should be a space before and after an operator (+,*). There should not be a space before ";"
• ubuntu_loco_team_info_menu(): wrong access arguments, use your defined permission.
• ubuntu_loco_team_info_perm(): the first permission is never used.
• ubuntu_loco_team_info_block(): "'info' => t("$blockname")": do not pass variables to t(), because then the text cannot be detected by translation extractors. Use t() directly on $ubuntu_loco_team_info_blocknames.
• "switch ($ubuntu_loco_team_info_blocknames[$delta]) {": indentation errors for the last 2 cases.
• "$form['ubuntu_loco_team_info_loconame'] = array(": the closing ");" should be on the same level, see http://drupal.org/node/318#array
• ubuntu_loco_team_info_admin(): many array structure indentation errors, always use 2 spaces per level.

I run coder from Drupal 7 with the minor setting enabled.

Review of the 6.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/ubuntu_loco_team_info.module:
   +621: [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, 1 normal warnings, 0 warnings were flagged to be ignored
  

• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  

  ./ubuntu_loco_team_info.module:399:  $block_content = '';  //declare your output variable
  ./ubuntu_loco_team_info.module:406:} // function ubuntu_loco_team_info_help
  ./ubuntu_loco_team_info.module:415:} // function ubuntu_loco_team_info_perm()
  ./ubuntu_loco_team_info.module:464:} // function ubuntu_loco_team_info_block
  

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

manual review:

• ubuntu_loco_team_info_init(): why do you need hook_init() (bad for performance)? Add the CSS when the block is displayed.
• "$img_url_tagged = filter_xss('<img src="' . $img_url . '">');": better use theme('image', ...) to generate image markup.
• "// create content": All comments should start capitalized and end with a "."
• "$events_json = strip_tags(file_get_contents($dataurl));": why do you need strip_tags() here? Sanitization should only be done immediately before output. Or do you receive invalid JSON here that you need to fix? Please document that if it is so.
• "'#options' => $locosubteamnames,": #options on checkboxes form elements is not XSS protected, so you should wrap those elements in check_plain(). See http://drupal.org/node/28984
• "if ($selectedloco != '') {": indentation errors in the body of this if statement. Make sure that the code is on the correct indentation level.
• I would recommend that you define your own theme functions for the markup that you build, because then it can be easily overridden by themes. You would separate retrieving/building the data from arranging it by passing variables to theme functions. But for me this is not a requirement for this approval process.

So I think this is nearly ready.

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

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.
• Comments: there should be a space after "//", see http://drupal.org/node/1354#inline
  

  ubuntu_loco_team_info.css:51:  //font-weight: normal;
  ubuntu_loco_team_info.css:57:  //text-align:center;
  

• ./ubuntu_loco_team_info.module: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function _ubuntu_loco_team_info_scheme() {
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./ubuntu_loco_team_info.info ./schemecontent.tpl.php ./memberscontent.tpl.php ./teamscontent.tpl.php ./README.txt ./ubuntu_loco_team_info.module ./eventscontent.tpl.php ./meetingscontent.tpl.php ./ubuntu_loco_team_info.css
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• eventscontent.tpl.php: do not do templates like this. Use the HTML as main content of the file and embed the PHP code where needed, not the other way around. Also in the other template files.

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

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• <?php print t('Forthcoming meetings') ?>: Always use the ";" PHP statement delimiter, see https://drupal.org/node/318#semicolon
• ubuntu_loco_team_info.css: indentation errors, always use 2 spaces per level, see https://drupal.org/node/302199
• "$event_url = 'http://loco.ubuntu.com/events/' . $selectedloco . '/' . $event['id'];": this could also use a check_url().
• _ubuntu_loco_team_info_meetingscontent(): what about sanitization? I'm a bit worried that you are printing third-party provided input unsanitized, like the meeting content. Use check_plain(), check_markup() or at least filter_xss() to protect against XSS attacks. See also http://drupalscout.com/knowledge-base/drupal-text-filtering-cheat-sheet-...

You have absolutely demonstrated your dedication to understand the Drupal community and willingness to embrace our community standards. We certainly should welcome you as a contributor.

Yes yes, the automated code reviewer thingiemajiggy is still flagging some things, but 99.9% of it is so incredibly minor most people wouldn't take the effort to correct it over the benefit they're getting with other readability enhancements. One thing popped up that my OCD would always fix, though... if (! $websitelink) should be if (!$websitelink) ... but I am not going to let that be any more of a further blocker to your application.

Marking this as "utterly and thoroughly" reviewed and tested by the community/klausi.

Congratulations, Thomas! You are now a "vetted" git user and can promote experimental sandboxes to full projects.

Thank you klausi for your fantastic complete reviews, and thank you ceardach for not letting this lie in limbo forever.

Thomas, i'd be interested in your feedback on this process also, maybe at Meta Discussion: Project Application Process Revamp. Thanks!