Pager for Content type

Run coder module and check it once.

pager_for_content_type.module
Line 50: There should be no trailing spaces
Line 57: There should be no trailing spaces
}
Line 60: There should be no trailing spaces
}
Line 70: There should be no trailing spaces
* @return array
Line 115: Missing parenthesis after function name
* Implements hook_theme
Line 132: There should be no trailing spaces
* @return string
Line 180: There should be no trailing spaces
* @return string
Line 197: There should be no trailing spaces
}
Line 200: There should be no trailing spaces
}
Line 205: There should be no trailing spaces
}
pager_for_content_type.admin.inc
Line 23: There should be no trailing spaces
Line 25: There should be no trailing spaces
Line 31: There should be no trailing spaces
Line 32: There should be no trailing spaces
Line 38: There should be no trailing spaces
Line 40: There should be no trailing spaces
Line 52: There should be no trailing spaces

README.txt
Ensure that the number of chars per line does not exceed 80 chars. Refer to the following link
http://drupal.org/node/161085

1. indentation is worng in total. Please have only 2 spaces for indentation.
2. Line 20: Please use placeholders. please refer to this link http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/t/7
3. Line 94: in page_for_content_type_menu, plz declare $items array, otherwise it gives an warning.
4. All functions should have doxygen comments.

Indentation errors in the following functions:
1.pager_for_content_type_permission.
2.pager_for_content_type_menu.
3.pager_for_content_type_theme.
4.theme_pager_for_content_type.
5.pager_for_content_type_admin_settings.

Automated Review:
Review of the 7.x-1.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/pager_for_content_type.module:
   +56: [normal] else statements should begin on a new line
   +192: [normal] else statements should begin on a new line
   +194: [normal] else statements should begin on a new line
   +198: [normal] else statements should begin on a new line
  
  Status Messages:
   Coder found 1 projects, 1 files, 4 normal warnings, 0 warnings were flagged to be ignored
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

Manual Review:

• "'access arguments' => array('administer site configuration'),": don't abuse this permission, should create your own.
• Don't think it needs validation currently, so you should remove empty function.
• Keep one line space between one function ends & other starts just.
• You don't need to pass TRUE while returning system_settings_form as that's default to TRUE.
• Line no 9 [ pager_for_content_type.admin.inc ] Some Typo errors & doc could be more elegant I think [ like General configuration form for controlling behaviour or something ... ] This may help Documentation standards => http://drupal.org/node/1354

Thanks

Why do you apply with more than one project anyway?

Please decide which one you want to be your "approval project".

@svipsa: Ah ... I see now what you mean (or I think so). It is only one project per developer that needs to go through this review. Once you have the git vetted user role you can create as many projects as you want - without approval. (We then trust you that you only create useful, decent modules.)

Yes, that's what I mean and as I can tell from just a few days reviewing intensively I really pray you chose which one of your projects is to be the one. The queue is horribly long and one project per applicant should really be enough for the sake of less waiting for all, don't you agree?

BTW: Why don't you join the reviewers' team a bit? Everyone who has some understanding of the process should really do so for the benefit of all!

Setting back to needs work in order for you to chose one project for appliance. OK? Thx!

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

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

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: ...l/modules/pareview_temp/test_candidate/pager_for_content_type.admin.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 5 WARNING(S) AFFECTING 5 LINE(S)
  --------------------------------------------------------------------------------
    9 | WARNING | Line exceeds 80 characters; contains 87 characters
   24 | WARNING | A comma should follow the last multiline array item. Found:
      |         | FALSE
   30 | WARNING | A comma should follow the last multiline array item. Found: )
   38 | WARNING | A comma should follow the last multiline array item. Found:
      |         | FALSE
   46 | WARNING | A comma should follow the last multiline array item. Found: )
  --------------------------------------------------------------------------------
  
  
  FILE: .../all/modules/pareview_temp/test_candidate/pager_for_content_type.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 3 WARNING(S) AFFECTING 3 LINE(S)
  --------------------------------------------------------------------------------
    34 | WARNING | A comma should follow the last multiline array item. Found: )
    39 | WARNING | Format should be * Implements hook_foo().
   128 | WARNING | A comma should follow the last multiline array item. Found: )
  --------------------------------------------------------------------------------
  

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

  ./pager_for_content_type.admin.inc ./README.txt ./pager_for_content_type.module
  

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.

