[D7] Extra Columns

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxdexxa2023835git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi dexxaye:

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:

• README.txt is missing, see the guidelines for in-project documentation.
• ./extra_columns.admin.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function _extra_column_fields($form_id) {
  function extra_column_config_form($form, $form_state) {
  function extra_column_config_form_submit($form, $form_state) {
  

• ./includes/extra_columns_data.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function _extra_column_get_field_label($field) {
  

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

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. You have to get a review bonus to get a review from me.

FILE: /var/www/drupal-7-pareview/pareview_temp/css/extra_columns.css
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 12 | ERROR | Additional whitespace found at end of file
 13 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: /var/www/drupal-7-pareview/pareview_temp/extra_columns.admin.inc
--------------------------------------------------------------------------------
FOUND 27 ERROR(S) AND 6 WARNING(S) AFFECTING 28 LINE(S)
--------------------------------------------------------------------------------
   2 | ERROR   | Missing file doc comment
   5 | ERROR   | Missing function doc comment
   8 | ERROR   | Inline control structures are not allowed
  21 | ERROR   | Missing function doc comment
  26 | ERROR   | No space before comment text; expected "// get fields" but
     |         | found "//get fields"
  26 | ERROR   | Inline comments must start with a capital letter
  26 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  28 | ERROR   | Inline control structures are not allowed
  30 | ERROR   | Use "elseif" in place of "else if"
  30 | ERROR   | Inline control structures are not allowed
  34 | ERROR   | No space before comment text; expected "// get custom fields
     |         | by this module" but found "//get custom fields by this module"
  34 | ERROR   | Inline comments must start with a capital letter
  34 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  38 | ERROR   | Case breaking statements must be followed by a single blank
     |         | line
  42 | ERROR   | Case breaking statements must be followed by a single blank
     |         | line
  53 | WARNING | A comma should follow the last multiline array item. Found:
     |         | $checked_fields
  58 | WARNING | A comma should follow the last multiline array item. Found:
     |         | $form_id
  69 | ERROR   | Missing function doc comment
  86 | ERROR   | else must start on a new line
  92 | ERROR   | Missing function doc comment
  96 | ERROR   | Missing function doc comment
 100 | WARNING | A comma should follow the last multiline array item. Found: )
 107 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
 114 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
 133 | WARNING | A comma should follow the last multiline array item. Found: )
 139 | WARNING | A comma should follow the last multiline array item. Found: )
 145 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
 151 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
 157 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
 162 | WARNING | A comma should follow the last multiline array item. Found: )
 179 | ERROR   | Missing function doc comment
 188 | ERROR   | elseif must start on a new line
 194 | ERROR   | elseif must start on a new line
--------------------------------------------------------------------------------


FILE: /var/www/drupal-7-pareview/pareview_temp/extra_columns.module
--------------------------------------------------------------------------------
FOUND 62 ERROR(S) AND 5 WARNING(S) AFFECTING 40 LINE(S)
--------------------------------------------------------------------------------
   2 | ERROR   | Missing file doc comment
   5 | ERROR   | Missing function doc comment
   6 | ERROR   | Opening brace should be on the same line as the declaration
   9 | WARNING | A comma should follow the last multiline array item. Found: )
  10 | WARNING | A comma should follow the last multiline array item. Found: )
  14 | ERROR   | Missing function doc comment
  15 | ERROR   | Opening brace should be on the same line as the declaration
  23 | WARNING | A comma should follow the last multiline array item. Found:
     |         | 'extra_columns.admin.inc'
  30 | WARNING | A comma should follow the last multiline array item. Found:
     |         | 'extra_columns.admin.inc'
  31 | WARNING | A comma should follow the last multiline array item. Found: )
  35 | ERROR   | Missing function doc comment
  36 | ERROR   | Opening brace should be on the same line as the declaration
  46 | ERROR   | Missing function doc comment
  47 | ERROR   | Opening brace should be on the same line as the declaration
  48 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
  49 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "NULL" but
     |         | found "null"
  51 | ERROR   | Inline control structures are not allowed
  54 | ERROR   | No space before comment text; expected "// get the table
     |         | header and rows" but found "//get the table header and rows"
  54 | ERROR   | Inline comments must start with a capital letter
  54 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  55 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
  59 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
     |         | found "true"
  60 | ERROR   | else must start on a new line
  60 | ERROR   | Use "elseif" in place of "else if"
  60 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
  62 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
  64 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
  64 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
     |         | found "true"
  65 | ERROR   | else must start on a new line
  65 | ERROR   | Use "elseif" in place of "else if"
  65 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
  67 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
  69 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
  69 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
     |         | found "true"
  76 | ERROR   | No space before comment text; expected "// get extra columns
     |         | that will be added" but found "//get extra columns that will
     |         | be added"
  76 | ERROR   | Inline comments must start with a capital letter
  76 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  79 | ERROR   | No space before comment text; expected "// merge extra columns
     |         | into header" but found "//merge extra columns into header"
  79 | ERROR   | Inline comments must start with a capital letter
  79 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  82 | ERROR   | No space before comment text; expected "// merge extra columns
     |         | into actual rows" but found "//merge extra columns into actual
     |         | rows"
  82 | ERROR   | Inline comments must start with a capital letter
  82 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  90 | ERROR   | No space before comment text; expected "// set the modified
     |         | table header and rows" but found "//set the modified table
     |         | header and rows"
  90 | ERROR   | Inline comments must start with a capital letter
  90 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  95 | ERROR   | Inline control structures are not allowed
  95 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
  98 | ERROR   | Inline control structures are not allowed
  98 | ERROR   | No space found after comma in function call
  98 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
 101 | ERROR   | else must start on a new line
 101 | ERROR   | Use "elseif" in place of "else if"
 103 | ERROR   | Line indented incorrectly; expected 6 spaces, found 8
 105 | ERROR   | Inline control structures are not allowed
 105 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
 108 | ERROR   | Inline control structures are not allowed
 108 | ERROR   | No space found after comma in function call
 108 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
 111 | ERROR   | else must start on a new line
 111 | ERROR   | Use "elseif" in place of "else if"
 113 | ERROR   | Line indented incorrectly; expected 6 spaces, found 8
 115 | ERROR   | Inline control structures are not allowed
 115 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
 118 | ERROR   | Inline control structures are not allowed
 118 | ERROR   | No space found after comma in function call
 118 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
