Consensus

I'm intrigued by the concept! Sounds very interesting.

Is there any way you could open up your sandbox to anon users, so reviewers don't have to create accounts?

Onto a code review...

- module code should not contain author credits -- credit is given in the git log, in issues, and project pages.
- run this through coder.
- tidy up the spacing of things like comment blocks
- hooks have a standard comment block start. Many of these need cleaning up. Eg, this has a stray space and needs to be in sentence case:
* implementation of hook_help

- consensus_old.module -- this file perhaps needs removing?
- consensus_class.php -- classes usually go in CLASSNAME.inc
- This is totally wrong:

	if (!module_exists('consensus')) {
		return;
	}

Use dependencies!
- consensus_toolbox_populate_form -- messing with $_POST and $_SESSION is a Bad Thing, usually.
- drupal_install_schema -- remember to uninstall this too
- consensus.views.inc -- file is empty
- consensus_settings -- this function is terrifyingly huge!!!
- variable_get('consensus_item_options_item_edit_permissions_owner', true) -- you appear to have millions of these. At some point, using the variable table becomes a bad idea and you have to implement your own storage of settings. Looks like you need a {consensus_access} table. Also, TRUE, not true.
- * Prevents the node body, which is in a Consensus XML format, from being overwritten
Using the node body like this and having to hack around to prevent it being overwritten seems a bit weird...
- your module file is huge. I would move menu callbacks to either an admin.inc or pages.inc accordingly, and helper functions for parsing to an include file.
- * Implementation of hook_consensus_revised -- if you have your own hooks, you really should have an api.php file to describe them. This is a D7 rather than D6 requirement, but it's good practice.
- dwsync.xml -- are these files meant to be here?
- consensus_groups_form_alter -- not the right way to get settings into a block admin form!

I'll stop there as this is a HUGE module and while there is lots still to look at, that's plenty for you to work on for now :)

One conceptual thing to end on though:

			'classes' => array(
				'item' => 'c_item',
				'objection' => 'c_objection',
				'reservation' => 'c_reservation',
				'question' => 'c_question',
				'note' => 'c_note',
				'block' => 'c_block',
				'heading' => 'c_heading',
				'comment' => 'c_comment',
				'hidden' => 'c_hidden',
				'heading' => 'c_heading',
			),

Anytime I see this sort of thing, I think that it should be provided by an info-type hook, rather than hardcoded, so it can be extended by other modules in future.

Sorry -- I meant your demo site, not the git sandbox...

Move the XML data into a field that is separate from the node body.

This project is indeed so huge that it would take a week or more to read through and understand the whole thing. Looking at the project it is obvious that the author is an expert programmer with an in-depth knowledge of Drupal. The project follows coding standards and is well documented. The only things I noticed were:

* The consensus_class.php file is still in the repository, but the code has been duplicated in consensus.inc
* The author uses include_once and require_once instead of module_load_include
* In the .module file, used a check_plain instead of t() for a title.

There is always the possibility that there could be some security vulnerability hidden somewhere in the code, but the author is obviously aware of and consistently uses correct procedures for writing secure code.

There is some third party code in the parser directory that consists of a single class that is stated to be released under GPL. This may have to be removed.

The entire JS directory has non-GPL licensed files.

Are we looking at the same thing? I looked at head for 6.x-1.x. The only thing in the js directory is consensus.js.

@Tommy Kaneko

It looks like you definitely are going to have to remove the code in the parser directory and instruct your users how to get it, unless you can convince an admin that it falls under an exception. See http://drupal.org/node/422996.

Still needs work per #11.

Also, the master branch should be emptied out similar to the examples module: http://drupalcode.org/project/examples.git/tree/refs/heads/master

Otherwise it just seems like that is the code to use.

Might be worth mentioning which git branch people should review, BTW.

> consensus_get_field_settings() -- I'm sure you can use something like content_field() to get these rather than a DB query. It'll probably come from a cache too, saving a query.

> drupal_set_message(t('This NID is not a consensus document'), 'error');

Don't use technical terms like nid for an end-user. (Also, not an acronym, so 'nid'...)

