FatherShawn [fathershawn]

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, and it should include also a comparison with the existing solutions.

Thank you for your reply.

I'm probably unable to judge the actual code going on here without going through a whole civicrm install, but WRT Drupal expectations...

1. use of require_once()

/**
 *  Initialize CiviCRM with the Profile & Relationship APIs
 */
function civi_relate_prep() {
  civicrm_initialize();
  require_once('api/v2/UFGroup.php');
  require_once('api/v2/UFJoin.php');
  require_once('api/v2/Relationship.php');
}

I presume civicrm_initialize() has done something to the include_path already? Because that require_once wouldn't normally work. Not a bug if it does indeed work, and if civicrm isn't set up in a way that allows module_load_include like we'd normally expect. I appreciate that as a 'bridge' module sometimes you have to use non-Drupal conventions when talking to the other side. It just looks like non-drupal-style code.

2. Uneven indentation

...
    if ( civicrm_error ( $result )) {
      return $created_result['error_message'];
      }
    else {
      return $created_result['id'];
      }
    }
}

Throughout, the close braces seem to be floating a bit. Drupal coding standards have them match.
Seeing as you've run coder.module, I would have thought it would have caught that.
Indenting is uneven in many places.

3. Use t()

  $options = array('NONE' => '- Select a Profile -');

Throughout, any place you enter user-facing English text, you should wrap it in t(). This helps English folk too who just want to override the wording.

4. Declare the use of hooks

/* Define the menu link so that the setting page just set up is displayed. */
function civi_relate_menu() {
..

This was the only func I recognized as a Drupal hook, should probably have had "implementation of HOOK_menu()" or something. I'm not sure from looking at the code if any of the other functions are hooks or unique local functions or specific to the civicrm side of things.

5. CamelCase?

function civi_relate_civicrm_postProcess( $formName, &$form )

Didn't coder.module warn about this?
If it is due to the civicrm side of things, that's why a doc saying "this is a civicrm callback function" would be the thing here.

6. operators?

  if ( $formName == 'CRM_Profile_Form_Edit' and 
       $profile_id==$selected_profile and
       $mode==4) {

I don't think there's anything in the standards about this, but everywhere else in Drupal, we use && not and. Is there a reason you need to change the operator precedence here?
Also spacing.
Also camelCase. I'd forgive camelCase if it's a civicrm convention for that variable.

...
I'm unable to evaluate it for security WRT integrity of data and data access permissions, but it doesn't introduce anything blatantly unsafe like uploads, XSS or injection. So it's doing nothing wrong there. Which is to say, "probably safe".

Can't comment on the coding standards which seem to have already been resolved. I can confirm it installs and works as expected with CiviCRM.

Reviews are not supposed to check if the module installs, and if the module works as expected (in that case, reviews will become a debugging session, and that is not what we want).

I am setting the previous status, waiting for somebody to make a review of the code, and to confirm the previous issues have been resolved.

I commented "I'm probably unable to judge the actual code going on here without going through a whole civicrm install, " above. So someone elses assertion that it does indeed work as expected is a constructive addition, thanks for that!

1. fixed
2. Better. A few columns seem indented by 7 spaces where 6 would be expected, but this doesn't effect the quality or layout. very minor - Not a blocker
3. Fixed
4. Fixed, but the exact layout of the /** comments */ is not quite correct doxygen yet. As far as documentation goes however, its adequate. minor - Not a blocker (as there is no doxygen for contrib available anyway)
5. Good
6. Fixed

Thanks, dman, for the confirmation.

Thank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.

I am giving credit to the users who participated and adding the default message with the useful links.