CiviCRM Reference fields

It's hard to get a review of civicrm projects. If you can I suggest getting someone familiar with civicrm to provide a review. http://groups.drupal.org/code-review has details on how to do that.

Suggestions:

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

Please take a moment to make your README.txt follow the guidelines for in-project documentation.

I noticed some very small code style issues. Please run the coder module on "minor" setting to help catch these. The coding standards have even more information in this area. I noticed non-standard whitespace.

These should be removed. It's only necessary to declare files[] if they declare a class or interface.

9 files[] = civicrm_reference_fields.install
10 files[] = civicrm_reference_fields.module

Required changes

These should be removed as they are added by the packaging script:

12 ; Information added by drupal.org packaging script on 2011-08-13
13 version = "7.x-1.alpha1"
14 core = "7.x"
15 datestamp = "1311834128"

I don't know someone who is familiar with the CiviCRM from Code Review team.

They don't have to be on the code review team, they can be anyone familiar with CiviCRM.

Thanks for making the updates.

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.
Review of the master 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/civicrm_reference_fields.module:
   +182: [critical] table names should be enclosed in {curly_brackets}
   +216: [critical] table names should be enclosed in {curly_brackets}
   +252: [critical] table names should be enclosed in {curly_brackets}
  
  Status Messages:
   Coder found 1 projects, 2 files, 3 critical warnings, 0 warnings were flagged to be ignored
  

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• There should be no space after the opening "(" of an array, see http://drupal.org/node/318#array
  

  civicrm_reference_fields.module:191:  $where = array( );
  

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

  ./civicrm_reference_fields.module:177:function _civicrm_reference_fields_get_values_contribution($ids=array(), $title='', $extra_where=array(), $limit=25) {
  ./civicrm_reference_fields.module:211:function _civicrm_reference_fields_get_values_event($ids=array(), $title='', $extra_where=array(), $limit=25) {
  ./civicrm_reference_fields.module:247:function _civicrm_reference_fields_get_values_pcp($ids=array(), $title='', $extra_where=array(), $limit=25) {
  ./civicrm_reference_fields.module:303:  $element += array('#type' => 'textfield',
  ./civicrm_reference_fields.module:427:function _civicrm_reference_fields_find_references($type, $field, $string, $match_exact=FALSE, $limit=10) {
  

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

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/civicrm_reference_fields.module:
   +182: [critical] table names should be enclosed in {curly_brackets}
   +216: [critical] table names should be enclosed in {curly_brackets}
   +252: [critical] table names should be enclosed in {curly_brackets}
  
  Status Messages:
   Coder found 1 projects, 1 files, 3 critical warnings, 0 warnings were flagged to be ignored
  

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

  
  function _civicrm_reference_fields_get_values_contribution($ids = array(), $title = '', $extra_where = array(), $limit = 25) {
  --
  
  function _civicrm_reference_fields_get_values_event($ids = array(), $title = '', $extra_where = array(), $limit = 25) {
  --
  
  function _civicrm_reference_fields_get_values_pcp($ids = array(), $title = '', $extra_where = array(), $limit = 25) {
  --
  
  function _civicrm_reference_fields_get_type($field) {
  --
  
  function _civicrm_reference_fields_get_details($items, $field) {
  --
  
  function _civicrm_reference_fields_find_references($type, $field, $string, $match_exact=FALSE, $limit=10) {
  --
  
  function _civicrm_reference_fields_available_types( ) {
  --
  
  function _civicrm_reference_fields_build_extra_where($extra_where, &$where, &$params) {
  

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

  ./civicrm_reference_fields.module:427:function _civicrm_reference_fields_find_references($type, $field, $string, $match_exact=FALSE, $limit=10) {
  

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

  ./civicrm_reference_fields.module ./README.txt ./civicrm_reference_fields.info ./civicrm_reference_fields.install
  

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:

• As for your query call, I understand the curly brackets issue and consider it ok. Nevertheless, at a first glance, I am not sure whether you are sanitizing all data coming from and going to the database. But I will get back to that in a subsequent round.
• There are still files in your master branch, you should remove them as described in http://drupal.org/node/1127732 at step 5
• In cases like e.g. civicrm_reference_fields.module lines 17-18, you should use the regular indentation of two spaces as that would dramatically increase readability. Also you could add a linebreak + indentation directly after the opening parenthesis, similar to functions, which would as well make reading much more comfortable for the first array element(s). Both applies to most of your array definitions.
• Just for a better understanding: I cannot find any call to your _civicrm_reference_fields_get_values_contribution() function. Please give me a hint where that applies to have a closer look.
• Still, doxygen blocks are missing. Make sure that you have read and understood http://drupal.org/node/1354#functions :)

HTH, dave

Manual review:

• There are still files in your master branch. You should replace them as described in http://drupal.org/node/1127732 at step 5.
• I still find civicrm_reference_fields_field_info() and following really hard to read. This is not to harass you, but wouldn't you consider a notable structure also more helpful for your own subsequent reviews? I mean e.g.
  

    return array(
      'civicrm_reference_fields_contribution' => array(
        'label' => t('CiviCRM Contribution Pages'),
        'description'  => t('Reference a CiviCRM Contribution Pages.'),
        'default_widget'    => 'options_select',
        'default_formatter' => 'civicrm_reference_fields_link'),
      ),
    );
  

  vs.

    return array('civicrm_reference_fields_contribution' => array('label' => t('CiviCRM Contribution Pages'),
                                                                  'description'       => t('Reference a CiviCRM Contribution Pages.'),
                                                                  'default_widget'    => 'options_select',
                                                                  'default_formatter' => 'civicrm_reference_fields_link'),);
  

  avoids some horizontal scrolling while debugging or simply reading, you know what I mean? Don't get me wrong, no big thing, and perhaps you have good reasons to do it the other way? But I personally love code being structured for quickest possible understanding, and I guess that's what most Drupalers do. As for control structures etc. you widely follow this approach anyway :)

• Nevertheless please have a look at indentation again, e.g. at .module, line 370.
• After thinking about it again, perhaps a short comment line on the curly bracket related warnings would be helpful or something else that clarifies that you are building and executing an external query here. This would ease understanding for others reading your code. E.g. an additional longer comment in the doxygen header like "This function builds a query that will retrieve data related to $foo and $bar from an external DAO source". This is of course not mandatory but might avoid misunderstandings in the future, so please consider it as just a proposal.

Hope this helps and you don't find my comments too fussy.
-dave

Changed git clone link.

Hi,

just updated my review tools, here are the results (I left the curly brackets warning in it but I know it is not relevant here as we already discussed):

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

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/civicrm_reference_fields.module:
   +252: [critical] table names should be enclosed in {curly_brackets}
   +312: [critical] table names should be enclosed in {curly_brackets}
   +374: [critical] table names should be enclosed in {curly_brackets}
  
  Status Messages:
   Coder found 1 projects, 1 files, 3 critical warnings, 0 warnings were flagged to be ignored
  

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

  
  FILE: ...l/modules/pareview_temp/test_candidate/civicrm_reference_fields.install
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AND 4 WARNING(S) AFFECTING 3 LINE(S)
  --------------------------------------------------------------------------------
    5 | WARNING | Line exceeds 80 characters; contains 83 characters
   11 | ERROR   | You must use "/**" style comments for a function comment
   14 | WARNING | A comma should follow the last multiline array item. Found:
      |         | FALSE
   14 | WARNING | A comma should follow the last multiline array item. Found: )
   14 | WARNING | A comma should follow the last multiline array item. Found: )
  --------------------------------------------------------------------------------
  
  
  FILE: ...ll/modules/pareview_temp/test_candidate/civicrm_reference_fields.module
  --------------------------------------------------------------------------------
  FOUND 9 ERROR(S) AND 21 WARNING(S) AFFECTING 28 LINE(S)
  --------------------------------------------------------------------------------
    20 | WARNING | A comma should follow the last multiline array item. Found: )
    48 | WARNING | A comma should follow the last multiline array item. Found: )
    64 | WARNING | A comma should follow the last multiline array item. Found: )
    72 | WARNING | A comma should follow the last multiline array item. Found: )
    80 | WARNING | A comma should follow the last multiline array item. Found: )
    81 | WARNING | A comma should follow the last multiline array item. Found: )
   124 | WARNING | A comma should follow the last multiline array item. Found: )
   132 | WARNING | A comma should follow the last multiline array item. Found: )
   133 | WARNING | A comma should follow the last multiline array item. Found: )
   142 | ERROR   | Inline control structures are not allowed
   162 | WARNING | A comma should follow the last multiline array item. Found: ]
   162 | WARNING | A comma should follow the last multiline array item. Found: )
   162 | WARNING | A comma should follow the last multiline array item. Found: )
   168 | ERROR   | Line indented incorrectly; expected 8 spaces, found 10
   200 | WARNING | A comma should follow the last multiline array item. Found:
       |         | TRUE
   208 | WARNING | A comma should follow the last multiline array item. Found:
       |         | TRUE
   249 | ERROR   | Inline control structures are not allowed
   309 | ERROR   | Inline control structures are not allowed
   371 | ERROR   | Inline control structures are not allowed
   442 | WARNING | A comma should follow the last multiline array item. Found: )
   477 | WARNING | A comma should follow the last multiline array item. Found: )
   484 | WARNING | A comma should follow the last multiline array item. Found: 1
   491 | WARNING | A comma should follow the last multiline array item. Found: 2
   580 | ERROR   | Inline control structures are not allowed
   590 | ERROR   | Inline control structures are not allowed
   595 | ERROR   | Inline control structures are not allowed
   643 | WARNING | A comma should follow the last multiline array item. Found:
       |         | TRUE
   651 | WARNING | A comma should follow the last multiline array item. Found:
       |         | TRUE
   663 | ERROR   | You must use "/**" style comments for a function comment
   668 | WARNING | A comma should follow the last multiline array item. Found: )
  --------------------------------------------------------------------------------
  

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

  ./civicrm_reference_fields.module ./civicrm_reference_fields.info ./civicrm_reference_fields.install
  

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.