> // $Id$ -- these are deprecated

> * Valid permissions for Consensus module.

Should be "Implementation of hook_perm().". Also, for hook implementations you can drop the @param and @return, since they are standard. In many cases you don't need any further documentation for that function, though I think the summary of paths you have on hook_menu() is a nice touch and could prove useful for anyone working with your module code.

These are all just cosmetic points so shouldn't stand in the way of the application being RTBC. Just the licensing needs sorting out.

• lines in README.txt should not exceed 80 characters
• consensus_menu(): this is a hook implementation, so document it properly http://drupal.org/node/1354#hookimpl . Also on other hook implementing functions.
• "// page for editing consensus content": indentation error, always use 2 spaces. Comments should start capitalized and end with a "."
• "'description' => t('The field name of the content')," no need to run database field descriptions through t(), avoids overhead for translators.
• wiki2xml.php is still in the repository, third party libraries should be removed.

• wiki2xml.php is still in the repository, third party libraries should be removed.

Awaiting resolution at #1301254: 3rd party code exception for project Consensus?.
The other objections are valid.

• I think you can just delete the old 6.x-1.x branch, as this is not a full project yet.
• "class consensus": class names should start capitalized, see http://drupal.org/node/608152
• class consensus: doc block indentation errors, they should be on the same level as the methods.
• methods should follow lowerCamel naming (no that important to me though)
• consensus.js: indentation errors, always use 2 spaces per level. Also in javascript files.
• consensus_groups_schema(): hook implementation doc block missing. Also on many other functions like consensus_groups_theme()
• consensus_groups_block(): there should be no empty line between function and doc block
• "SELECT data FROM {consensus_groups_settings} WHERE uid=%d AND nid=%d": there should be a space before and after "=", see http://drupal.org/node/2497
• please use coder to check your code style http://drupal.org/project/coder

As the results of my automated review are pretty long, I rather add it as attachment. You can paste it in a comment and preview it to get the HTML rendered. I removed the errors on the wiki parser library.

manual review:

• remove all debug statements from your code (print_r etc.)
• there is a lot of commented out code, remove it as well
• consensus_schema(): do not run database description texts throught t(), this will only create overhead for translators
• "// until hook_requirements works reliably!": All comments should start capitalized
• theme_consensus_extra(): you are embedding $notes here, you should sanitize it according to http://drupalscout.com/knowledge-base/drupal-text-filtering-cheat-sheet-... to avoid XSS.
• theme_consensus_extra(): "if (!empty($notes)) {": indentation error in the if-body.
• consensus_enabled_languages_form(): indentation errors on the $form array and in one if statement
• consensus_users_form_alter(): indentation errors in the if-body.
• don't use short variable names like $h, $a, $v etc., use more self explaining ones like $header, $consensus_counter, $new_items etc.
• "if ($added) $a++;": always use "{}" around if statements

Hi,

in standard regex there is no such option*.

In a PERL script, the proper regex were (using posix syntax):

s/^( +\/\/ )([a-z])/$1\u$2/g

In PHP, you would use something like

$filecontent = preg_replace(
  '!^( +// )([a-z])!e',
  "'\\1'.strtoupper(.'\\2')",
  $filecontent
);

*of course you could replace ([a-z]) with (a), (b), ... (z) and \u$2 with A, B, ... Z and do the search/replace 26 times. But nobody would want that.

Won't work. Consider the following:

  // I wanted to make an inline comment, but I found that I couldn't quite
  // finish my point in a single 80-column line, so I wrapped to two.

Your regex would capitalize the word "finish" on the second line.

Oh, sh... you are right. I simply had nothing but one line comments in my mind. Sorry.

The regex would have to be extended to recognize whether the line above already starts with / *\/\//, but i have no clue a.t.m. :-(

That said, if the overwhelming majority of Consensus comments are single-line, your regex might indeed save some work.

Using Textpad or a similar enhanced editor supporting regex search/replace, it would also save time doing it semi-automatted.

