[D7] Node Detail View

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.

Found some issues:

node_detail_view.module:
Line 29:
'access callback' => 'user_access'
user_access is the default callback so the 'access callback' array element is not needed

Line 175:

  $result = db_select('node', 'n')
  ->fields('n', array('nid'))
  ->condition('title', $title, '=')
  ->execute()
  ->fetchCol();

This query allows node access control bypass, so it's a security flaw.
See https://api.drupal.org/api/drupal/modules!node!node.module/group/node_ac....
In your case just use addTag('node_access').

Line 210:
Better use theme output than directly embedded HTML code .

Nice module, but node_detail_view.module:

Line 187:
I think that should be used instead 'und' constant LANGUAGE_NONE
Before:

if ($node->language != 'und') {
  $details[$result[0]]['Language'] = $node->language;
}

After:

if ($node->language != LANGUAGE_NONE) {
  $details[$result[0]]['Language'] = $node->language;
}

I also think that the lines 195, 200, 210 and 213 you have to use theme functions.

Sorry for my english.

Hi, I found a bug:
I chose view style = list style, select all nodes and chose the option to Delete selected content and get the following message:

Notice: Undefined index: admin in node_detail_view_form_node_admin_content_alter() (line 85 of /home/quickstart/websites/organicmw.dev/sites/all/modules/node_detail_view/node_detail_view.module).
Notice: Undefined index: admin in node_detail_view_form_node_admin_content_alter() (line 87 of /home/quickstart/websites/organicmw.dev/sites/all/modules/node_detail_view/node_detail_view.module).
Warning: Invalid argument supplied for foreach() in node_detail_view_form_node_admin_content_alter() (line 89 of /home/quickstart/websites/organicmw.dev/sites/all/modules/node_detail_view/node_detail_view.module).
Notice: Undefined variable: detail in node_detail_view_form_node_admin_content_alter() (line 106 of /home/quickstart/websites/organicmw.dev/sites/all/modules/node_detail_view/node_detail_view.module).
Notice: Undefined index: nodes in node_detail_view_form_node_admin_content_alter() (line 118 of /home/quickstart/websites/organicmw.dev/sites/all/modules/node_detail_view/node_detail_view.module).

If there are no nodes, i get a notice:

Notice: Undefined variable: detail in node_detail_view_form_node_admin_content_alter() (line 106 of /home/quickstart/websites/organicmw.dev/sites/all/modules/node_detail_view/node_detail_view.module).

Hi,

Automated review: PAreview.sh on ventral.org: http://git.drupal.org/sandbox/gauravjeet/2011490.git... 0 problems found.
Coder Drupal module: OK, nothing found.

Manual review:
- Install a fresh drupal 7.22
- Install module
- Visit /admin/content page (after installation of Drupal already one node exists)
- added some nodes (article, basic page, webform)
- Visit /admin/content page
- Result as expected: select option (View Style) "List View": The traditional listing of node titles (link to corresponding node), and
- Result as expected: select option (View Style) "Detailed View" The new version of content management that gives a preview of a node.

One suggestion: You may want still a fieldset to generate your select box, it looks a little clearer....

Like zestagio in No #7 I get self same error if i choose the option to Delete selected content or if no nodes are available.

Best regards.

Hello gauravjeet_singh,

Here is some quick review.
I enabled the module in a fresh drupal installation and it worked well for me.

Just a few comments :

- You could put the link to the config more in a submenu and not in the root of the main admin menu. The link could for example be placed in user interface, in configuration menu : admin/config/user-interface/node_detail_view
Then in the .info file you could add this line, so it's possible to reach configuration form directly from modules list: configure = admin/config/user-interface/node_detail_view

- .module l.76 & 96 : "Implements"

