chrislynch42 [chrislynch42]

I can't properly review not having cvs access myself, but a couple of points:

1. you need to use the doxygen formatting conventions
http://drupal.org/node/1354
EG.
for each 'hook' function you build should have this at the least

<?
/**
 * Implements hook_help().
 */

?>

2. for all your .tpl.php files ( except : page-milan-person-add.tpl.php ) - and I'm sure this applies to some of your other files.

// $Id: node.tpl.php , v 1.4 2007/08/07 08:39:36 goba Exp $

should be

// $Id$

3. I'm not sure why, but all your tpl.php files are the same ( except node-milan.tpl.php ), and all they do is print $content is there a reason for this?

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

When you install a module into Drupal it automatically takes // $Id$ and makes it // $Id: node.tpl.php , v 1.4 2007/08/07 08:39:36 goba Exp $ .

CVS ID tags are expanded when a file is committed in a CVS repository, not when a Drupal module is installed.

It would be better if you upload the updated code, to avoid reviewers report something that you have already changed.

Hi,
The code looks improved. have you ran it through the coder module? ( also you can use the options to do a 'minor' search for issues)

a couple of points that coder has thrown up ( not an exhaustive list )
company.module Line 80 & Line 127:
company.pages.inc Line 37 & Line 50:
else statements should begin on a new line

  } else {

Should be:

  }
 else {

company.pages.inc
Line 44: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE

  $output .= node_view($loaded_node , true , false , true);

company.install
milan_series.pages.inc

 ?>
 

the final ?> should be omitted from all code files

you've also got lots of trailing spaces, some tabs instead of double spaces ( using the 'minor option in coder will help with this.

when you mention a function in the doxygen comments you've missed the () from the functions, these should be included
EG: company.install
Line 9: @see should always be followed by a filename, a url or a function name with ()
@see hook_install
should be
@see hook_install()

milan_person.pages.inc
( also appears in milan_company.pages.inc
Line 24: Line 38: Line 40: In SQL strings, Use db_query() placeholders in place of variables. This is a potential source of SQL injection attacks when the variable can come from user data. (Drupal Docs)

  $result = db_query("SELECT n.nid FROM {node} n WHERE n.type = 'person' $author_sql ORDER BY n.title , n.sticky DESC , n.created DESC");

( the above was pulled from coder, it may not be valid, can you explain what the variable is/does in that context and provide a reason to not use a placeholder?

page-milan-company-add.tpl.php
Line -1: @file block missing (Drupal Docs)

There are a few issues with the @file bit, but if I'm honest they didn't make sense to me and I didn't have time to reread the doxygen stuff to make sense of it, its probably needing a space here or a line there type of thing.

I've not had a chance to play around with the module itself to check for bugs, and I'm unlikely to do so before next week. however I'd suggest that you work on the issues here ( using the coder module hint hint ) to allow someone to evaluate its functionality without formatting and doxygen issues.

OH as a final note, please include a readme.txt , I like reading them to find out how to install/use the module

I'm afraid I wont be able to look at this for a few days, setting to needs review in the hope someone else will.

@chrislynch42 : each time you submit a revision to your module you should change the status to 'needs review' ( unless it still needs work :P )

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.