--------------------------------------------------------------------------------


FILE: ...var/www/drupal-7-pareview/pareview_temp/includes/extra_columns_data.inc
--------------------------------------------------------------------------------
FOUND 29 ERROR(S) AND 7 WARNING(S) AFFECTING 28 LINE(S)
--------------------------------------------------------------------------------
   2 | ERROR   | Missing file doc comment
  23 | ERROR   | Missing function doc comment
  30 | ERROR   | Missing function doc comment
  37 | ERROR   | Missing function doc comment
  46 | WARNING | A comma should follow the last multiline array item. Found:
     |         | 'extra_columns_column'
  55 | WARNING | A comma should follow the last multiline array item. Found:
     |         | 'extra_columns_add'
  64 | ERROR   | Missing function doc comment
  68 | ERROR   | No space before comment text; expected "// get the entity" but
     |         | found "//get the entity"
  68 | ERROR   | Inline comments must start with a capital letter
  68 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  69 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "NULL" but
     |         | found "null"
  74 | ERROR   | Case breaking statements must be followed by a single blank
     |         | line
  77 | ERROR   | Case breaking statements must be followed by a single blank
     |         | line
  83 | ERROR   | No space before comment text; expected "// handing field
     |         | values" but found "//handing field values"
  83 | ERROR   | Inline comments must start with a capital letter
  83 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  94 | WARNING | A comma should follow the last multiline array item. Found: 50
 102 | WARNING | A comma should follow the last multiline array item. Found: )
 157 | ERROR   | Blank lines are not allowed after DEFAULT statements
 163 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
 163 | ERROR   | No space before comment text; expected "// handling non-field
     |         | values" but found "//handling non-field values"
 163 | ERROR   | Inline comments must start with a capital letter
 163 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
 172 | ERROR   | else must start on a new line
 172 | ERROR   | Use "elseif" in place of "else if"
 214 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
     |         | found "true"
 215 | WARNING | A comma should follow the last multiline array item. Found: )
 223 | WARNING | A comma should follow the last multiline array item. Found: )
 240 | ERROR   | Missing function doc comment
 246 | ERROR   | Case breaking statements must be followed by a single blank
     |         | line
 249 | ERROR   | Case breaking statements must be followed by a single blank
     |         | line
 252 | ERROR   | Case breaking statements must be followed by a single blank
     |         | line
 255 | ERROR   | Case breaking statements must be followed by a single blank
     |         | line
 276 | WARNING | Line exceeds 80 characters; contains 83 characters
 278 | ERROR   | Missing function doc comment
 280 | ERROR   | A cast statement must be followed by a single space
--------------------------------------------------------------------------------


FILE: /var/www/drupal-7-pareview/pareview_temp/js/extra_columns.js
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 73 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

Manual Review

1. It defines a hook_uninstall() to eliminate the variables defined by the module.
2. Why would you define constants which act as variable name?
3. why do require the include file in the global scope on every single page request? You should only include it if you actually need it from function bodies?

[manual review]