Manual review:

• There are still files in your master branch. You should replace them as described in http://drupal.org/node/1127732 at step 5.

Good idea for a module, I have had to build this functionality before, it would have been nice to have a starting place. I have an issue with the name.

This module doesn't really create pages for content types. It creates chronological next/previous links from one node to the next/previous. The name implies that the module breaks up a node into pages.

Maybe "previous and next node" would be better.

The problem I have is with the word "page." There are a few paging content module out there already: Pager, Pagination, and Flippy. Notice how they all have something to do with pages in the name of the module. They all break a single piece of content into several pages. Your module doesn't do this. Paging a node already has this meaning.

I expect that your module will not be found when someone is searching for your module's functionality.

Maybe no "page" name at all. Probably rather something with "navigation", "relation" or "chronology"? Just three things that just came to my mind while reading.

No, you can rename your sandbox project by clicking edit on the page. I like the suggestions btw. - they're getting closer, but I can't decide for one.

Suggestion for the module name:

Node Type Pager

Any good?

@bailey86 : this module doesn't page content. The word 'page', or any derivative of the word 'page' should not be in the name of the module --it creates confusion. A little bit of confusion can be okay if it is in the favor of a play on words. For instance, take the proposed name "chronological node type navigation" and turn it into "chronode navigation".

This is not a technical name. It could be confusing until the project description is read. However, there are several good modules out in contrib that have a name like this: Contemplate, Loggin Tobagan, Shurly, etc.

The reason a name like Chronode Navigation is acceptable [to me] is that it doesn't mislead a user. The name 'Node Type Pager', may mislead a user as this module doesn't paginate content.

Within Drupal, the term 'pager' is used to refer to the (usually numerical) block at the top or bottom of a list, which is used to navigate between pages of that list. Within this context, the 'next' and 'previous' buttons allow you to navigate through a listing of nodes belonging to a specific content type ... the only difference being that your 'list' only displays one item per page. Based on this interpretation, I don't see any issues with using the word 'Pager' in the module title.

Manual Review:
1. Your project page description is a bit short, and could use more text (and grammatical cleanup) in describing what the project does. Posting the contents of your README.txt file would probably be sufficient.

2. package = Volcano: In general, this field should only be used by large multi-module packages, or by modules meant to extend these packages, such as CCK, Views, E-Commerce, Organic Groups and the like. All other modules should leave this blank.

3. pager_for_content_type_help(): The help text could use a bit of grammatical cleanup:

$output .= '<h3>' . t('About Pager for Content Type') . '</h3>';
$output .= '<p>' . t('Adds "previous" and "next" navigation buttons to each node of a particular content type.') . '</p>';
$output .= '<h3>' . t('Usage') . '</h3>';
$output .= '<dl>';
$output .= '<dd>' . t('This module provides a custom pager which allows the user to navigate between nodes of a particular content type. ');

4. $pager_for_content_type is not declared in the function, which makes the last part of this line of code redundant:

$output .= '<dd>' . t('Module provide custom pager for content type. Add @code to end node template.', array('@code' => '<?php print (isset($pager_for_content_type)) ? $pager_for_content_type : ""; ?>')) . '</dd>';

5. Rename your access permission ('cck' is a Drupal 6 term, but it doesn't have any meaning in Drupal 7.). Also, the permission text could use a bit of language cleanup:

  'title' => t('Access "Pager for content type" navigation'),
  'description' => t('Access the "next" and "previous" navigation buttons on nodes.'),

6. * Implements of hook_preprocess_page().
Actually ... you're implementing hook_preprocess_node(). And the text of your hook declarations should read 'Implements hook_...', not 'Implements of hook_...'.

7. Proper use of Render API
Because this is a 'preprocess' function, I believe it's preferred to append the unthemed pager to the bottom of the node render array, rather than directly to the ['#markup'] component ... this ensures that other functions can easily manipulate it if needed.

8. pager_for_content_type_get_pager($node_type, $cnid)
This query should take into account whether i) the nodes have been published, and ii) the user has access to view the node ...