I would search in TextPad with ^ +// [a-z], hit ESC to hide the search form, hit CTRL+U for uppercase (if it's really the first line), and then cycle with CTRL+F - CTRL+U through all the documents.

Not super elegant but fast enough for keyboard addicts like me ;)

Review of the 6.x-2.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/consensus.module:
   +10: [minor] Comment should be read "Implements hook_foo()."
   +19: [minor] Comment should be read "Implements hook_foo()."
   +121: [minor] Comment should be read "Implements hook_foo()."
   +884: [minor] Comment should be read "Implements hook_foo()."
   +1012: [minor] Comment should be read "Implements hook_foo()."
   +1037: [minor] Comment should be read "Implements hook_foo()."
   +1078: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1082: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1086: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1088: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1094: [minor] Comment should be read "Implements hook_foo()."
   +1125: [minor] Comment should be read "Implements hook_foo()."
   +1141: [minor] Comment should be read "Implements hook_foo()."
   +1175: [minor] Comment should be read "Implements hook_foo()."
   +1196: [minor] Comment should be read "Implements hook_foo()."
   +1203: [minor] Comment should be read "Implements hook_foo()."
   +1320: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1323: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +1461: [minor] Comment should be read "Implements hook_foo()."
  
  sites/all/modules/pareview_temp/test_candidate/consensus_users/consensus_users.install:
   +57: [critical] 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.
  
  sites/all/modules/pareview_temp/test_candidate/consensus.install:
   +8: [minor] Comment should be read "Implements hook_foo()."
   +395: [minor] Comment should be read "Implements hook_foo()."
   +410: [minor] Comment should be read "Implements hook_foo()."
  
  sites/all/modules/pareview_temp/test_candidate/consensus.items.inc:
   +8: [minor] Comment should be read "Implements hook_foo()."
  
  sites/all/modules/pareview_temp/test_candidate/consensus.edit.inc:
   +72: [critical] 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.
   +296: [critical] Potential problem: when FAPI element '#type' is set to 'markup' (default), '#value' only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.
   +438: [minor] Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), !f
  ilter_xss() or similar.
  
  Status Messages:
   Coder found 1 projects, 20 files, 3 critical warnings, 6 normal warnings, 86 minor warnings, 0 warnings were flagged to be ignored
  

