[1.0.x] MD WordsCloud

Thank you for applying! Reviewers will review the project files, describing what needs to be changed.

Please read Review process for security advisory coverage: What to expect for more details and Security advisory coverage application checklist to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.

To reviewers: Please read How to review security advisory coverage applications, What to cover in an application review, and Drupal.org security advisory coverage application workflow.

While this application is open, only the user who opened the application can make commits to the project used for the application.

Reviewers only describe what needs to be changed; they don't provide patches to fix what reported in a review.

Hey Rajan,

I see that there is a D7 module available, so it might be better to merge your project into the existing D7 one:
https://www.drupal.org/project/md_wordcloud →

I've found following modules that might be similar, maybe it's good to describe why yours differs from these similar ones:

• https://www.drupal.org/project/tagclouds →
• https://www.drupal.org/project/dynamictagclouds →

I see the following issues with your module:

PART 1: Manual review

1. It seems you've added files to the module that aren't actually yours to add. It seems like d3.min.js and the d3.layout.cloud.js come from another library. See licensing part of this page for more information: https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... → .
I suggest you add these as external dependencies or help with the porting of this module to D9/10: https://www.drupal.org/project/d3 → and then having a dependency on it.
See this module for examples: https://git.drupalcode.org/project/slick/-/blob/8.x-2.x/slick.libraries.yml

2. Remarks on md_wordcloud.js:

• It's a good practice to pass your dependencies as parameters. You did it for Jquery but you are also using drupalSettings. See https://www.drupal.org/docs/drupal-apis/javascript-api/javascript-api-ov... →
• Not sure if it is required but you've also seem to have forgotten to pass context and settings to the attached function as per https://www.drupal.org/docs/drupal-apis/javascript-api/javascript-api-ov... →
• The codingstandards for javascript files needs some work: https://www.drupal.org/docs/develop/standards/javascript/javascript-codi... → . In particular: indentations, non-strict comparisons,

3. Readme.md is not following recommendation: see template https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... →

4. Remarks on src/Plugin/Block/MdWordscloud.php:

• Don't use prefix and suffix to wrap 2 fields in a container. Ideally use a container element to wrap these as per https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...
• Only declare/set the $build variable once. It looks like the one on line 206 can be removed.
• I see no reason why you wouldn't allow this block to be cached. I recommend removing the max-age 0 and replacing it with proper cache tags. As far as i see you don't need any cache context. As you are getting the data from a straight from a database query you don't have easy access to the tags of the terms themselves. Either rework the service to use the entity api instead, if you can do it in a performant way, you might run into issues if you try to process 100+ entities at once. You need to have at least the taxonomy_term_list:[whatever voc is being used for this instance] so newly created or deleted items invalidate the cache and have the individual cache tags for terms in case they are updated. For more info look here: https://www.drupal.org/docs/drupal-apis/cache-api/cache-tags →

5. Not 100% sure about this one, but I think you can create your theme hook without passing variables. (as you aren't passing any data anyway)

6. Remarks on /src/WordcloudsContent.php:

• This might not be an issue per se but, it looks like the functionality doesn't take into account translations. As i see it currently it will just take terms from all languages and not only the current active one.
• getmdcloudTags could be a private function as you are only calling it from within the service.
• Why are you making a join to the node_field_data table? I don't see what functionality it brings. If it isn't needed I would remove it as it might significantly slow down your query.
• Currently no access checks are being performed on the terms that you are showing. This could lead to possibly unwanted leak of data if combined with other modules that give more fine grained access control over taxonomy terms.Like https://www.drupal.org/project/taxonomy_access_fix →

PART 2: Automated review
Too much feedback was returned from the parereview.sh script. https://www.drupal.org/project/pareviewsh →

A brief summary is:

• Readme.md does not follow best practices. Also mentioned in the manual review
• Missing hook_help
• Bad line endings
• The js files from D3 also fail because they shouldn't be in the project.

I have added How they are different? section on project page.

PART 1: Manual review

1) Third party library code added in libraries.yml and updated details in project page as per your suggestion.
2) Remarks on md_wordcloud.js == JS format fixed
3) Readme.md is not following recommendation: see template == fixed
4) Remarks on src/Plugin/Block/MdWordscloud.php == added cache mechanism and fix formatting issue.
5) hook_theme() variable issue fixed
6) Remarks on /src/WordcloudsContent.php:

Translation added, now only current language term will display.

getmdcloudTags is now a private function

Why are you making a join to the node_field_data table? == I have check the current language of node that's why make join with this table

PART 2: Automated review

Readme.md does not follow best practices. Also mentioned in the manual review
Missing hook_help
Bad line endings
The js files from D3 also fail because they shouldn't be in the project.

I have fixed above automated review issues and committed code in latest branch.

1. Fix phpcs issue.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml md_wordscloud/

FILE: md_wordscloud/README.md
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 13 | WARNING | Line exceeds 80 characters; contains 183 characters
----------------------------------------------------------------------


FILE: md_wordscloud/src/WordcloudsContent.php
-----------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-----------------------------------------------------------------------------
  91 | WARNING | Line exceeds 80 characters; contains 109 characters
 129 | WARNING | Line exceeds 80 characters; contains 106 characters
-----------------------------------------------------------------------------

2. FILE: src/WordcloudsContent.php

  /**
   * {@inheritdoc}
   */
  private function getmdcloudTags(array $vids, $size) {

{@inheritdoc} is used if you are overriding or implementing a method from a base class or interface.

// \Drupal::logger('data_from_cache')->warning('<pre><code>' . print_r($tags, TRUE) . '

');

// \Drupal::logger('data_from_db')->warning('<pre><code>' . print_r($tags, TRUE) . '

');

Remove commented code (line 91 and 129).

Fixed #9 in new release.

@Rajan Kumar There is no need to create a new release for every review. You can just push the changes to the branch.

The rest looks fine to me.

Let’s wait for other reviewers to take a look, and if everything goes fine, you will get the role.

@vishal.kadam got it, thank you for your review.

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 Slack → #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 reviewers.

Automatically closed - issue fixed for 2 weeks with no activity.