[D7] Content Tab

Hello a_thakur,
why are you doing this with a standalone module?
You can do this with two pages build by the views module including the tab menu entry.
A views solution is more flexible for example to show any kind of entities a user is publishing.

The next thing is you are only using the permission "administer nodes". To be more flexible you can use hook_permission() to create a new permission for this functionality. In this way a sitebuilder can give this permission to a moderator role:
http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...

Some review of the code:

• Your strings are not translatable (e.g array('No', 'Yes')). Please use the t() function: http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7
• Use indent of 2 and avoid trailing whitespace at the end. – http://drupal.org/coding-standards#indenting

There are a lot of coding standard issues in your code.
Please use modules like coder to test yourself.
Here is the result of http://drupal.org/project/pareviewsh Online Version:
http://ventral.org/pareview/httpgitdrupalorgsandboxashishthakur1511158git

@logeman aww, providing the link / attached file containing the automated report is much better then posting these ugly reports directly into issues, please edit your comment and short it ;) thanks

Hiya - yes views can be complex for beginners, and (omg) some sites would need this module as they do not use views [like me sometimes].

OK. in the install file, you have listed the module file, this is only required if you are defining a class.

-files[] = content_tab.module

Set your editor to show the magic print margin line of 80 characters. Keeping these short assists code readability among other things. So line 4 is too long and the comments on line 12 could be extended out, etc.

Line 12+ - watch your spacing. You should use no tabs (hard to tell as my editor converts all tabs to spaces) and use indentation of 2 spaces. The automated code reviews should be picking these up, but best to do it before the review is run or you will get a message for almost every loc (line of code).

Line 16
You could use the menu wildcard loader "%user" rather than "%user_uid_optional".

I would define an 'All' tab for all content types and make this the default local task, allowing the individual types all be standard local task. If you are unsure of what I am saying here, take a look at the first couple of callbacks defined in taxonomy_menu(). The List is the tab that is bound to 'admin/structure/taxonomy'.

It is best to avoid underscores in the menu items too. For example, from the node module:

  foreach (node_type_get_types() as $type) {
    $type_url_str = str_replace('_', '-', $type->type);

Notice that in the context of just node types, "$type" can be used rather than "$node_type". Particularly as you have already used "$types".

i.e.

  foreach (node_type_get_types() as $type) {
    $type_url_str = str_replace('_', '-', $type->type);
    $items['user/%user/content/' . $type_url_str] = array(
      'title' => $type->name,
      'access callback' => 'node_access',
      'access arguments' => array('create', $type->type, 1),
      'page callback' => 'content_tab_get_content_list',
      'page arguments' => array(1, $type->type),
      'type' => MENU_LOCAL_TASK,
    );
  }

The permission here is "user can create nodes of type xxx". This makes sense, but you loss the functionality of listing the users nodes when that user loses the right to create nodes of that type. Also, you are restricting access based on the authors permission rather than the users permission.

It could be worth defining an custom access callback. Maybe something in the lines of:

if (user_access("has permission to access content tabs")) {
  // Show tab if user can create these pages, even if user doesn't have any atm.
  // Else, show tab if author has created pages of this type.
}

This would need a hook_permissions() and custom menu access callback.

content_tab_content_list() has a security issue.
This has unfiltered content being rendered ($user->name). It is best to use something like this:

    $output = t('Click on the tabs to get the list of content written by %name of a particular content type',
      array('%name' => $user->name));
    return $output;

!name would pass through raw values.
%name or @name both use check_plain($value), with the former adding em HTML tags.

I'll let you get the formatting fixed for the rest before continuing (mainly spacing).

This is probably a lot to take in but mostly these are all trivial things that you will do in your sleep after your first couple of modules :)

I think that we could use this on one of our projects. So in this regards, ping me if you would want paging / sorting on the page, plus extended permissions to fine turn it.

i.e. Maybe
-- global access checks that must be meet before seeing the tabs
view any users content tabs
view own content tabs
-- type specific
view any users all content tab
view own all content tab
view any users [node type] content tab
view own [node type] content tab

We need administrators to see users blog and events, and individual users to see their own blogs and events.

