🇧🇪Belgium @nielsaers

nielsaers → created an issue.

BramDriesen → credited nielsaers → .

nielsaers → created an issue.

@john-pitcairn if I follow your logic the same could be said for webform. A user creates them so it should be a content entity but in webform it's case it is not. I bet there are many more gray area cases like that in the Drupal eco system of modules.

At least when it is a config entity you as a site builder can choose to consider it either more to the side of config or more to the side content by leveraging the config ignore module. Now that it is a content entity your only option is to leverage content sync modules to get it synced across environments.

You can get the patch easily from the merge request. By going to the following url: https://git.drupalcode.org/project/computed_breadcrumbs/-/merge_requests...

You should ask the maintainer if they want to merge this. I'm not planning on asking maintainership of this module.

Luke.Leber → credited nielsaers → .

Looks good.

nielsaers → made their first commit to this issue’s fork.

Additional review done:
https://www.drupal.org/project/projectapplications/issues/3357132#commen... →

Hey Velocity29,

Not a requirement, but just trying to help. We can't all be native speakers right. I've a proposal for a rewritten description on the module page:

Overview

The bulk batch delete is a module which helps you to delete users, content or taxonomies in bulk. It comes with very simple form where you can select which bundle you want to delete the content from. You can also add a custom description so the log reflects the action taken.

After submitting the form the chosen entities will be deleted.

Features

Quantity – Option to select number of entities to delete.
Tracking & No log burden on DB – Logs will get written into a file instead of DB.

I'd also mention the functionality that is available in the drush command.

For the review:
Similar modules:
Similar functionality could be achieved by using Views Bulk operations. Maybe it's interesting to let possible users know what's the advantage of using your module instead of building it using Views Bulk operations.

It might also be interesting to see if you can help this module with a Drupal 9/10 version instead of starting your own project: https://www.drupal.org/project/bulkdelete →

Maybe also see what is different enough about your module as opposed to this one: https://www.drupal.org/project/delete_all → (it looks like this module does a lot similar and has a lot of sites using it as well)

Manual review

1. Likely security issue with the permission on the route. Currently "access content" is used to allow access to the deletion form. This permission is easily given to anonymous users (see standard install profile https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/profiles/st...). So that means this would allow an anonymous user to delete all content on your site.

2. Your composer file might have some issues:

• I think your package name should be: drupal/bulkbatchdelete
• It's now defined as drupal-drush but it's mainly described as a Drupal module
• You must enter license as GPL-2.0-or-later when hosting on Drupal.org

See: https://www.drupal.org/docs/develop/creating-modules/add-a-composerjson-... →

3. Not a requirement, but it would make more sense to have a link to this form in the content section instead of the /admin/config/system.

4. Very debatable, but having the log files stores in a public directory, might lead to a security issue. A malicious actor having access to these logs could help with enumeration of user ids and could help in subsequent attacks. But as i said very debatable. My suggestion would be to have the option to: use the default logging of Drupal (which can go to db or syslog) or have the choice of where to store these files. (private or public)

5. Readme file is not following standards. See: https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... →

6. Feedback on: BulkUserDeleteCommands.php

• Seems to have a lot of comments that come from boilerplate generators, this adds very little value to the developer reading the code as it's not really relevant for the functionality
• It seems very arbitrary to only delete active users from a certain role. I'd expect all user to be deleted. Or at least be given the option to choose by way of a parameter.

7. Feedback on: BatchCreationForm.php

• Seems to have some comments that come from boilerplate generators, this adds very little value to the developer reading the code as it's not really relevant for the functionality
• I also seem some comments that don't seem to be adapted from the original purpose of this module. It looks like this was originally only to delete users in bulk not multiple entity types.
• Ideally someone can help you fix the grammar on the descriptions. Not a requirement, but a nice to have.
• It's an interesting approach to creating the options for the select list with ajax. I think you might run into possible security issue as Drupal can't validate if those values are within the range it can expect. For a dynamic select list with ajax I'd like to point you to: https://git.drupalcode.org/project/examples/-/blob/4.0.x/modules/ajax_ex...
• You've added a lot of custom validation which could be handled with the #required parameter on the form elements. You might want to have a look here on how to bypass the required issue when using ajax: https://www.drupal.org/project/drupal/issues/2476569 →

