[1.2.1] Taxonomy Overview

Thank you for applying!

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.

The important notes are the following.

• If you have not done it yet, enable GitLab CI for the project, and fix what reported from the phpcs job. This help to fix most of what reviewers would report.
• For the time this application is open, only your commits are allowed. No other people, including other maintainers/co-maintainers can make commits.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application.
• Nobody else will get the permission to opt projects into security advisory policy. If there are other maintainers/co-maintainers who will to get that permission, they need to apply with a different module.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the reviewers

Please read How to review security advisory coverage applications → , Application workflow → , What to cover in an application review → , and Tools to use for reviews → .

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• Reviewers should show the output of a CLI tool → only once per application. The configuration used for these tools needs to be the same configuration used by GitLab CI, stored in the GitLab Templates repository.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues → .

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that does not follow the coding standards, contains possible security issue, or does not correctly use the Drupal API
• The single review points are not ordered, not even by importance

src/Controller/TagsOverviewController.php

Since that class does not use methods from the parent class, it does not need to use ControllerBase as parent class. Controllers do not need to have a parent class; as long as they implement \Drupal\Core\DependencyInjection\ContainerInjectionInterface, they are fine.

The documentation comments for class, properties, and methods are missing or incomplete.

    $route_match = \Drupal::routeMatch();
    $this->vocabulary_id = $route_match->getParameter('taxonomy_vocabulary');
    $this->taxonomy_term = $route_match->getParameter('taxonomy_term');

Dependencies must be injected using the dependency-injection container.

          $edit_links['edit_' . $langcode] = [
            'title' => $this->t('Edit ' . strtoupper($langcode)),
            'url' => Url::fromRoute('entity.taxonomy_term.edit_form', [
              'taxonomy_term' => $term->id(),
              'langcode' => $langcode,
            ]),
          ];

The first argument passed to $this->t() or t() must be a literal string, not what returned from a function/method, nor a concatenation of a literal string with the value returned by a function/method.

src/Form/TagsOverviewForm.php

The class does not use any method from the parent class. A form does not need to have FormBase as parent class: as long as it implements the required interface, it is fine.

  /**
   * {@inheritdoc}
   */
  public function submitForm(array &$form, FormStateInterface $form_state) {
    // Handle form submission.
  }

Without code for the submission handler, a form does not do anything, which means that either the code is not complete, or that form class is not necessary.

taxonomy_overview.info.yml

dependencies:
  - taxonomy:taxonomy
  - paragraphs:paragraphs
  - node:node

Two of those dependencies do not follow the format <project name>:<module name>.

1. 1.2.1, 1.2.0, 1.1.2, 1.1.1, 1.1.0, and 1.0.0 are wrong names for a branch and should be removed. Release branch names always end with the literal .x as described in Release branches → .

2. FILE: README.txt

Remove README.txt since README.md is present.

3. FILE: README.md

The README file is missing the required sections → - Installation and Configuration.

4. FILE: TagsOverviewTermMergeForm.php

  /**
   * Constructs the form.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager.
   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
   *   The route match service.
   * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
   *   The request.
   */
  public function __construct(EntityTypeManagerInterface $entity_type_manager, RouteMatchInterface $route_match, RequestStack $request_stack) {

The documentation comment for constructors is not mandatory anymore, If it is given, the description must be “Constructs a new [class name] object”, where [class name] includes the class namespace.

Hello everyone, thank you for taking the time to validate the module!

Below is information on what was fixed

• Removed forms that didn't implement the submit method.
• Controllers that didn't use parent methods were modified to implement ContainerInjectionInterface.
• PHPCS fixes were applied to the code.
• GITLAB-CI added and validating correctly.
• Adjusted constructor comments.
• Updated README with installation and configuration information.

Also, check the branching pattern. I created the 8.x-1.x branch.

I've released the 8.x-1.0 tag with the latest updates for this issue.
Once everything is verified, I'll remove them and release the final release.

I think the Release branches → page could be a bit misleading.

Release branches for modern Drupal projects follow the format {major}.x or {major}.{minor}.x. Drupal 8 and earlier used {API compatibility}-{major}.x.

I encourage you to use a modern format instead of the old 8.x- branch naming.

Hi @bbu23

So you're telling me I should create a 9.1.x branch and within it, increment tags, for example, 9.1.0, 9.1.1, 9.1.2...

Is that it?

hi @cristianodemari,

No, I am talking about these branch names:

Take a look at Search API solr module how they transitioned 8.x-3.x to 4.x. Another example is Admin Toolbar. Or let's take Drupal Core which uses both types.

Now it's up to you from the two types of branch naming which one is suited for you, and hopefully follow the Semantic versioning for release tags, but you can either have something like:

2.x - which should cover both minor and patch releases.
or dedicated branch per minor releases:
2.0.x
2.1.x
etc.

And once you do have a final default branch, don't forget to update Gitlab. Currently your default branch is 1.0.0.

Thanks for the help @bbu23

I created the 2.0.x branch and updated it with the adjustments from this ticket. I also put it as the default branch in gitlab.

Thanks

You're welcome, @cristianodemari!
The branch name looks great!

Below you have my feedback from a partial review. I won't manage to get a full one now:

- There is no dev release. While not mandatory, it's a highly encouraged practice in the Drupal community.

- composer.json should be improved. See Add a composer.json → file for more info.
- Also, your module seems to depend on a contrib module, which is not required in composer. Either make the module optional, or ensure it's required in composer.

- taxonomy_overview.info.yml: The feedback from comment #3 📌 [1.2.1] Taxonomy Overview Needs review was resolved, but one of the modules now has the incorrect format. Dependencies are prefixed with their project name. The paragraphs' project name is not drupal.

- taxonomy_overview.routing.yml: The permission access content is too open for some of these routes. I don't think this is as intended. This permission is often given to anonymous users as well.

- There are untranslated strings here and there. Examples:
-- src/Controller/TagsOverviewTermGroupController.php:

line 99:

$merge_link = Link::fromTextAndUrl('Merge', $merge_url)->toString();

line 111:

   return [
      '#type' => 'table',
      '#header' => [
        'Grouped Similar Terms',
        'Operations',
      ],
      '#rows' => $rows,
      '#title' => 'Grouped Similar Taxonomy Terms',
    ];

- src/Form/TagsOverviewTermMergeForm.php: Related to untranslated strings, but also potential wrong usage of $context['results'] + unsanitized user input passed to it. See Creating a batch in form.inc for more info.

$context['results'][] = "Deleted term \"$label\" (tid: $tid)";

...

Thank you, @bbu23. I appreciate your time.

The mentioned adjustments have been fixed.

awaiting new review

Sorry, I see no new commits in the default branch.

And I suggest deleting the tag 2.0.0 if it was not released yet, and create it at the right time from branch 2.0.x as the first stable release from version 2.
Bring all new commits since last public tag to the default branch.

Also, you should delete all the other unused branches that have their name look like a tag.

Hi @bbu23

Sorry buddy, I had committed the adjustments to branch 1.2.1.

I've already updated 2.0.x with the latest commits and also deleted the 2.0.0 tag.
I removed the other branches that looked like tags.
Can you check again?

Let me ask you a question. What else is needed to obtain security advisory approval?

@cristianodemari: Let me ask you a question. What else is needed to obtain security advisory approval?

Everything is described in comment #2 📌 [1.2.1] Taxonomy Overview Needs review and its linked pages.