Just realized that there is another security issue here, there is no db_rewrite_sql in Drupal 7, but the tagging system allows other modules to provide additional access administration tasks.

See
http://api.drupal.org/api/drupal/includes%21database.inc/function/db_rew... for a brief overview.

Another minor security related issue. Only some users get to see unpublished content.

  global $user;
  $view_unpublished_nodes = user_access('bypass node access') ||
      ($account->uid == $user->uid && user_access('view own unpublished content'));
  if (!$view_unpublished_nodes) {
    // Additional SQL for including only node.status = 1
  }

The spacing is a good place to start rather than worrying too much about extending the functionality at this stage.

# This is indented by 4 spaces, should be only 2
    if ($count == 0) {
# And this is 8 when it should be just 4 (2 from the if above and 2 for the inner if)
        return t("No content written by the user yet");
    }
    else {
        $output = theme('table', array(
                                   'header' => $header,
                                   'rows' => $rows,));
        $output .= theme('pager', array(
            'tags' => array()
        ));
        return $output;
    }

A formatted example would be

  if ($count == 0) {
    return t("No content of this content type");
  }
  else {
    # These are fairly small, so they can go inline.
    $output = theme('table', array('header' => $header, 'rows' => $rows));
    $output .= theme('pager', array('tags' => array()));
    return $output;
  }

Have another quick look at the array section, http://drupal.org/coding-standards#array.

For readability, single quotes on strings are preferred c/f rather than double quotes

t("No content of this content type");
t('No content of this content type');

PHP reads 0, '0', FALSE, NULL as all FALSE, so you can negate the logic:

if ($count) {
  # not empty
}
else {
  # empty
}

Or even better, use the table #empty property

  # rather than returning just "STRING", a rendering array is better so other modules can alter individual components.
  $build['node_table'] = array(
    '#theme' => 'table',
    '#header' => $header,
    '#rows' => $rows,
    '#empty' => t('No content of this content type');,
  );
  $build['node_pager'] = array('#theme' => 'pager');
  return $build;

Going right back to the start.

For the comments, the first line should be short and to the point. Then leave a blank line, followed by more in-depth information as required. From the core menu module:

/**
 * @file  << on line by itself (good)
 * Allows administrators to customize the site's navigation menus. << Single brief overview (yours is a bit long)
 *
 * A menu (in this context) is a hierarchical collection of links, generally
 * used for navigation. This is not to be confused with the 
 * @link menu Menu system @endlink of menu.inc and hook_menu(), which defines << longest line is under 80 characters.
 * page routing requests for Drupal, and also allows the defined page routing
 * URLs to be added to the main site navigation menu.
 */

i.e. From the guidelines (http://drupal.org/node/1354)

Comments should be word-wrapped if the line length would exceed 80 characters (i.e., go past the 80th column, including any leading spaces and comment characters in the 80 character count). They should be as long as possible within the 80-character limit.

Normally, drop any default calls to various functions:

db_select()->condition('uid', $uid, '=')
db_select()->condition('uid', $uid)

Feel free to ignore this one if you think that you need it to follow the logic. db_select()->condition() defaults to IN if $value is an array, and = otherwise.

This should keep you going for a while. Ping the thread when you want more.

PS: I would recommend the automated testing for the remaining formatting issues, this way you get feedback instantly. Just ask if you do not understand anything that is returned by these tools.

Nice, much better!

1) There are some exceptions to the 80 print margin rule. One is the t() strings. These need to be automatically parsed so you should keep these on a single line. (PS: And content typo) I personally do not know if the t() arguments should be on the same line or not, but in D7 most of the user module ones are. So:

-  $output = t('Click on the tabs to get the list of conten written by %name
-      of a particular content type', array('%name' => $user->name));
+  $output = t('Click on the tabs to get the list of content written by %name of a particular content type', array('%name' => $user->name));

2) You will need to check if the user can see unpublished nodes, i.e. the 'view own unpublished content' permission. This would be enough to trigger a "low risk" security warning and force an update.

3) To maintain "Best practice" with the node queries, tag these so that other modules can append access control alterations to these. Some sites allow you to create some types, but then lock out edit / view permissions. Another example, is domain access module that hides nodes if these are assigned to other domains.

 $query->addTag('node_access')