Module works as I'd expect. Here are some things to consider:

• hook_init() implemantation seems heavy-handed. Are these files needed on every admin page or just those that this module defines. I'd revise this. I believe this is what dclavain was referrring to in #2.
• hook_form_alter() implementation could probably use some refactoring. I might use some switch statements and move my processing to a separate, reusable function.
• Might be nice to be more explicit with which forms/pages that you're altering on the admin page. Perhaps with a #description value that shows the path? I found this kinda confusing.
• For the "Add column" overlay. Perhaps you could display both the machine name and the field label? This might make the fields you're adding a bit easier for all user types.
• The "Add column" overlay could use a tile and description.
• Perhaps the "+ header cell" shouldn't be a part of the table at all. Might make more sense to move it out of the table to a button that says "Add Columns to this table". Having it in table just adds and empty column.

Outside of these few things, the module seems to work nicely. Could be a great module for folks who don't know how to override these pages with their own views or with custom code/alterations.

Best,
Chris

https://drupal.org/project/admin_views

This sounds like a feature that should live in the existing admin_views project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the admin_views issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

manual review:

1. Please add the exact differences to admin_views to your project page, so that users can make a better choice between the modules. admin_views is more flexible, customizable and re-usable for advanced users, while your module seems to be simpler for beginners.
2. extra_columns.install: do not globally include other files, only include them in function bodies when you actually need them. Same in extra_columns.module
3. extra_columns_init(): why do you need hook_init() which will be called on every single page request? I think you want hook_form_alter() or hook_form_FORM_ID_alter() instead, where you can add your assets only when they are actually needed.
4. extra_columns_config_form(): I think you should use system_get_date_formats() or a similar function instead of a raw db query to get all date formats.
5. extra_columns_get_column_value(): this is vulnerable to XSS exploits. If I have a node with a body of <script>alert('xss');</script> I will get a nasty javascript popup on the admin table. You need to sanitise all user provided text before printing. Please read http://drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

manual review:

1. "t('Total:') . $total": do not concatenate translatable strings, use placeholder with t() instead. Also elsewhere.
2. extra_columns_get_column_value(): the check_plain() at the end will ruin links that you create, right? You should only run check_plain() where it is needed.
3. extra_columns_get_field_label(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075
4. extra_columns_get_field_label(): don't query the DB directly, use an API function like field_info_field() or similar.
5. extra_columns_get_field_label(): this is vulnerable to XSS exploits. If I have a field with the label <script>alert('xss');</script> I will get a nasty javascript popup as soon as I add that as column. Yes, you also need to sanitize field labels, as they are user provided text.

Hi,

.admin.inc .module files
You can use something like require_once() in instead of using module_load_include() in every function you use

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

manual review:

• The Git commits are not connected to your user account. You need to specify an email address. See http://drupal.org/node/1022156 and http://drupal.org/node/1051722

But otherwise looks RTBC to me now.

Assigning to dman as he might have time to take a final look at this.

From your project description, when I read

additional columns/data to be added to the node, comment, and user tables

I thought you were talking about database columns and tables, and it wasn't until I scanned the code, and after I actually installed and tried it out I realized it was an enhancement to the normal Drupal admin screens.
Perhaps a "Why you might want to use this module" explanation would have prevented my confusion, or maybe It's just me thinking too technically.

I also see this as an alternative to VBO - which is the first thing I'd use if I wanted an extra column in the content management screen. I agree with Klausi that Admin views (which is built on top of VBO) already really beefs up the content or user management screens like this module begins to do.
My most common addition there is to switch the user to 'last editor' instead of 'owner/creator'.

Per your points against that:
* Citing a usage of 'views' as a bad thing is not very convincing. Usually "It leverages Views" is a really good thing. But OK, sometimes having less dependencies is a good point...
* Your UI is a few clicks less, and certainly easier to use. But therefore considerably less powerful and configurable right now.
* I don't think that you'll be able to even begin to match the sheer number of custom fields and field formatters all natively supported by views - which can do ... well absolutely everything already.
As a module, I don't think it's sufficiently unique to add to the existing libraries.

But it's code style to look at here. I think the coding exercise you've been through here in the style review already has been very helpful.
And on those merits, it's a good pass.

Your function structure, documentation, abstraction and style is good PHP.
Clearly you've given enough thought and attention to the data structures inside the native entities to know what's going on inside the system to support a good number of the basic examples. And the UI is good and the form additions look appropriate.

I'm looking forward to seeing you contribute a little of that usability polish to some other projects! There's a number of developer-driven modules that could benefit from a make-it-easier-to-use approach like this.

----------------

Thanks for your contribution, dexxaye!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here 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 lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

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.

Thanks to the dedicated reviewer(s) as well.