HTH, dave

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

manual review:

• "Implemetation of hook_menu()." should be "Implements hook_menu().". Also for the other hook implementations.
• civicrm_reference_fields_menu(): doc block: do not break lines earlier than 80 characters if you do not have to.
• civicrm_reference_fields_field_info(): the array key word should be on the same line as the return keyword, and the whole array block should be moved 2 spaces to the left. Also in other functions.
• civicrm_reference_fields_field_is_empty(): no need for the conditional statement, just return the result of the empty() call.
• _civicrm_reference_fields_get_values_contribution(): do not concatenate variables into DB query strings, use placeholders instead. See http://drupal.org/writing-secure-code . Also in other places, check all your queries.
• civicrm_reference_fields_field_settings_form(): do not run variables through t(), as the text cannot be detected by translation tools. Do not divide your messages as this is bad for translation. Use the full string directly, e.g. t('CiviCRM Event Start date')
• civicrm_reference_fields_autocomplete_validate(): indentation error on "$id = $matches[2];".
• civicrm_reference_fields_autocomplete_value(): Are you sure that you don't need to sanitize $row['title'] before outputting it?
• _civicrm_reference_fields_build_extra_where(): Are you building where conditions for DB queries here? If so you need to use DB placeholders here.

Seems like you have done the updates? I do not get the coding standards errors you are referring to.

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended here: http://drupal.org/node/1011698

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

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: ...ll/modules/pareview_temp/test_candidate/civicrm_reference_fields.module
  --------------------------------------------------------------------------------
  FOUND 4 ERROR(S) AFFECTING 3 LINE(S)
  --------------------------------------------------------------------------------
   168 | ERROR | BREAK statements must be followed by a single blank line
   513 | ERROR | Trailing punctuation for @see references is not allowed.
   674 | ERROR | There should be no white space after an opening "("
   674 | ERROR | There should be no white space before a closing ")"
  --------------------------------------------------------------------------------
  

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. Get a review bonus and we will come back to your application sooner.