- I get the following notice when going to admin/content :
Undefined index: #options in node_detail_view_form_node_admin_content_alter() (line 115 of/Applications/XAMPP/xamppfiles/htdocs/tdftest/sites/all/modules/node_detail_view/node_detail_view.module).
In .module l.115, you could verify if variable is set :
if (isset($form['admin']['nodes']['#options']) && count($form['admin']['nodes']['#options']) > 0) {

- .module l. 284 & 285 : I think the use of the semantic <i> tag is not necessary here and may be it would be better to use css font-style:italic but it's a detail.

- Content interface : you could adjust theming of border between nodes list and current node content (I've uploaded a screenshot / admin theme used = Seven).

Good luck !!

Hi,

I installed the module and it worked fine. It works as expected. Good work !!

Just a few minor things to take care of:
1. Still an error reported by http://ventral.org/pareview/httpgitdrupalorgsandboxgauravjeet2011490git . Please fix it asap.
2. File: node_detail_view.module

Before:

$details[$nid]['Comments'] = ($node->comment_count == 1) ? l(t('View ') . $node->comment_count . ' comment', 'node/' . $node->nid, array('fragment' => 'comments')) : l(t('View ') . $node->comment_count . ' comments', 'node/' . $node->nid, array('fragment' => 'comments'));

After:

$details[$nid]['Comments'] = ($node->comment_count == 1) ? l(t('View') . $node->comment_count . ' comment', 'node/' . $node->nid, array('fragment' => 'comments')) : l(t('View') . $node->comment_count . ' comments', 'node/' . $node->nid, array('fragment' => 'comments'));

The string inside t() should not start or end with space, it is hard to translate correctly. FYI: https://drupal.org/node/304150

3. File node_detail_view.info:

Before:

configure = admin/config/administration/node_detail_view
Right now it leads to the default admin config page rather than the config page of your module.

After:

configure = admin/config/user-interface/node_detail_view

4. File node_detail_view.module
return '<div id="node_details" style="text-align:center;color:darkgray;">Preview not available.</div>'

Its recommended to avoid inline styling. Anyways, the module has a css file, I would recommend using a class instead of inline styling.

5. node_detail_view.css

Although you can ignore this, but using color constants like "darkgray" is discouraged. Its recommended to use color codes for example: #ccc

Nice module, just couple of quick observations:
1. There are some inline styles which easily can be moved to css file
line 244
'#prefix' => !empty($options) ? '<div id="node_details" style="float:left;"><i style="color:darkgray;">Click on node title on the left to view.</i></div>' : '<div id="node_details" style="float:left;"><i style="color:darkgray;">Preview not available.</i></div>',
and line 353
return '<div id="node_details" style="text-align:center;color:darkgray;">Preview not available.</div>';

2. On the same line 353 the string 'Preview not available' will not be translated. Better use t() here.
Same applies to lines 299, 244

Just one small addition - lines 63 and 115:

      '#options' => array(
        'List View',
        'Detailed View',
      ),

Selectable options must be processed by t() as well

updating manual review links of other projects

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

I also removed the automated review from the issue summary.

manual review:

1. node_detail_view_uninstall(): no all variables are removed?
2. "t('!info', array('!info' => 'Title ( ' . $count . ' nodes )'))": wrong usage of t(), $count should be used as placeholder like t('Title (@count nodes)', ...).
3. node_detail_view_theme(): all theme functions or templates should be prefixed with your module name to avoid name collisions.
4. node_detail_view_form_node_admin_content_alter(): you are executing "strip_tags($option[$nid]['title']['data']['#suffix'])" twice?
5. Notice: Trying to get property of non-object in theme_author_info() (line 301 of /home/klausi/workspace/drupal-7/sites/all/modules/node_detail_view/node_detail_view.module).
6. Bug: The node info on the right displays my user as blocked, which is not the case.
7. node_detail_view_form_node_admin_content_alter(): I think strip_tags() is not the appropriate function of choice here to sanitize the output. filter_xss_admin() seems more appropriate. I was thinking of some possible exploits that could bypass strip_tags() but could not find any, so this does not look like a security issue.
8. <div class="node_detail_view_author_column">Roles</div>: all user facing text must run through t() for translation.
9. theme_author_info(): this is vulnerable to XSS exploits. If I have a role with the name <script>alert('XSS');</script> I get a nasty javascript popup on the node detail page. You need to sanitize all user provided text before printing. Please read https://drupal.org/node/28984 again. This is not a security issue per se because a trusted permission is needed to inject the malicious role name in the first place, but it should be fixed anyway.

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

updating links for reviewed other projects

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

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

  
  FILE: /home/klausi/pareview_temp/node_detail_view.module
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AND 5 WARNING(S) AFFECTING 5 LINE(S)
  --------------------------------------------------------------------------------
   298 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
   298 | WARNING | Only string literals should be passed to t() where possible
   299 | ERROR   | Functions must not contain multiple empty lines in a row;
       |         | found 2 empty lines
   301 | WARNING | Only string literals should be passed to t() where possible
   350 | WARNING | Only string literals should be passed to t() where possible
   350 | WARNING | Only string literals should be passed to t() where possible
   355 | WARNING | Only string literals should be passed to t() where possible
  --------------------------------------------------------------------------------
  

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.

manual review:

• theme_node_detail_view_author_info(): why do you send the user name and the UID through t()? Those are user provided dynamic variables, so they should not be translated.

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

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

Hello, your module is cool.

Manual review:

node_detail_view.module
l.122: unused variable $header.

Bug report
Ajax conflict: With a product display (node with product reference field) on drupal commerce, when I click on "Add to cart" button. I am rerouted to a blank page (http://example.com/system/ajax).

Bye.

Project applications are tasks.

The bugs reported in #28 don't seem to be blockers and there were no other objections for more than a week, so ...

Thanks for your contribution, gauravjeet_singh!

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.