[D6] CCK Combo

chhavik,

If I understand the discussion in http://drupal.org/node/1153250, correctly, your CCK Combo module is essentially the same as the CCK Multigroup module included with CCK-3.0; except that yours 1) works with cck-2.0 and 2) bundles in the 'nodereference' and 'userreference' functionality without the need to install those modules.

While it isn't stressed as much as it should be in the 'Full Project Application' documentation, the 'How to Review Full Project Applications' page contains the following quote:

Module duplication is a real problem on drupal.org, and continues to grow. Again, to ensure that the code on drupal.org is useful to the community of people using Drupal, it is important to ensure that module duplication is avoided.

With over 8000 modules in the Drupal contrib space, and more being added daily, there is a very large movement within the Drupal community to curb the proliferation of modules which duplicate functionality already available in other contributions. The guiding principle is "Collaboration, not competition"; referring to the preference that developers offer their contribution as patches to the existing modules, rather than generating new modules with the same functionality.

Unfortunately, I'm afraid that your module as submitted would not pass the 'duplicate module' test which is a major component of this review process; for two reasons:

1. The functionality provided in your module is already available via the existing CCK Multigroup module. I recognize that Multigroup is not available for CCK2; but it would be preferable to:
   • provide a CCK2 solution that re-leverages the 'CCK Multigroup' namespace, which would best be handled as a patch submitted to the CCK2 issue queue
   • avoid the introduction of a new module that would compete with CCK Multigroup for CCK3 implementations
2. The bundling of 'user reference' and 'node reference' fields within your own module, when existing modules already exist with this capability, is strongly discouraged.