manual review:

1. civicrm_reference_fields_menu(): your menu path is unprotected. Almost anyone can query that path to retrieve information about your fields. You must limit the access to users that are actually allowed to use the fields.
2. _civicrm_reference_fields_get_values_contribution(): You are adding only one parameter (the title), but you should also do that for $ids and $limit, right? There is a lot of code where you concatenate variables directly into the query strings, I can smell possible SQL injection vulnerabilities.
3. _civicrm_reference_fields_build_extra_where(): the first sentence in the doc block should use the full 80 characters limit and not break earlier.

Hi Rajan,

Can you give some examples of how this module might be used? We may be able to test.

Thanks,

Owen.

Hi Rajan,

I have reviewed your module with coder manually and found following issues.

civicrm_contact_ref.module

severity: normal
Line 10: Menu item titles and descriptions should NOT be enclosed within t(). (Drupal Docs)
'title' => t('Contacts'),
severity: minor
Line 91: Missing period
* Implements hook_field_formatter_view()

severity: critical
Line 204: Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.
'#title' => ts('CiviCRM Contact Type %1', array(1 => $label)),

severity: minor
Line 365: use lowercase html tags to comply with XHTML
AND sort_name NOT LIKE '%%'

civicrm_contact_ref.install
severity: normal
Line -1: @file block missing

