[D8] Next Previous Post Block

I updated the Git instructions for non-maintainer users, and added a link to the PAReview checklist.

Automated Review

1. The nextpre.module does not implement hook_help(). See https://www.drupal.org/docs/develop/documenting-your-project/module-docu..... Implementing hook_help() will also mean there will be content for your nextpre.module file, and you won't have to deal with the DrupalSecure warning about it being empty.
2. WARNING: nextpre/src/Plugin/Block/NextPreviousBlock.php line 86: "NodeType::loadMultiple calls should be avoided in classes, use dependency injection instead". This can be fixed by changing the line to $node_types = $this->nodeTypeStorage->loadMultiple();

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Duplication/Fragmentation

This module appears to duplicate some of the features provided by the Flippy module. Could you add a "Similar projects and how they are different" section to your Project page explaining how your project differs? Please see duplication and/or fragmentation for more information about this issue.

README.md

1. You can change the INSTALLATION section to something like
   

   Install as you would normally install a contributed Drupal module. See: https://www.drupal.org/node/895232 for further information.

2. The content under INTRODUCTION belongs in CONFIGURATION.
3. Also, you may want to change item 3, '...click on "Place order" and enable', so that it uses the exact phrases the user will encounter. For example,
   

   ...click the "Place block" button and in the modal dialog click the '"Place block" button next to "Next Previous Block"'

4. The way the word "View" is used in the module name and README.md makes it seem that the module creates a view, and not a block.

Project page

Please take a moment to make your project page follow tips for a great project page.

For example:

1. Along with the "Similar projects and how they are different" section mentioned above, a "Features" section could mention that you can change the text in the Previous and Next links.
2. You should also mention that the user needs to place the block in a region in order for the previous/next links to appear on the page.
3. The way the word "View" is used in the module name makes it seem that the module creates a view, and not a block.

Code long/complex enough for review

Yes, the code meets guidelines for project length and complexity.

Coding style & Drupal API usage

1. (*) Bad declaration: nextpre/src/Plugin/Block/NextPreviousBlock.php line 14:
   use Drupal\Core\node\Entity\NodeType as NodeType; should be changed to: use Drupal\node\Entity\NodeType as NodeType;. However, if you fix the Automated Review warning on line 86, mentioned above, you can remove this line entirely.
2. (*) Please review the API documentation and comment standards. For example, in nextpre/src/Plugin/Block/NextPreviousBlock.php, generatePrevious(), generateNext(), and generateNextPrevious() need @param and @return DocBlocks.
3. (*) The nbproject folder and .gitignore file are being included in releases. Use .gitattributes to prevent this. Please see Creating a Project Release for more information.
4. You can remove the nbproject folder from the git index by using the command git rm -r --cached nbproject and it will be removed from the repository after a commit and push. Note: if you don't include the "--cached" argument, the folder will also be deleted from your local directory!
5. Since you are only using .gitignore for the nbproject folder, you don't really need it in your repository. You can actually add ".gitignore" to the .gitignore file. You can then do a git rm --cached .gitignore to remove it from the index.

The starred items (*) are fairly big issues and warrant going back to Needs Work. The rest of the comments in the code walkthrough are recommendations.

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

This review uses the Project Application Review Template.

Remember to change the status to Needs Review after you are done addressing the issues mentioned above.

Automated Review

• Your README.md does not follow best practices (headings need to be uppercase). See https://www.drupal.org/node/2181737
  • The INTRODUCTION section is missing.

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Coding style & Drupal API usage

1. (*) Please review the API documentation and comment standards. For example, in nextpre/src/Plugin/Block/NextPreviousBlock.php, generatePrevious(), generateNext(), and generateNextPrevious() need @param and @return DocBlocks.

The starred items (*) are fairly big issues and warrant going back to Needs Work.

I can understand how you would think that the API documentation and comment standards doesn't apply to your project. However, even though the page looks like it's just for API documentation, the standards apply to all project code.

The missing @param and @return that I mention are just examples. You should read the entire page, and go through your code and make any necessary changes. Look at a few other projects, especially well established ones, to see how they apply the standards.

You should try to put yourself in the position of someone with some knowledge of how Drupal works, but who is looking at your code for the first time. For example, imagine looking at this for the first time:

/**
   * Lookup the next or previous node.
   */
  private function generateNextPrevious($created_time, $direction = 'next')

• What is $created_time? Is it a date object, a string, an integer of a Unix timestamp...?
• The default value for $direction is the string 'next', but what other values does it take?
• Does the method return anything? If it does, what is the variable type? Can it return different variable types depending on the circumstances? The current DocBlock implies that it returns a node.
• What happens, or is returned, if there isn't a next or previous node?

Again, this is just one example. You should read the entire page, go through your code and make any necessary changes.

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

This review uses the Project Application Review Template.

After making changes, go to the PAReview checklist and Repeat review to make sure they didn't introduce any new issues.

By the way, you may find these projects helpful for developing Drupal projects. They will let you run the automation tests without having to commit your code to the repository:

• PAReview.sh
• Coder

Also, you may want to start following the best practices outlined in Commit messages - providing history and credit. Particularly the part about referencing issues in commit messages, and giving credit to other contributors who submitted patches.

The priority doesn't change, since 3 weeks have not passed from the last review.

Automated Review

No issues found

Manual Review

No duplication

May Cause module duplication and/or fragmentation. Someone with more experience/authority needs to determine if this module satisfies this requirement.

Secure code

Yes Meets the security requirements.

Also confirmed by entering <script>alert('test');</script> in all form fields created by the module.

Coding style & Drupal API usage

