[D8] PanKM Chart

Thanks for your contribution!

1. pankm_chart_uninstall(): you are using lot's of deprecated functions like drupal_set_message() and entity_delete_multiple(). Please cleanup your module and only use non-deprecated Drupal APIs.
2. pankm_chart_add_node(): doc block is wrong, this is not a hook. Please check all doc blocks of your functions.
3. pankm_chart.libraries.yml: indentation errors. Please make sure that all lines have the correct number of spaces as a multiple of 2.
4. pankm_chart_preprocess_node(): do not put markup into #prefix and #suffix. Use a twig template instead.
5. the javascript files are not formatted correctly to Drupal coding standards. Can you check with eslint and fix them? See https://www.drupal.org/docs/develop/standards/javascript/eslint-settings
6. pankm_chart_preprocess_node(): no XSS vulnerability there because Drupal 8 auto sanitizes for us, yay!
7. webfonts and JS libraries: you should not include them with the module, there could be licensing issues. Instead please tell users where to download them and where to put them.

Otherwise looks good to me!

Thank you for applying! I added the Git instructions for non-maintainer users and the PAReview checklist link. Reviewers will check the project and post comments to list what should be changed.

If you haven't done it, yet, please check the PAReview report and fix what needs to be fixed. There could be some false positives; verify that what reported is correct, before making any change.

In particular, the following points need to be fixed.

• The repository is using wrong branch names; see Release naming conventions, which explains which are the correct branch and tag names
• The code doesn't follow the Drupal coding standards in many points

8.x-1.1 is a tag name, not a branch name; a branch name would eventually be 8.x-1.x.

Thanks for the contribution.
I am seeing few warnings in the PAReview checklist. You may want to resolve those.
https://pareview.sh/pareview/https-git.drupal.org-sandbox-rahuljohnson-3...

Thank you for the contribution!

About Tests: Yes, you are right.
I have not found any security related issues into the code.

I think wrong git branch names were not application blockers, so this can go back to RTBC straight away.

What we review in these application didn't change.
The purpose of these applications is checking what the users understand about using the Drupal API, how to correctly use them, how to write secure code, and what they understand of what reported in the reviews. It's wrong to directly set an application as reviewed when there are changes to be done in the code. It's also wrong to mark it as reviewed when the OP doesn't seem to understand what reported in a review.

The branch name has been fixed and all other issues as well, all looking good to me!

See comment #4.

Could you repeat again what points from #4 are still left to do? It seems to me they have all been addressed.

There have not been commits, in the last 30 days.

See comment #4.

@kiamlaluno: Could you repeat again what points from #4 are still left to do? It seems to me they have all been addressed.

Please replace the deprecated functions, classes, etc.:

 ------ --------------------------------------------------------------------------
  Line   pankm_chart.install
 ------ --------------------------------------------------------------------------
  145    Call to deprecated function entity_get_form_display():
         as of Drupal 8.0.x, will be removed before Drupal 9.0.0.
         If the entity form display is available in configuration use:
  158    Call to deprecated function entity_get_display():
         as of Drupal 8.0.x, will be removed before Drupal 9.0.0.
         If the display is available in configuration use:
  170    Call to deprecated method entityManager() of class Drupal:
         in Drupal 8.0.0 and will be removed before Drupal 9.0.0.
         Use \Drupal::entityTypeManager() instead in most cases. If the needed
         method is not on \Drupal\Core\Entity\EntityTypeManagerInterface, see the
         deprecated \Drupal\Core\Entity\EntityManager to find the
         correct interface or service.
  202    Call to deprecated function file_prepare_directory():
         in Drupal 8.7.0, will be removed before Drupal 9.0.0.
         Use \Drupal\Core\File\FileSystemInterface::prepareDirectory().
  215    Variable $file might not be defined.
 ------ --------------------------------------------------------------------------

 ------ -------------------------------------
  Line   pankm_chart.module
 ------ -------------------------------------
  71     Variable $nid might not be defined.
  93     Constant fids not found.
  109    Constant value not found.
 ------ -------------------------------------
 [ERROR] Found 8 errors

The indentation is also wrong on the lines initializing a variable. The coding standards don't say they must be all aligned.

1. Please remove the loop with empty body:

2. Please check again the attached drupalSettings in pankm_chart_page_attachments hook. Can we move it into pankm_chart_preprocess_page

The indentation is also wrong on the lines initializing a variable. The coding standards don't say they must be all aligned.

/**
 * Implements hook_preprocess_page().
 */
function pankm_chart_preprocess_page(&$variables) {
  /*For node pages*/
  if (isset($variables['node'])) {
    $node = $variables['node'];
    if ($node->bundle() == 'pankm_chart') {
      global $base_url;
      $type                                                   = $node->field_chart_type->value;
      $csv                                                    = isset($node->field_csv->entity) ? $node->field_csv->entity->getFileUri() : "";
      $module                                                 = drupal_get_path('module', 'pankm_chart');
      $csv_path                                               = file_create_url($csv);
      $variables['#attached']['drupalSettings']['chartType']  = $type;
      $variables['#attached']['drupalSettings']['csvPath']    = $csv_path;
      $variables['#attached']['drupalSettings']['modulePath'] = $base_url . '/' . $module;
      $variables['#attached']['library'][]                    = 'pankm_chart/common-styling';
    }
  }
  $variables['#attached']['library'][] = 'pankm_chart/iframe-styling';
}

See the code used from Drupal core. The equal characters on lines initializing variables aren't aligned, when there are multiple lines initializing a variable.

Thank you for fixing the issues.

Could you please take a look some points below:

1. Is the pankm_chart/iframe-styling need to include for all page?

2. All JavaScript code MUST be declared inside a closure wrapping the whole file, see this link.
I'm not sure this is a JavaScript coding standard issue or Not. Hi @kiamlaluno, Could you please help to take a look to this point.

1. t('Please grant the writing permission to the Directory :directory', [':directory' => $directory]): The ':' placeholder does not exist, please use "@" instead.
2. pankm_chart_preprocess_page(): why do you need this second preprocess function and cannot attach in pankm_chart_preprocess_node()?

Otherwise looks good to me.

@phthlaap anything that still needs to be done here? Please don't change the status without comment.

How about your comments in #41

1. t('Please grant the writing permission to the Directory :directory', [':directory' => $directory]): The ':' placeholder does not exist, please use "@" instead.
2. pankm_chart_preprocess_page(): why do you need this second preprocess function and cannot attach in pankm_chart_preprocess_node()?

No need any action for the 2 points above?

Ah yes, sorry. Those are "nice to have" improvements, but not application blockers as they are not security issues.

Thank you for your contribution! I am going to update your account.

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.