PS: If you find that any of the DB chained queries fail, check each individual function to see if it ends with "return $this;". The api here is not 100% hooked into this chaining pattern. If not then $query->a()->b() will fail. Even after a year using D7 I forget which ones do and which ones do not, but it can be a big wtf when you find one that doesn't.

4) Add a single new line to the end of the file. This causes additional lines in GIT patches.

5) Default local menu tasks inherit their parent parameters, and look untidy if not on the very left hand side of the list (like if you had a content type called "ACDC" or similar). So

  $items['user/%user/content/all'] = array(
    'title' => 'All',
    'type' => MENU_DEFAULT_LOCAL_TASK,
    'weight' => -10,
  );

6) From the automated review: http://ventral.org/pareview/httpgitdrupalorgsandboxashishthakur1511158git

The comments from line 16-18 could just be deleted (or cleaned up). Then it is mainly space issues with comments:

-  //No space and no trailing full stop
+  // With space and full stop.

And the final comma on multi-line arrays.

Tests are bonus points but I do not think that they would be needed for the application.

Sorry, feeling a bit nick-picky here:

content_tab.info
1) Full stop at the end of the description.
2) Either commit the test file or remove the include from the info file.

content_tab.module

3) The function content_tab_content_list() is not used. Are you planning to use this still or to delete it?

4) There is still a space issue with the inline comments.

-  //Database query to get the number of posts in a particular content type.
+  // Database query to get the number of posts in a particular content type.

5) This line can easily be mis-read:

  $view_unpublished_nodes = user_access('bypass node access') || ($account->uid
    == $user->uid && user_access('view own unpublished content'));

It is better to keep the two conditional components on lines by themselves.

  $view_unpublished_nodes = user_access('bypass node access') 
      || ($account->uid == $user->uid && user_access('view own unpublished content'));

Notice that I have dropped the OR to the next line, the operator being used should always do this. A concat would be the same. i.e.

  $a ='aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'
      . 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'
      . 'ccccccccccccccccccccccccccccccccccccccccccccc';

This allows you to see exactly how one line relates to the next instantly, rather than having to read the line of code.

6) Spaces. If a single line of code is split into two lines, there are 4 spaces rather than just two. This visually separates a new line of code and a continuation of a single line of code.

**if*(a
******||*b)*{
****$c = $a;
**}

**if*(a*||*b)*{
****$c = $a;
**}

7) I take it that you must not like displaying empty tables?

  $output = theme('table', array(
    'header' => $header,
    'rows' => $rows,
    'empty' => t('This text is displayed if the table is empty, otherwise it is hidden.'),
  ));

If that is the case, do not use the $count SQL, use the row count. For starters, it is another query to maintain, another system call, and thirdly, it is wrong at the moment!

  if (empty($rows)) {
    return t('No content written by %name yet', array('%name' => $account->name));
  }
  else {
    $output = theme('table', array('header' => $header, 'rows' => $rows));
    $output .= theme('pager', array('tags' => array()));
    return $output;
  }

8) Full stops on the empty text.

'No content written by %name yet.'

9) The query formats are almost there: 4 spaces if you chain these to a new line.