• Remove .DS_Store from your repository.
• ./consensus.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

    // Double new lines, so they will be recognised by the parser as a new praragraph..
          // Build an array of all lines after the current line to send to the recursive function.
  

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

    // Create an array/object to store the timstamp/id pairs of all revision items.
  

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

    // Iterate backwards through list of revisions to find current - more efficient in most cases.
    // Iterate backwards through list of revisions to find current - more efficient in most cases.
        // If it is a revise (edit) button, check that the user has permission to edit it.
        // If it is a revise (edit) button, check that the user has permission to edit it.
        .. many other palces ...
  

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

  ./consensus.module:338:      'edit_permission' => $node ? node_access('update', $node) : FALSE, // TODO: these are obsolete, replace with access array
  ./consensus.module:339:      'access_permission' => $node ? node_access('view', $node) : FALSE, // TODO: these are obsolete, replace with access array
  ./js/consensus.js:144:  var url = Drupal.settings.basePath+'consensus/ajax/'+data.nid; // Drupal path needs to be defined
  ... many other palces ..
  

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

  
  function consensus_install() {
  --
  
  function consensus_update_6100() {
  --
  
  function consensus_uninstall() {
  --
  
  function consensus_upate_6200() {
  

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

  
  function consensus_slider_block_form_submit($form, &$form_state) {
  

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

  
  function theme_consensus_toolbox($form) {
  

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

  
  function consensus_groups_get_data($nid = NULL, $uid = NULL, $cache = TRUE) {
  --
  
  function consensus_groups_form_submit($form, &$form_state) {
  

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

  
  function consensus_groups_schema() {
  --
  
  function consensus_groups_uninstall() {
  

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

  
  function consensus_users_schema() {
  --
  
  function consensus_users_uninstall() {
  

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

  
  function consensus_users_settings($node = NULL) {
  

• ./consensus_users/consensus_users.module: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  298- *
  

• There should be a space before and after operators like ==, ===, && and ||. See http://drupal.org/node/318#controlstruct
  

  consensus.inc:86:    if ($simple&&$dom) {
  js/consensus.js:1930:      else if (itemType == 'endorse' || itemType =='retract') {
  js/consensus.js:1954:      else if (itemType == 'endorse' || itemType =='retract') {
  js/consensus.js:2115:    case (action =='insert_revision'):
  js/consensus.js:2121:    case (action=='insert_item'):
  js/consensus.js:2131:    case (action=='insert_retract'):
  js/consensus.js:2132:    case (action=='insert_endorse'):
  js/consensus.js:2140:    case (action =='insert_translation'):
  

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

  consensus.module:466:  foreach ( $array2 as $key => &$value ) {
  

• There should be no space after the opening "(" of an array, see http://drupal.org/node/318#array
  

  consensus.edit.inc:76:      '#attributes' => array( 'class' => 'c_textarea'),
  consensus.edit.inc:129:      '#attributes' => array( 'class' => 'c_note'),
  consensus.edit.inc:187:      '#attributes' => array( 'class' => 'c_note'),
  consensus.edit.inc:207:      '#attributes' => array( 'class' => 'c_note'),
  consensus.edit.inc:227:      '#attributes' => array( 'class' => 'c_note'),
  consensus.edit.inc:246:      '#attributes' => array( 'class' => 'c_heading'),
  consensus.edit.inc:275:      '#attributes' => array( 'class' => 'c_note'),
  consensus.edit.inc:332:      drupal_set_message(t('the node with ID @nid does not exist', array( '@nid' => $post['nid'])), 'error');
  consensus_users/consensus_users.module:270:    'author-current' => array( 'name' => t('No. of active contributions'), 'image' => 'bullet-edit.png'),
  

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

  ./consensus_slider/consensus_slider.js:37:  var i=0;
  ./js/consensus.js:928:  for (var i=num-1; i>-1; i--) {
  ./js/consensus.js:941:  for (var i=num-1; i>-1; i--) {
  ./consensus.inc:645:    $dom_node= dom_import_simplexml($text);
  

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

I am subscribing to Tommy's points. I recently installed the latest dev release of coder (I was running the release version before). I tried to retest some other review results and had less and/or different results. It really looks like many reviewers use coder 7 on d6 code. Is that possible and if so, could this be stopped? Alternatively, the d6 version of the coding standards page should really be revised. I also agree that this is causes unnecessary delays in the review process and, even worse, results in more (also unnecessary) work for both appliants and reviewers.

Disclaimer: I really appreciate automatting the review process with both coder and PAreview. Also I would support these tools and also the documentation teams where possible Just let me know where I could be helpful.

Yes, I wouldn't blame worse example anyway since it's my personal ambition to deliver the "cleanest" code possible regardless of whether others don't. And the guidelines mention theirselves in some places that core code base partially is a rather bad example ;)

I think the general two goals for all are clean code and as few overhead as possible. We all should do our best to achieve those :-)

(Will also switch to the linked discussion after this post here.)

See #1296842-29: Clarify scope of PHP coding standards for core (major version) and contrib code
And #711918-145: Documentation standard for @param and @return data types

We do not backport new coding standards to previous versions of Drupal.

Also see #1161796-7: Initial d8 port of Coder and Coder Review modules.

Coder maintainers can keep track of which ones have been included in the Coder* modules by looking at:
http://drupal.org/list-change-updates/drupal
On that page, you can search for changes that affect module or theme developers, and which are not yet marked as having been incorporated into Coder*.

I've added my opinion to the thread at http://groups.drupal.org/node/179939#comment-624219 but will re-iterate the points pertinant to this application here:

... new standards seem to be applied to old code.

- I consider all code in the project application process to be 'new' code, until the project has been promoted to 'full' project status.

Drupal coding standards are version-independent and "always-current".

- Changes to coding standards are not made without reason ... I interpret the 'D6 versus D7 version' argument as referring to *core*, while all contrib code should attempt to abide to today's coding and documentation standards.

I would suggest that the common sense approach is to use the latest D6 Coder module for D6 projects, and use D7 Coder for D7 modules

- My opinion is that applying a single set of rules to all projects simplifies the process for reviewers, making the process more efficient, and thus accelerating the process for applicants.

Please note that the "critical warnings" have been considered, and I deem them not be a threat.

- Providing justification as to why you deem them to not be a threat would probably help potential reviewers to arrive at your same conclusion.

Hi Tommy,

I just had klausi's script running over your latest snapshot. I cannot tell whether I did it completely right (your project is the oldest in the review queue and the only other I checked with the tool before was my own one), so if not, don't hate me ;) Also see my remarks at the bottom.

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

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

      // If user does not belong to any groups, put them in the independent groups.
      // If entry exists, merge the new and old data TODO: it doesn't quite merge them yet.
  

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

      // Turn the legend into a clickable link and wrap the contents of the fieldset
  

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

      // If entry exists, merge the new and old data TODO: it doesn't quite merge them yet.
  

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

    // TODO: users contributing to the page for the first time are not loaded.  Force page refresh.
      // Turn the legend into a clickable link and wrap the contents of the fieldset
  

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

      // Check to see if a node has been passed to the constructor, if not, create it.
      // If no object is passed to the $item variable, we find it by using itemid and revid.
      // If itemid has been passed, then use function to find revid.  Otherwise, set it to 1..
      // $text, $type, FALSE, $parentid, $parentrevid, $note, $reason, $extra); // Load revision tag.
      // If no object is passed to the $item variable, we find it by using itemid and revid.
  

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

    // Will try and replace templates right inline, instead of using <template> tags; requires global $content_provider
  (...) skipped most exampes regarding comment length
    // Compatability for table cells starting with blanks; *evil MediaWiki parser!*
  

• ./templates/consensus_revision.tpl.php: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

    // Important! The text node in $revsion->text is wrapped in CDATA tags, and these must be removed first..
  

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

          // Check permissions for foreign (non-owner) edits  TODO: the groups permission check is not implemented yet.
  

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

  ./consensus_users/consensus_users.tpl.php:34:          if ($group_counter < 4) { // Set the maximum number of groups to display.
  ./consensus.inc:416:    $this->insertUser($author); // Load user tags.
  (...) skipped most exampes regarding comment length
  ./parser/wiki2xml.php:1215:    if ( $c != "|" && $c != "!" ) return FALSE ; // No possible table markup
  

• There should be a space before and after operators like ==, ===, && and ||. See http://drupal.org/node/318#controlstruct
  

  consensus.inc:86:    if ($simple&&$dom) {
  help/wikitext.html:53:    <td><p><tt>== Level 2 ==</tt><br>
  help/wikitext.html:54:        <tt>=== Level 3 ===</tt><br>
  help/wikitext.html:55:        <tt>==== Level 4 ====</tt><br>
  help/wikitext.html:56:        <tt>===== Level 5 =====</tt><br>
  help/wikitext.html:57:        <tt>====== Level 6 ======</tt></p></td>
  js/consensus.js:2132:    case (action =='insert_revision'):
  parser/wiki2xml.php:1239:    // Reached a ||, so return blank string.
  parser/wiki2xml.php:1240:    if ( $this->nextis( $b , "||" , FALSE ) ) return "" ;
  parser/wiki2xml.php:1241:    // Reached a ||, so return blank string
  parser/wiki2xml.php:1340:        ( $c == "|" && $sameline && $this->nextis( $b , "||" , FALSE ) ) OR
  

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

  parser/wiki2xml.php:97:    // if ( !isset ( $this->profile[$f] ) ) $this->profile[$f] = 0 ; // PROFILING
  (...) skipped most exampes regarding comment length
  parser/wiki2xml.php:1470:    if ( !$this->p_article( $a , $xml ) ) return $this->errormessage ;
  

• There should be no space after the opening "(" of an array, see http://drupal.org/node/318#array
  

  consensus_users/consensus_users.module:275:    'author-current' => array( 'name' => t('No. of active contributions'), 'image' => 'bullet-edit.png'),
  parser/wiki2xml.php:19:  var $protocols = array( "http" , "https" , "news" , "ftp" , "irc" , "mailto" ) ;
  parser/wiki2xml.php:308:      $found = in_array( ucfirst( $target ) , $this->template_list ) ;
  parser/wiki2xml.php:571:    if ( !$this->scanplaintext( $b , $x , array( "]" ) , array( "\n" ) ) ) return FALSE ;
  parser/wiki2xml.php:961:    if ( !$this->scanplaintext( $b , $tag , array( " " , ">" ) , array( "\n" ) ) ) return FALSE ;
  parser/wiki2xml.php:1082:    if ( !$this->scanplaintext( $b , $name , array( " " , "=" , ">" , "/" ) , array( "\n" ) ) ) return FALSE ;
  

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

  ./consensus.inc:645:    $dom_node= dom_import_simplexml($text);
  ./parser/wiki2xml.php:384:        $v = explode( '=' , $v , 2 ) ;
  ./templates/consensus_item.tpl.php:16:  class="consensus_item c_<?php print (string)$current->attributes()->type; ?>
  

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

  ./consensus.info ./consensus_groups/consensus_groups.install ./consensus_groups/consensus_groups.module ./consensus_groups/consensus_groups.tpl.php ./consensus_groups/consensus_groups.js ./consensus_groups/consensus_groups.info ./consensus_users/consensus_users.module ./consensus_users/consensus_users.tpl.php ./consensus_users/consensus_users.info ./consensus_users/consensus_users.js ./consensus_users/consensus_users.install ./consensus.theme.inc ./consensus.items.inc ./README.txt ./consensus.module ./consensus.inc ./js/consensus.js ./consensus.install ./parser/wiki2xml.php ./templates/consensus_revision.tpl.php ./templates/consensus_document.tpl.php ./templates/consensus_item.tpl.php ./consensus.admin.inc ./consensus_toolbox/consensus_toolbox.js ./consensus_toolbox/consensus_toolbox.info ./consensus_toolbox/consensus_toolbox.module ./consensus_slider/consensus_slider.js ./consensus_slider/consensus_slider.info ./consensus_slider/consensus_slider.module ./css/consensus.css ./consensus.edit.inc
  

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.

I found some strange things again; some of the lines claimed to be longer than 80 chars actually aren't. Since I ran the script in unix, there should be no problem in my setup but I can't figure out what it is.

HTH, D.

(Of course any hints on mistakes I made with the pareview script are highly appreciated! Thx!)

Hi Tommy,

I ran the script again. Regarding your comment on the false positives, I agree on that and would suggest reporting this directly to klausi in order to check it. But it is also possible that I did something wrong with the installation of the script, as it should not check .html IMO. I don't know.

Here's the check result (I left out the false positives named above):

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

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

      // Compatability for table cells starting with blanks; *evil MediaWiki parser!*
  

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

  ./consensus.inc:417:    $this->insertUser($author); // Load user tags.
  ./consensus.inc:501:      $this->insertUser($endorse); // Load user tags.
  ./consensus.inc:641:    $revision->addAttribute('current', '1'); // Make this the current revision.
  ./consensus.inc:716:    $revision->addAttribute('current', '1'); // Make this the current revision.
  ./consensus.inc:727:    $this->insertAuthor($revision); // Load user tags.
  ./js/consensus.js:298:    $('.c_moving, .sorting, .no-nest, .c_moving_button').removeClass('c_moving sorting no-nest c_moving_button'); // Remove moving classes.
  ./js/consensus.js:1333:        forceHelperSize: true, // May not be necessary.
  ./js/consensus.js:2269:  }); // End tag of resize callback.
  ./parser/wiki2xml.php:590:    if (substr($x , -$level ) != $h ) return FALSE ; // No match
  

• There should be no space after the opening "(" of an array, see http://drupal.org/node/318#array
  

  consensus_users/consensus_users.module:275:    'author-current' => array( 'name' => t('No. of active contributions'), 'image' => 'bullet-edit.png'),
  

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

  ./consensus.info ./consensus_groups/consensus_groups.install ./consensus_groups/consensus_groups.module ./consensus_groups/consensus_groups.tpl.php ./consensus_groups/consensus_groups.js ./consensus_groups/consensus_groups.info ./consensus_users/consensus_users.module ./consensus_users/consensus_users.tpl.php ./consensus_users/consensus_users.info ./consensus_users/consensus_users.js ./consensus_users/consensus_users.install ./consensus.theme.inc ./consensus.items.inc ./README.txt ./consensus.module ./consensus.inc ./js/consensus.js ./consensus.install ./parser/wiki2xml.php ./templates/consensus_revision.tpl.php ./templates/consensus_document.tpl.php ./templates/consensus_item.tpl.php ./consensus.admin.inc ./consensus_toolbox/consensus_toolbox.js ./consensus_toolbox/consensus_toolbox.info ./consensus_toolbox/consensus_toolbox.module ./consensus_slider/consensus_slider.js ./consensus_slider/consensus_slider.info ./consensus_slider/consensus_slider.module ./css/consensus.css ./consensus.edit.inc
  

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.

Personal additions:

• I am not sure, but wiki2xml.php looks like 3rd party code, I am not yet experienced enough to judge that. But if it is, IMO it is not allowed to be included. Would someone else with more experience please comment on this, thanks!
• Regardless of being 3rd party or not, the file seems to be an include file. But as such, the .php extension is deprecated and should be changed to .inc IMO.
• The "line ending at file end" issue more and more appears like a bug, although I couldn't tell where it is. When I download your latest snapshot as a ZIP file, there are definitely single newlines at the files I checked manually. As long as this is not clearer, this would be no blocking issue for me.

BTW, I was also unsure how difficult it would be to set up the pareview script. If you are somehow familiar with *X systems, it is really not that complicated. Just try it out, there is a short intro on klausi's project page. (As of today, even ambitioned Windows fans may install a Linux system inside a "Virtual PC" environment. It really works! ;))

HTH, dave

Hey, it is really no issue. E.g. in Debian Linux you just install git and pear via apt. That's three lines and five minutes, honestly. There's no native shell scripting at all unless you intend to automate the process even more (e.g. embed it in a script that automatically pushes the output into a remote file like I have configured it to speed up). And as you can plainly see, every support for the review queue is badly needed (that's why I told myself to just do it).

OK, sorry for not looking closer at the wikixml. Just make sure that the copyright notices really do not interfere with GPL. As far as I have understood, this is considered very, very important (but obviously you have already been deeply involved with that :)).

Ok, once you feel we are ready to continue, please change status. I would then make a full test install in my review environment. :)

Hi Tommy, you better do (and also the drupalcs script which is unfortunately a little more tweaking). As I just did, I ran this setup over your repo. Results are attached. Don't be shocked, many things can be fixed with a global search/replace (I can tell, just went through it myself recently...).

What I have seen in other reviews, copyright notices are not welcome except for the readme file. Just to mention.

HTH!
-dave

Within GPL, copyright can still be used to prevent someone else claiming the work as their own ... while it's use may be unofficially discouraged within the Drupal contrib libraries by some (who might see it as contrary to the 'community' mantra within Drupal), I don't think it's actually 'forbidden'.

Just my 2 cents. :)

I know the script output seems somewhat overwhelming ... but there are a few showstoppers in there. For example ...

FILE: .../web/dp709/sites/all/modules/pareview_temp/test_candidate/consensus.inc
589 | ERROR   | Arguments with default values must be at the end of the argument list

- Avoids 'ambiguity' in argument passing

706 | ERROR | Constants must be uppercase; expected ITEM but found item
- missing '$' on '$item'

795 | ERROR | Inline control structures are not allowed
- While supported within php, the Drupal standard is to use {} on one-line 'if' structures for consistency purposes

@Tommy Kaneko has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Notify readers of the abandoned 6.x-1.x branch