1. (+) You allow users to submit your block's form without entering any values, other than the title.
   • It's odd to have a radio button group that is not required to have a value selected, especially when it's not possible to go back to a "valueless" state once a radio button is selected.
   • If no values are entered for the "Previous Text" or "Next Text" fields, then the respective link will not be added to the page. Is this by design?
2. implementation of build() in NextPreviousBlock.php returns a non-empty array when there is no next and no previous link. This means the block is still added to the page even if there aren't any links to show. If this is not by design, then please look at the DocBlock of the build() method you are inheriting from to see what should be returned if the block should not be added to the page.
3. (*) The DocBlock for generatePrevious() says it returns "A render array...", as does generateNext(). However, the DocBlock for generateNextPrevious() says it returns "Find the alias of the next node." Also, it's possible for those methods to return an empty array.
4. Most of your @param descriptions begin with the word "Get", which isn't correct usage in this context. Please look at the DocBlocks of some core and major contrib modules to see how they write their @param descriptions.
5. (+) If node revisions is turned on, then your module may not behave as people expect it to. Your module determines what the order of the nodes are by the revision id of the node. The more recent a revision, the later it appears in your pager. Suppose you have 100 blog entries on your site, and the first one was written over a year ago. If you correct a minor grammatical error on it today, it will then become the last blog post in your pager. You should also check what happens if someone reverts to an earlier revision of a node. Does your pager link to the highest revision or the reverted one for that node (I wasn't able to test this)?

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.

what for the call parent method in 115, 120, 83 lines?
https://git.drupalcode.org/project/nextpre/blob/8.x-1.x/src/Plugin/Block...
https://git.drupalcode.org/project/nextpre/blob/8.x-1.x/src/Plugin/Block...
https://git.drupalcode.org/project/nextpre/blob/8.x-1.x/src/Plugin/Block...

why used request_stack service?
If use current_route_match, build method change to:

$node = $this->routeMatch->getParameter('node');
 if ($node instanceof Node) {
      $link['prev'] = $this->generatePrevious($node->id());
      $link['next'] = $this->generateNext($node->id());
    }

Not recomende using maxAge=0.
More info
More
https://git.drupalcode.org/project/nextpre/blob/8.x-1.x/src/Plugin/Block...

Recomended used cache-tag.
Example, don't checked

 public function getCacheTags() {
    return $current_node->getCacheTagsToInvalidate();
  }

Add default value for $direction param
https://git.drupalcode.org/project/nextpre/blob/8.x-1.x/src/Plugin/Block...
Method changed to:

private function generateNextPrevious($current_nid, $direction = 'next') {
    $comparison_opperator = '>';
    $sort = 'ASC';
    $display_text = $this->configuration['next_text'];
    $class = "nextpre_next";

    if ($direction === 'prev') {
      $comparison_opperator = '<';
      $sort = 'DESC';
      $display_text = $this->configuration['previous_text'];
      $class = "nextpre_previous";
    }

Thank you for create this module.

Check node type before query in build method
https://git.drupalcode.org/project/nextpre/blob/8.x-1.x/src/Plugin/Block...

if ($node instanceof Node && $node->bundle() == $this->configuration['content_type'])

https://git.drupalcode.org/project/nextpre/commit/4775224831978ab0872b25...

$node really an Node entity?

For check use if ($node instanceof Node) {

Where returns default value?
https://git.drupalcode.org/project/nextpre/blob/8.x-1.x/src/Plugin/Block...

Nitpick
I'm not sure if ParamConverterInterface uses in request.
I think instead request.

$node = $this->request->attributes->get('node');

use routeMatch

$node = $this->routeMatch->getParameter('node');

Hello rohit-drupal,

nextpre.module

The help is not translatable. Not everyone is speaking english. You should give the ability to anyone to contribute to your module translations using the standard system. My advice would be to put a translatable short description of the module and usage, and to include a link to any other documentation page.

src\Plugin\Block\NextPreviousBlock.php

build()

$node = $this->request->attributes->get('node');

You need to allow only a set of routes before calling this and going further. Only allow entity.node.canonical, entity.node.revision and any other suitable routes for current route.

Currently, it is not possible to use $node = $this->routeMatch->getParameter('node'); as suggested earlier. Indeed, this is not yet working on other routes than entity.node.canonical, and this is tracked by this issue: #2730631: Upcast node and node_revision parameters of node revision routes ➡️ the returned value is inconsistent. So no need to change the $node = $this->request->attributes->get('node'); line.

getCacheMaxAge()

Why override this? Is it intended to make this block uncacheable by setting the value to 0? This seems not efficient. You should use cache tags: https://www.drupal.org/docs/8/api/cache-api/cache-tags.

Specifically, a simple cache tag for your block would be node_list:

node_list — list cache tag for Node entities (invalidated whenever any Node entity is updated, deleted or created, i.e. when a listing of nodes may need to change).

A better solution, which could come as an enhancement, would be to implement a little system which tags using node:id (e.g. node:15, node:214) when there are previous and next nodes (you already have their ids in the process of building the block, so it's just a few lines to refactor), and node_list when the next node is not yet created.

'next' and 'prev'

I would suggest using PHP const instead of string literals. This makes code more maintainable. Something like:

const DIRECTION__PREVIOUS = 'prev';
const DIRECTION__NEXT = 'next';

private function generateNextPrevious($current_nid, $direction = self::DIRECTION__NEXT) {

Other considerations

• You may state on the module page this can be achieved using Views, and highlight the simplicity of your module.
• You may state on the module page the rule of ordering. This module orders by ID and should make this point clear.

Thank you for your contribution! I am going to update the project to opt it into security coverage.

These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.