9. Remove function pager_for_content_type_perm()
You've already declared your permissions ... no need to include the D6 version as well. ;)

10. Remove 'Volcano' references in menu and paths.

$items['admin/config/volcano'] = array(
  'title' => 'Volcano modules',

11. Remove $quanntity = $pager['count'];
The $quanntity (quantity) variable is never referenced.

12. function theme_pager_for_content_type($pager):
Remove extra whitespace (double blank lines).

13. $li_previous = ($pager_prev) ? l(t('NEWER POSTS'):
As a suggestion for improvement, I'd like to see the text used for 'NEWER POSTS' in this line configurable (i.e. a setting provided through the settings page).

added branch and fixed all issues

You have some code style issues http://ventral.org/pareview/httpgitdrupalorgsandboxsvipsa1331038git

I would add the config page under "User interface" perhaps, nobody likes a crowded configuration menu.
Use hook_uninstall() the delete your variables after somebody uninstalls your module.

Besides that this module works for me.

The comment about hook_uninstall() in http://drupal.org/node/1331090#comment-5835944 is a requirement, so I'm marking this "needs work". I have not done additional code review but will do it once that is addressed.

However, there is still this unresolved issue. As frob pointed out in http://drupal.org/node/1331090#comment-5280086 there are many modules that do this kind of pagination:
http://drupal.org/project/flippy
http://drupal.org/project/custom_pagers

I think there is room for multiple options but it is part of code review to check for this and your module is an edge case because it is hard to say why this is better right away. Please provide a description of not just what your module does but what cases you would use it for that the others can't do.

Also, please review your automated code review again. I see above that you fixed some issues, but either they're back or there are new errors to address:
http://ventral.org/pareview/httpgitdrupalorgsandboxsvipsa1331038git-7x-1x

Hello, guys! This pager module is obviously very useful for those who want to create their own blog, like me. I will follow your instruction and install this application. I want my blog to be properly organized and interesting for the users. By the way, it is devoted to best internet radio

Actually, it does have a few errors - but it looks like this is because there is a new capitalization test for comments in there. Since these are new I don't believe they should be blockers.

I'm still confused about exactly when I would use this module vs. some of the other options. But variety isn't necessarily a bad thing, and although there's a guideline to look for duplication, there's no hard rule that modules can't be similar to others. In my opinion, this module is different enough that some people might prefer it, and that's value enough to justify releasing it to the community.

Other than that, code review looks good pending any Git Admin final review related to the above comments.

Sorry, this need work.

If a list nodes on a standard Drupal install I get new and old links for every teaser on page a like /node?page=1. I think this seam to be the wrong behaviour, or?

On the configuration page it says: 'Users can manage this content types', I do not really understand what you mean with that. Manage what?

Also, there a lot of pages modules around, you should tell how yours module differ and if there a similarities.

No big issues I think, but I think we need to sort this out to have it back to RTBC.

I think we need a better answer on what is the difference between your module and existing ones, if it just simpler - why not join forces with some of the existing ones to make it more simple? We try not to have new modules that duplicates existing ones.

If the only reason to do this module is that a similar have a name that is difficult to find, I see no reason for this module.

Given that the two modules identified are in 'maintenance fixes only' mode, and the applicant has demonstrated interest in continued development and introduction of differentiating features, I'm willing to overlook the duplication factor in this case.

@jthorson, now that @svipsa added more functionality to the module, I do not think this is a issue any more - but I would rather seen @svipsa taking over as maintainer of one of the existing ones. But I leave as RTBC.

svipsa: What I meant is that eaton's modules (fippy and custom_pagers) were both in 'maintenance mode'; not yours. :)

I agree with MiSc's opinion that it would be better to merge your project with the existing solutions, and look at taking over or applying as a co-maintainer there ... it is always better to collaborate and reduce the amount of module duplication rather than contribute to the problem. That said, though I feel we need to stress the importance of avoiding duplication, I prefer not to block an otherwise acceptable application on this factor alone.

Well then

Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.