**if (!$view_unpublished_nodes) {
**$query = db_select('node', 'n')
****->condition('uid', $uid)
i.e.
**if (!$view_unpublished_nodes) {
****$query = db_select('node', 'n')
********->condition('uid', $uid)
********->next()
********->next();
****$b = $c;

10) This is not listed anywhere in the coding standards, but it is something that you will be very unlikely to see in other published code.

  if (!$a) {
    // do something
  }
  else {
    // do something
  }
 AND
  if ($a) {
    // do something
  }
  else {
    // do something
  }

See the difference? Many of your conditionals start like the first compound if statement.

If you are skim reading the code, both look the same and you would incorrectly read this as "if (TRUE) do something", while it is really saying "if (FALSE) do something"

This is not part of the review, but may help.

You are duplicating effectively the same logic 4 times when doing the main queries, 6 time if you included the incorrect count queries, and replicating the table generation twice. These two functions are reworked examples of how you could unify things (pure untested hack).

You can modify the menu callbacks to use the single function.

/**
 * Helper function to generate the query for the users content listing.
 * 
 * @return SelectQueryInterface
 *   The called object.
 */
function _content_tab_content_query($account, $view_unpublished, $type = NULL) {
  $db_select = db_select('node', 'n')
    ->condition('uid', $account->uid)
    ->condition('status', '1')
    ->addTag('content_tab')
    ->addTag('node_access')
    ->fields('n')
    ->extend('PagerDefault')
    ->extend('TableSort')
    ->limit(10);
  if (!$view_unpublished) {
    $db_select->condition('status', '1');
  }
  if (!empty($type)) {
    $db_select->condition('type', $type);
  }
  return $db_select;
}

/**
 * Function of get the list of content by a particular user of particular
 * content type.
 */
function content_tab_get_content_list($account, $node_type = NULL)  {
  global $user;
  $noyes = array(t('No'), t('Yes'));

  // Check to see whether only allowed users are able to see unpublished nodes.
  $view_unpublished = user_access('bypass node access')
      || ($account->uid == $user->uid && user_access('view own unpublished content'));

  $header = array(
    array('data' => t('Title'), 'field' => 'title', 'sort' => 'asc'),
    array('data' => t('Created'), 'field' => 'created'),
    array('data' => t('Changed'), 'field' => 'changed'),
    array('data' => t('Published'), 'field' => 'published'),
    array('data' => t('Sticky'), 'field' => 'sticky'),
    array('data' => t('Promoted to front page'), 'field' => 'promoted'),
  );
  if (!$view_unpublished) {
    unset($header[3]);
  }
  
  // The logic being used is that the user needs to be able to create content
  // or to have content within the tab. Menu controls the first bit, the user
  // ID completes these restrictions.
  $db_select = _content_tab_content_query($account, $view_unpublished, $node_type);
  $result = $db_select->orderByHeader($header)->execute();
  foreach ($result as $data) {
    $row = array(
      l($row->title, "node/$row->nid"),
      format_date($row->created),
      format_date($row->changed),
      $noyes[$row->status],
      $noyes[$row->sticky],
      $noyes[$row->promote],    
    );
    if (!$view_unpublished) {
      unset($row[3]);
    }    
    $rows[] = $row;
  }
  if (empty($rows)) {
    return t('No content written by %name yet.', array('%name' => $account->name));
  }
  else {
    $output = theme('table', array('header' => $header, 'rows' => $rows));
    $output .= theme('pager');
    return $output;
  }
}

Enjoy :)

Agree with Alan.
A few more issues I found while going through the code.

line #55, #63-69, #132, #133, #144, #154-159, #218, #223, #226, #227 content_tab.module
needs a comma after the last element of the array (http://drupal.org/coding-standards#array)

line #59, content_tab.module
Instead of running a count query with a wildcard, a specific key can be used. I suggest nid as it would never be null.
SELECT COUNT(*) FROM {node} WHERE
can be replaced by
SELECT COUNT(nid) FROM {node} WHERE
or you might like to check out http://drupal.org/node/310075#count_queries

line #142, content_tab.module
double space after the closing parenthesis and curly bracket instead of one

Hi,

.info file

There is no need to put quotation marks in the description field.

.module file

hook_menu()

variable_get('content_tab_weight'), - you should provide some default value in case value is not set.
the same with content_tab_unset_column variable

content_tab_unset_column()

I believe that two nested if and the first checking !$value would be better readable.
What's more - you have only one $key at the time. Consider changing this big if structure to switch case or add elseif to better performance (now php code checks all of the conditions even if one of them was already met).

content_tab_operations_permission()

What if user has access to edit all content and delete all or Bypass content access control but not explicity to edit or delete own? Or there will be some content access modules as TAC?
He or she will not see management links.
Use node_access function instead of user_access.
What's more - I do not see content tab on any account (event my superuser) until I set some configuration options, because weight variable does not provide any weight.

I've also have warnings:
Warning: Invalid argument supplied for foreach() in content_tab_unset_column() (line 152 of C:\wamp\www\dashboards\sites\all\modules\drupal.org\content_tab\content_tab.module).
Warning: Invalid argument supplied for foreach() in content_tab_get_content_list() (line 124 of C:\wamp\www\dashboards\sites\all\modules\drupal.org\content_tab\content_tab.module).

Your module tells me that I have no content even though i have some. I believe it's fault of these warnings (I haven't made any changes to the settings yet, so thats why I think).