I have set the status of your application to 'needs work'; but in my own opinion, the module can not meet both of the 'duplicate module' concerns above, and still have enough functionality left over to warrant its release. If you feel you can adequately address the above points, then please do (and set the application status back to 'needs review' accordingly) ... otherwise, I'd encourage you set the status to 'closed (won't fix)', and re-apply with a different module.

I understand that this may seem a harsh judgement - please do not mistake this as a suggestion that your contribution is not valued. By taking the step of submitting your module to this application queue, and offering to share your code back to the Drupal community, you have shown more initiative than the vast majority of developers who ever tinker with Drupal ... on behalf of the larger Drupal community, I want to recognize this and thank you for taking this added step. Please do not let this review discourage you from future submissions ... instead, I'd encourage you to view it as a learning opportunity, which can only serve to help your next 'Full Project Status' application and strengthen any future contributions you may consider.

Hi chhavik,

After looking through the code, I agree CCK Combo is not the same as CCK Multigroup, but it still seems to duplicate existing functionality, which is still something we want to avoid on Drupal.org as much as possible. It looks like I can enable CCK Multigroup, User Reference, and Node Reference, do some configuration, and get all of the functionality available in CCK Combo. If I am missing something, please clarify.

If your goal is simply to eliminate the need for configuration, I would suggest you focus on doing that in collaboration with the existing modules by auto-creating field instances and/or content types, rather than trying to replace those modules.

Closing due to inactivity, feel free to re-open if this was a mistake.

It looks like I can enable CCK Multigroup, User Reference, and Node Reference, do some configuration, and get all of the functionality available in CCK Combo.

Is this really a reason to reject a module? By that logic 1/2 the modules on d.o. should have been rejected. For instance, comment notify should be rejected because you can do it with Rules, comment fields and some configuration. Does that mean it is not useful? No, it's absolutely essential because it fulfills a usability need.

That being said, I think the author needs to articulate exactly why someone browsing d.o. would want to install this. I'm not clear on that either since the title and description doesn't suggest there is a usability enhancement here. Maybe this module should be called "Reference and Description" or something? Is that the purpose? As @sreyen suggested, are there other modules which would want to have this configuration automatically set up?

In general, I do feel like modules which provide a configuration headstart are *really* important for Drupal's UX, and we need to encourage it, but I don't know exactly where this one fits.

Is this really a reason to reject a module?

No, and nothing was rejected. As tim.plunkett said in #7, this was closed due to inactivity, and can be re-opened whenever chhavik wants to continue the review process.

If chhavik would prefer to not integrate with other modules for whatever reason, that's her choice. It's just a big enough potential change that it's worth talking about before a release, to avoid complicated upgrades later.

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/cck_combo.module:
   +2: [minor] Commits to the Git repository do not require the CVS $Id$ keyword in each file.
   +10: [minor] Comment should be read "Implements hook_foo()."
   +30: [minor] Comment should be read "Implements hook_foo()."
   +50: [minor] Comment should be read "Implements hook_foo()."
   +59: [minor] Comment should be read "Implements hook_foo()."
   +72: [minor] Comment should be read "Implements hook_foo()."
   +239: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +245: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +247: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +248: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +249: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +251: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +265: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +272: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +274: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +275: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +276: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +278: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +292: [minor] Comment should be read "Implements hook_foo()."
   +402: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +406: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +425: [minor] Comment should be read "Implements hook_foo()."
   +435: [minor] Comment should be read "Implements hook_foo()."
   +458: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +461: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +480: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +492: [minor] Comment should be read "Implements hook_foo()."
   +553: [minor] Comment should be read "Implements hook_foo()."
   +637: [minor] Comment should be read "Implements hook_foo()."
   +714: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +716: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +745: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +774: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +778: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +814: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +818: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +944: [minor] Comment should be read "Implements hook_foo()."
   +1002: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1025: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1228: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1236: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1262: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1281: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1291: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1292: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1314: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1324: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1341: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1351: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1357: [minor] Comment should be read "Implements hook_foo()."
   +1402: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1426: [minor] Comment should be read "Implements hook_foo()."
  
  Status Messages:
   Coder found 1 projects, 1 files, 37 normal warnings, 15 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.
• ./cck_combo.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

      // When preparing a translation, load any translations of existing references.
            // We can assume the translation module is present because it invokes 'prepare translation'.
    // Used so that hook_field('validate') knows where to flag an error in deeply nested forms.
    // Used so that hook_field('validate') knows where to flag an error in deeply nested forms.
      // Use === to be sure we get right results if parent is a zero (delta) value.
      // If the view doesn't exist, we got FALSE, and fallback to the regular 'standard mode'.
      // If the view doesn't exist, we got FALSE, and fallback to the regular 'standard mode'.
      // TODO: We should let the user pick a display in the fields settings - sort of requires AHAH...
      // TODO from merlinofchaos on IRC : arguments using summary view can defeat the style setting.
      // We might also need to check if there's an argument, and set *its* style_plugin as well.
      // TODO: We should let the user pick a display in the fields settings - sort of requires AHAH...
      // TODO from merlinofchaos on IRC : arguments using summary view can defeat the style setting.
      // We might also need to check if there's an argument, and set *its* style_plugin as well.
  

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

  ./cck_combo.module:228:    } // end of switch
  ./cck_combo.module:377:    } // end of switch
  ./cck_combo.module:419:    } // end of switch
  ./cck_combo.module:621:    } // end of switch
  ./cck_combo.module:632:    } // end of switch
  ./cck_combo.module:693:  } // end of switch
  ./cck_combo.module:962:  } // end of switch
  

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

  
  function cck_combo_user_ref_value($element, $edit = FALSE) {
  --
  
  function cck_combo_user_ref_process($element, $edit, $form_state, $form) {
  --
  
  function cck_combo_user_ref_validate($element, &$form_state) {
  --
  
  function _cck_combo_user_potential_references($field, $string = '', $match = 'contains', $ids = array(), $limit = NULL) {
  --
  
  function _cck_combo_user_potential_references_views($field, $string = '', $match = 'contains', $ids = array(), $limit = NULL) {
  --
  
  function _cck_combo_user_potential_references_standard($field, $string = '', $match = 'contains', $ids = array(), $limit = NULL) {
  --
  
  function theme_cck_combo_node_ref($element) {
  --
  
  function theme_cck_combo_user_ref($element) {
  

• There should be no space after the opening "(" of a control structure, see http://drupal.org/node/318#controlstruct
  

  cck_combo.module:78:      switch ( $field['widget']['type'] ) {
  cck_combo.module:197:    switch ( $field['widget']['type'] ) {
  cck_combo.module:215:      switch ( $field['widget']['type'] ) {
  cck_combo.module:232:      switch ( $field['widget']['type'] ) {
  cck_combo.module:318:    switch ( $field['widget']['type'] ) {
  cck_combo.module:381:    switch ( $field['widget']['type'] ) {
  cck_combo.module:668:  switch ( $field['widget']['type'] ) {
  cck_combo.module:947:  switch ( $field['widget']['type'] ) {
  

• Direct concatenation of strings into a sql query is a HUGE red flag for SQL injection vulnerabilities. Use proper variable placeholders in all sql strings. Example issue:
  

  ./cck_combo.module:1386:          $ids = db_query(db_rewrite_sql("SELECT DISTINCT(n.nid), n.title, n.type FROM {node} n LEFT JOIN {" . $table . "} f ON n.vid = f.vid WHERE f." . $column . "=" . $account->uid . " AND n.status = 1"));
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  cck_combo.info:1:; $Id$
  cck_combo.install:2:// $Id$
  cck_combo.module:2:// $Id: cck_combo.module, 2011/05/11 $
  README.txt:1:/* $Id: README.txt, v 1.0 2011/05/11 cck combo $ */
  

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

Other comments (manual/cursory review):
- I'd suggest breaking any multi-sentence strings contained within a t() function down into multiple t() strings, one per sentence. This would also allow you to move some of the html elements outside of the t() functions ... both of these will make things easier on potential translators.
- For code readability, I'd suggest explicitly concatenating variables into strings, rather than including them within quotes (ie. from"users_$table_alias" to "users_" . $table_alias