[D8] Nodeinfo

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.

Hi @kirkkala,

I have reviewed your module. Following are the feedback for the same :

Automated Review

• For Drupal 8 :

[No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.]

• For Drupal 7:

Manual Review

Individual user account

• Yes Follows the guidelines for individual user accounts.

Coding style & Drupal API usage

List of identified issues:
• Found issues after testing the module with coder module. Few minor errors :

Recommendations for Application Issue

• Provide Similar Modules and how your module is different from others.
• As a developer I would have preferred creating a view block and place this block in the region as per my current theme. The view block select node nid from the contextual filter and provide the value from url.(contextual filter will be important to get the current nid of the viewed page) refer screenshot. Thus this block would be configurable and could be restricted for users on as per the role. As per the requirement more node information/ submission information could be added/removed to the view. This would be more configurable.

Hi kirkkala,

The module is working fine as mentioned in the description

It would be good if we have the hook_help for the module

https://www.drupal.org/docs/develop/documenting-your-project/module-docu...

Hi kirkkala,

I have review you d7 branch, please find below details :

Please add t() function on line 57 $pub_status = ' <em>unpublished</em> ';

Please add t() function on line 87 $ad .= ' <strong>Assigned to all domains</strong><br />';

Please add t() function on line 113 $ad .= $delimiter . '<em title="' . $domain['sitename'] . '">' . $pt . '</em>';

Hi kirkkala,

In d8 no need to write the hook_permission inside .module file instead of that you can use modulename.permission.yml which you have already implement.

File:

nodeinfo.module

20

/**
 * Implements hook_permissions().
 */
function nodeinfo_permission() {
  return array(
    'view node info' => array(
      'title' => t('View node info'),
      'description' => t('Display node info message when viewing nodes.'),
    ),
  );
}

The above code looks like its not required. It is recommended to remove the above code.

Hi kirkkala,

Here is my review report about 8.x-1.x branch !! The overall code looks good. The only issue I found here is the length of code. While it's not mandatory to have a specific minimum code length. Kindly have a look at "guidelines for project length" page to get a general idea about how much code one should at least put in else it seems good to issue RTBC to this issue.

Thanks !! :)

Automated Review

Ran 8.x branch thru pareview.sh It showed no errors/warning: https://pareview.sh/node/664

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

No: Does not follow the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

Meets coding standards. Ran code thru phpcs. It returned no warnings/errors.

This review uses the Project Application Review Template.

Hi

Please find the comments below

1. In your current, you have used the "hook_preprocess_page" hook as in drupal 8 the event subscriber is recommended.
It is recommended to use the eventsubsriber" for the same.

These are surely not a application blockers, anything else that you found or should this be RTBC instead?

Hi @kirkkala,

The module is working fine for me, Please find my comment/suggestions on over the same.

1. It will be good if you can provide the edit on message.

2. Currently, you are using "drupal_set_message" function. This method have the \Drupal::service('page_cache_kill_switch')->trigger(); code, that does not all me to cache the pages.
So it's a suggestion if your module provides the block integration to display the messages then the cache issues is resolved.

3. Please try to avoid the unwanted hooks "hook_preprocess_page" as they may be deprecated in future drupal versions.

Otherwise, the module is working fine. Good work. :)

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

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

Hello,

After reviewing Drupal 8 version of module I must say that I agree with @kajalkiran, creating a block that can be assigned to any region sound much better then displaying information about node using drupal_set_message(). In this case you could also avoid hooking into KernelEvents. This is only suggestion so decision is up to you.

In order to fix your problem with dependency injection you can pass additional arguments to your service, this is example how inject route_match, modify nodeinfo.services.yml

services:
  nodeinfo:
    class: 'Drupal\nodeinfo\EventSubscriber\NodeInfo'
    arguments: ['@current_route_match']
    tags:
      - {name: event_subscriber}

And then inside NodeInfo class define protected variable $routeMatch and initialise it inside constructor so you class looks something like this

/**
 * Class NodeInfo.
 */
class NodeInfo implements EventSubscriberInterface {

  /**
   * The current route match.
   *
   * @var \Drupal\Core\Routing\RouteMatchInterface
   */
  protected $routeMatch;

  /**
   * Constructs a TwitterEntityManager object.
   *
   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
   *   The current route match.
   */
  public function __construct(RouteMatchInterface $route_match) {
    $this->routeMatch = $route_match;
  }

After you do it you can replace this code
$node = \Drupal::routeMatch()->getParameter('node');
with this one
$node = $this->routeMatch->getParameter('node');

Is it similar to:
https://www.drupal.org/project/about_this_node
https://www.drupal.org/sandbox/narendra23/2702411

Also , please check if this module is duplicate of https://www.drupal.org/project/about_this_node or not.
I have already created a port of drupal 8 and waiting for the maintainer to provide me access of project.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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