Thats the reasons why I put your application into needs work. Other issues in my opinion are minor and should not be a blocker.

admin.inc file

Consider using range function instead of for loop. Of course it is not any mistake to use for loop. It is only my thought.

'#default_value' => variable_get('content_tab_weight', '0'), - probably it should be 0. I know that php has no control of variable types, but you are using it as number 0, not string 0.

content_tab_admin_settings_submit()

All values passed by system_settings_form are saved in variables by default. There is no need to manually add them.

user_access can be used here but then you need to take into account 'edit any ' . $data->type . ' content' and 'delete any ' . $data->type . ' content'.
node_access would take care of this by itself.

Access

Not sure what you are doing here:

/**
 * Function to handle multiple access arguments.
 */
function content_tab_access($account = NULL) {
  global $user;
  return user_access('view content tab', $account) ||
    user_access('administer site configuration', $user);
}

This reads - show the page to anyone if the account owner has access to that tab OR if the logged in user is a site admin (administer site configuration). First one has no context to the logged in user, the second has no context to permissions related to see node listings.

The saving grace there is:

->addTag('node_access')

Which will enforce the view restrictions of the content itself as least!

It could be legitimate in this use to check if a user account is allowed to see these before showing the tabs to admin users, but then some other restrictions should to be applied.

Again with the operations in the table. The checks are against the account rather than the current logged in user. Basically, use user_access('permission here') for all access checks against the logged in user. Anything else is to check if the specified user can do something and is pointless in most circumstances.

Sooooo.... maybe it would be cleaner to add a permission to "access own content tabs" and "access any content tabs". These would define that base access restrictions, then limit tabs based on the users account that has the listing against (breaking the standard rule) and have the query finally filter by the content that the user is allowed to see.

Table empty text

Being a parrot and repeating myself:

  if (empty($rows)) {
    return t('No content written by %name yet.', array('%name' => $account->name));
  }
  else {
    $output = theme('table', array('header' => $header, 'rows' => $rows));
    $output .= theme('pager');
    return $output;
  }

Doesn't this look cleaner??

  $output = theme('table', array(
    'header' => $header,
    'rows' => $rows,
    'empty' => t('No content written by %name yet.', array('%name' => format_username($account))),
  ));
  $output .= theme('pager');
  return $output;

Note that I have used format_username() here, this masks the users login name if the site requires it.

Weight element

Change FAPI type and drop the options

  $form['content_tab']['content_tab_weight'] = array(
    '#type' => 'weight',
  );

But big plus as the code now looks tiny and clean (by a quick glance)

This looks like a duplicate of the core tracker module - please add the differences to that to your project page. And you are right, this could be easily done with Views, which would also allow more flexibility when configuring the output. Have you thought about just using Views if it is available and falling back to this primitive table if not? Anyway, I guess this could still be useful.

manual review:

1. Notice: Undefined variable: rows in content_tab_get_content_list() if there are no nodes in a content type. Please enable PHP notices in your development environment.
2. PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'published' in 'order clause': SELECT n.* FROM {node} n WHERE (uid = :db_condition_placeholder_0) ORDER BY published ASC LIMIT 10 OFFSET 0; Array ( [:db_condition_placeholder_0] => 1 ) in PagerDefault->execute() (line 79 of drupal-7/includes/pager.inc). Occurs if I click on the operations column header or published.

"just using Views if it is available and falling back to this primitive table if not..."

I would rather create a doc on d.o "10 steps to replicate content tab using views" and post its link on my project page than adding extra lines of code. This module is an out-of-box solution and does enough.
Its my personal opinion though.

Ok, looks RTBC to me now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

@a_thakur: I can sign off on this. I just have one question: could you please confirm that Rules Plus will remain a sandbox? I think that would be rather confusing for users, otherwise.

Thank you for clarifying, @a_thakur, and thank you for contributing.

I updated your account to let you 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 to the dedicated reviewers as well.