8. Feedback on: /src/Service/ProcessEntity.php

• A nice to have would again be a grammar thing here: use entity instead of entitie
• The methods to get the options for the select lists don't allow the labels to be translated. e.g. Blocked should be wrapped in a translation function: https://www.drupal.org/docs/8/api/translation-api/overview →
• I think you could also get the options for getListOfCancelMethod from the original list in core instead of hardcoding it here. If core changes something your code will still be in sync. See user_cancel_methods() in user.module

9. Feedback on: src/Service/BatchService.php

• It's a bit confusing that the variable node_type_list is being used for bundles and roles at the same time.
• Not sure why the debug tag remains on the query. I would remove it.
• A nice to have: Dependency injection in the batchoperation as per the example here: https://www.previousnext.com.au/blog/refactoring-drupal-batch-api-callba... as we are blocked doing it the normal way here: https://www.drupal.org/project/drupal/issues/2875151 →

Skipping the automated review as Vishal already mentioned them above.

Additional review done: https://www.drupal.org/project/projectapplications/issues/3356949#commen... →

Hey akalam,

1. date_all_day.js:
I think you might be attaching multiple change events to the same elements. Every time behaviors get triggered you'll add your change listener to existing and new items alike. If it is the case, use once and the context to apply your logic: https://www.drupal.org/docs/drupal-apis/javascript-api/javascript-api-ov... →

2. date_all_day.libraries.yml:
You define once as a dependency but you don't use it. See remark in 1.

The rest looks good! (Automated check didn't have any complaints other than the proposal to write some tests, but these aren't mandatory for this)

Picking this one up as well. (ideally today)

Picking this one up today.

Hey apaderno, is this what you were looking for? https://www.drupal.org/project/projectapplications/issues/3356457#commen... →

As you are checking these on the regular, is there a possibility you could assign a requested review to me? That way I don't have to check every day if there are any more.

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.

Setting to needs work again so it doesn't get picked up by anyone.

Sounds like a plan.

nielsaers → created an issue.

nielsaers → created an issue.

Ok, so it's a numbers game. Amount of commits and not the actually committed code is the rule.

How do I go forward, one of the following option?

1. Close this request and I open a new one for a different module I maintain.
2. Do I create a new branch from and old commit so you can check there? (but feels counterproductive)

Based on what do you make that assertion? I have 16 commits (counting those git user whoopsies) out of 42 commits on this branch. I have the most commits on this project and I've done the initial code for this project. Which should show that I understand about writing secure code that follows Drupal coding standards and correctly uses the Drupal API.

So I respectfully disagree with your assertion. How do we take this forward?

Fixed coding standards in the following commit: https://git.drupalcode.org/project/disable_language/-/commit/b739901a03f...

Manual review

Individual user account
yes
No duplication
yes - Does not cause
Master Branch
yes
Licensing
Yes
3rd party assets/code
Yes
README.txt/README.md
No

• 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.
  • The REQUIREMENTS section is missing.
  • The INSTALLATION section is missing.
  • The CONFIGURATION section is missing.

  Code long/complex enough for review
  Yes
  Secure code
  yes
  Coding style & Drupal API usage
  Resolved after comment 5.

nielsaers → created an issue.

nielsaers → created an issue.

Patch resolves the issue. Haven't seen any side effects.

nielsaers → created an issue.

Patch from 154 works for me on 8.x-1.22

For those creating a patch from https://git.drupalcode.org/issue/drupal-2942975/-/tree/2942975-reroll-9.4.x and using the the patch from https://www.drupal.org/project/drupal/issues/2946333 📌 Allow synced Layout override Translations: translating labels and inline blocks Needs work . There will be a conflict in applying it.

The patch in 2946333 adds the following line to LayoutBuilderEntityViewDisplayResourceTestBase.php:

    $expected['hidden'][OverridesSectionStorage::TRANSLATED_CONFIGURATION_FIELD_NAME] = TRUE;

Below:

    $expected['hidden'][OverridesSectionStorage::FIELD_NAME] = TRUE;