Thanks & Regards,
prathK
http://extrimity.in

Hi rajan,

OOoops !!! Sorry for the earlier misunderstanding.
It was because of similar names of modules.
I have done review of your module with coder. I found following errors :

Line 263: table names should be enclosed in {curly_brackets}

$query = "SELECT id, title FROM civicrm_contribution_page WHERE is_active = 1 ";

severity: critical

Line 324: table names should be enclosed in {curly_brackets}

$query = "SELECT id, title FROM civicrm_event WHERE ( civicrm_event.is_template IS NULL OR civicrm_event.is_template = 0 ) AND is_active = 1 ";

severity: critical

Line 387: table names should be enclosed in {curly_brackets}

$query = "SELECT id, title FROM civicrm_pcp WHERE is_active = 1 ";

severity: normal

Line 451: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms

'#autocomplete_path' => $instance['widget']['settings']['autocomplete_path'] . '/' . $instance['entity_type'] . '/' . $instance['bundle'] . '/' . $field['field_name'],

severity: critical

Line 488: Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.

'#title' => ($type == 'event') ? t('CiviCRM Event Start date is') : t('CiviCRM Contribution Start date is'),

severity: critica

lLine 495: Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.

'#title' => ($type == 'event') ? t('CiviCRM Event End date is') : t('CiviCRM Contribution End date is'),

Thanks & Regards,
PrathK
http://extrimity.in

Hi Rajan,

I have reviewed it with devel and for functionality wise as well.
Module seems ready to go for me after all these fixes related to the module.
Nice job !! very useful module if we want to put contribution pages as field reference itself.

Cheers,
PrathK
extrimity.in

There is still a master branch, make sure to remove it. See also step 6 and 7 in http://drupal.org/node/1127732

manual review:

• civicrm_reference_fields_field_formatter_view(): "'#markup' => $values[$item[$field_id]]['title'],": I assume the title is already sanitized, so this is not vulnerable to XSS?

Otherwise looks good to me, so ...

Thanks for your contribution, Rajan M! 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 mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.