[D7] Shortened Comments

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

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.

1) shortened_comments.info
remove

core = "7.x"
project = "shortened_comments"
datestamp = "1392733020"

this is generated by drupal

2) use drupal coding standards e.g (2 spaces)
3) Missing install file for removing your variables when uninstalling your module.
4) Redundant module, why not use a custom field formatter?
5) $build['comments']['comments'][$delta]['comment_body'][0]['#markup'] = $trim; -> seems kind of harcoded, what if the user removes this field.

For point 4, I think with custom filed formatter it will be possible, when displaying only trimmed comment.

I think you should keep the field display configuration on the node manage display page. Because this can be confusing for other dev landing on a project using this module, they will expect that a custom formatter is used for this behaviour. That's why i advise you to not use node view alter and use a custom formatter for the comment field.
What would imply you remove the node types from the configuration page and leave that to the manage display configuration.

btw have you tested your code with display suite? because i have a good feeling that it will not work because of node view alter

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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

@stefank, thank you for your contribution!

I like the concept of your module but IMHO currently lot of work is left in this module.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. http://pareview.sh/pareview/httpgitdrupalorgsandboxstefank2199467git reported number of issues that need to be address.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

(*) No: Follows the guidelines for in-project documentation and the README Template. It is not clear from this file, how this module works and how user will configure this with content type.

Code long/complex enough for review

No: Follows the guidelines for project length and complexity. Added PAReview: Single project promote tag.

Secure code

(*)No. If "no", list security issues identified.

1. shortened_comments_field_formatter_view: this function is vulnerable to XSS. User provided input are not sanitize before use, make sure to read https://www.drupal.org/node/28984 again.
   

   if (!empty($sc_char) && strlen($value) >= $sc_char) {
           $text = (strlen($value) > $sc_char) ? substr($value, 0, $sc_char) . '... ' : $value;
   

   Also you are using substr() function directly with $value, If I put some html tag in $value, substr will break that string. Of course we will put html tags as this formatted would be used with editor. Better to use drupal_substr() and also go through some thread where you would found how harmful is substr in case html tags are coming with string.

Coding style & Drupal API usage

1. (*) Unable to use this module as functionality not working as expected because of following code.
   

   $perm = array_intersect(array('administrator'), $user->roles);
   

   $perm already hold some value and it can never be empty because of above code. Now moving further, following case never be true because $perm can never be empty.

   if (user_access('access shortened comments') && empty($perm)) {
   

   Then I tried this code logic by removing && empty($perm) and found there is no else condition for following case.

   if (!empty($sc_char) && strlen($value) >= $sc_char) {
           $text = (strlen($value) > $sc_char) ? substr($value, 0, $sc_char) . '... ' : $value;
           $text .= t($sc_text);
           $element[0] = array('#markup' => $text);
         }
         if ((!empty($sc_words) && isset($value)) && str_word_count($value, 0) > $sc_words) {
           $words = str_word_count($value, 2);
           $pos = array_keys($words);
           $text = substr($value, 0, $pos[$sc_words]) . '... ';
           $text .= t($sc_text);
           $element[0] = array('#markup' => $text);
         }
   

   It means first it goes to first if condition then in second if condition that doesn't make any sense. Every time $element value overwrite by another if case.

   After this, following code is there and again $element value overwrite and if condition comes true for most of the cases.

   
   if ((strlen($value) < $sc_char || str_word_count($value, 0) < $sc_words) || !empty($perm)) {
         $element[0] = array('#markup' => $item['safe_value']);
       }
   

   This all above logic is not well organised and tested as it is not working as intended.

2. hook_help() is missing in your module.
3. (+) Extra html tag (</br>) is appearing on admin interface of this module that looks wired.
   
   
   
   
   
4. Project Page also not well organised, contains very few information about this module. Need to be extend little bit more.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks Again!

manual review:

1. project page: how are the comments shortened? So part of the comment content is removed when the user saves the comment? Or is this like a node teaser where part of the comment is shown with a "read more" link of some sort? A screenshot would also be helpful, see also the tips for project pages: https://www.drupal.org/node/997024
2. what is the difference to just use the comment display setting of Drupal core? Go to admin/structure/types/manage/article/comment/display for the article content type and just set the format to trimmed? Please add the difference to the project page.
3. shortened_comments_field_formatter_view(): don't check for the administrator user role for access, that might have a different meaning on different Drupal sites. Always use permissions, as you already do - so that should be enough.
4. So there is no "read more" link? The remaining content of the comment is just hidden and can not be viewed anywhere?

But that are not critical application blockers, otherwise looks good to me. The code is slightly over 120 lines and you seem to have understood the security principle of filtering user provided text against XSS, so I think we can promote you to a git vetted user. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

I'd like to see klausis questions addressed before continuing here

Agreed, maybe too many open questions before an RTBC :-)

'sc_text' => 'The Comment has been shortened.',

I'm pretty sure you should translate the default text too.

'#title' => t('<h1>OR</h1>How many words should be displayed'),

h1-tags are usually reserved for the sites main title.

Maybe make the '... ' configurable too ? :)

This is a pretty small module and there's not much to review; but what's there looks good enough.

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 for your contribution!
And thanks to the dedicated reviewer(s) as well.