[2.x] Taxonomy Scheduler

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 smother 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 → .

Since the project is being used for this application, for the time this application is open, only the user who created the application can commit code.

Please change the title of issue in this format:
[latest branch] Module name

So that reviewers can check your code.

Thanks

Hello @vishal.kadam,
Reviewer cannot declare the review branch.

Please fix PHPCS issues
phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml web/modules/contrib/taxonomy_scheduler

FILE: /app/web/modules/contrib/taxonomy_scheduler/README.md
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
52 | WARNING | Line exceeds 80 characters; contains 93 characters
59 | WARNING | Line exceeds 80 characters; contains 82 characters
----------------------------------------------------------------------

FILE: ...cheduler/src/EventSubscriber/TaxonomySchedulerCronSubscriber.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
16 | WARNING | The class short comment should describe what the
| | class does and not simply repeat the class name
----------------------------------------------------------------------

FILE: ...duler/src/EventSubscriber/TaxonomySchedulerPresaveSubscriber.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
16 | WARNING | The class short comment should describe what the
| | class does and not simply repeat the class name
----------------------------------------------------------------------

FILE: .../taxonomy_scheduler/src/Exception/TaxonomySchedulerException.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
7 | WARNING | The class short comment should describe what the class
| | does and not simply repeat the class name
----------------------------------------------------------------------

FILE: ...pp/web/modules/contrib/taxonomy_scheduler/src/Form/AdminForm.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
17 | WARNING | The class short comment should describe what the
| | class does and not simply repeat the class name
----------------------------------------------------------------------

FILE: ...taxonomy_scheduler/src/Service/TaxonomySchedulerFieldManager.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
16 | WARNING | The class short comment should describe what the
| | class does and not simply repeat the class name
----------------------------------------------------------------------

FILE: .../taxonomy_scheduler/src/ValueObject/TaxonomyFieldStorageItem.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
9 | WARNING | The class short comment should describe what the class
| | does and not simply repeat the class name
----------------------------------------------------------------------

FILE: ...axonomy_scheduler/src/ValueObject/TaxonomySchedulerQueueItem.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
9 | WARNING | The class short comment should describe what the class
| | does and not simply repeat the class name
----------------------------------------------------------------------

FILE: ...contrib/taxonomy_scheduler/tests/src/Kernel/FieldStorageTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
13 | WARNING | The class short comment should describe what the
| | class does and not simply repeat the class name
----------------------------------------------------------------------

FILE: ...xonomy_scheduler/tests/src/Unit/TaxonomyFieldStorageItemTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
11 | WARNING | The class short comment should describe what the
| | class does and not simply repeat the class name
----------------------------------------------------------------------

Thanks for reviewing. I have updated the code to comply with PHPCS coding standards.

Hello @vinlaurens,

I have reviewed the changes, and they look fine to me.

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

Thanks

• 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 doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

taxonomy_scheduler.install

/**
 * Implements hook_uninstall().
 */
function taxonomy_scheduler_uninstall() {
  \Drupal::service('config.factory')
    ->getEditable('taxonomy_scheduler.settings')
    ->delete();
}

None of the Drupal core modules delete their own configuration objects, which I take it means Drupal core code does that automatically.

src/Service/TaxonomySchedulerFieldManager.php

    catch (EntityStorageException $e) {
      $this->logger->error($e->getMessage());
    }

For exceptions, the function used to log them should be watchdog_exception(), or code similar to that should be used.

src/Form/AdminForm.php

  /**
   * ConfigImmutable.
   *
   * @var \Drupal\Core\Config\ImmutableConfig
   */
  private $configImmutable;

  /**
   * Config.
   *
   * @var \Drupal\Core\Config\Config
   */
  private $configEditable;

There is no need to use those properties, in a class that extends ConfigFormBase</em>. It is sufficient to use its <code>config() method.
It would also be better to use more than a word for the property description.

    if (\is_array($storedVocabs)) {
      $disabled = \array_diff($storedVocabs, $vocabularies);

      if (!empty($disabled)) {
        $fieldStorageItem = new TaxonomyFieldStorageItem([
          'vocabularies' => $disabled,
          'fieldLabel' => $fieldLabel,
          'fieldName' => $fieldName,
          'fieldRequired' => $fieldRequired,
        ]);
        $this->fieldManager->disableField($fieldStorageItem);
      }
    }

It is not necessary to use the full namespace for the PHP functions in the root namespace.

src/Plugin/QueueWorker/TaxonomySchedulerQueueWorker.php

/**
 * Class TaxonomySchedulerQueueWorker.
 *
 * Processes nodes with changed topic.
 *
 * @QueueWorker(
 *   id = "taxonomy_scheduler",
 *   title = @Translation("Taxonomy scheduler queue"),
 *   cron = {"time" = 300}
 * )
 */

The class name is not much helpful as short description.

  /**
   * TaxonomySchedulerQueueWorker constructor.
   *
   * @param array $configuration
   *   The configuration.
   * @param string $pluginId
   *   The plugin id.
   * @param array $pluginDefinition
   *   The plugin definition.
   * @param \Drupal\taxonomy\TermStorageInterface $termStorage
   *   The term storage.
   * @param \Drupal\Component\Datetime\TimeInterface $dateTime
   *   The time interface.
   * @param \Drupal\Core\Config\ImmutableConfig $config
   *   The config.
   */

The class name given in the first line needs to contain its full namespace too.

tests/src/Kernel/FieldStorageTest.php

  /**
   * Modules.
   *
   * @var array
   */
  public static $modules = [
    'hook_event_dispatcher',
    'automated_cron',
    'taxonomy_scheduler',
    'field',
    'taxonomy',
    'text',
    'datetime',
  ];

That property is already set as private, in the parent class.

taxonomy_scheduler.info.yml

core: 8.x
core_version_requirement: ^8 || ^9

Since Drupal 8 is a unsupported branch, it should be better to update the core requirements.

Hi apaderno,

Thanks for reviewing. I have amended the code due to some of your comments, for others, I have some questions.

None of the Drupal core modules delete their own configuration objects, which I take it means Drupal core code does that automatically.

Alright. I have removed this code and it indeed seems that the configuration is already deleted upon module uninstallation.

For exceptions, the function used to log them should be watchdog_exception(), or code similar to that should be used.

Isn't it cleaner to use dependency injection here? As far as I see this is merely a wrapper function that ends up doing a static call to Drupal::logger in the end. Is this part of the standard or merely a suggestion?

There is no need to use those properties, in a class that extends ConfigFormBase. It is sufficient to use its config() method.
It would also be better to use more than a word for the property description.

Good point. I have refactored the code to use the parent's class config() method.

It is not necessary to use the full namespace for the PHP functions in the root namespace.

According to my mentor, who taught me to define the namespace for PHP functions, there is a small performance win for defining the namespace of these functions. I have however not tested this myself.

Since Drupal 8 is a unsupported branch, it should be better to update the core requirements.

Updated. I have also tested this module against Drupal 10 and made necessary changes, which makes it now compatible with both 9.x and 10.x.

I have updated the comment blocks.

All changes are reflected in the 2.x branch.

Thanks for your efforts.

This can be reviewed again with the last code changes.

Just a note: Drupal 9.3 requires PHP 7.3, which cannot use type-hinted properties like private QueueInterface $queue;. Either the module expressely requires PHP 8.1, or it requires only Drupal 10, which runs only with PHP 8.1.

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.