General settings should be plugins, as there can be more settings which would need implementation later, like nodejs.
Currently it is hardcoaded in the general settings form.

Issue #2692781 Should be merged before this ticket for the test to pass.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

abhishek-anand created an issue. See original summary.

abhishek-anand’s picture

Status: Needs work » Needs review
FileSize
41.89 KB

Status: Needs review » Needs work

The last submitted patch, 2: settings_as_plugin-2708789-1.patch, failed testing.

abhishek-anand’s picture

Status: Needs work » Needs review

The test will pass only after https://www.drupal.org/node/2692781 is merged.

abhishek-anand’s picture

abhishek-anand’s picture

Status: Needs review » Needs work

The last submitted patch, 6: settings_as_plugin-2708789-6.patch, failed testing.

abhishek-anand’s picture

abhishek-anand’s picture

Issue summary: View changes
abhishek-anand’s picture

Issue summary: View changes
Berdir’s picture

I did not port those settings plugins more or less on purpose.

As far as I see, it adds a ton of complexity and a huge amount of functions and methods to forward many hooks to our plugin classes. That seems over-engineered to me, that code can just implement those hooks.

  1. +++ b/src/Plugin/ultimate_cron/Settings/QueueSettings.php
    @@ -0,0 +1,380 @@
    +  /**
    +   * Get cron queues and static cache them.
    +   *
    +   * Works like module_invoke_all('cron_queue_info'), but adds
    +   * a 'module' to each item.
    +   *
    +   * @return array
    +   *   Cron queue definitions.
    +   */
    +  private function get_queues() {
    +    $module_handler = \Drupal::moduleHandler();
    +    if (!isset(self::$queues)) {
    +      $queues = [];
    +      foreach ($module_handler->getImplementations('cron_queue_info') as $module) {
    +        $items = $module_handler->invoke($module, 'cron_queue_info');
    +        if (is_array($items)) {
    +          foreach ($items as &$item) {
    +            $item['module'] = $module;
    +          }
    +          $queues += $items;
    +        }
    +      }
    +      $module_handler->alter('cron_queue_info', $queues);
    +      self::$queues = $queues;
    +    }
    

    this hook doesn't exist anymore, as mentioned in the other issue, queue processing happens in \Drupal\ultimate_cron\UltimateCron::run(). If we need more than that, then we should just implement it there.

  2. +++ b/src/Plugin/ultimate_cron/Settings/QueueSettings.php
    @@ -0,0 +1,380 @@
    +      }
    +
    +      $items['queue_' . $name] = [
    +        'title' => t('Queue: @name', ['@name' => $name]),
    +        'callback' => [get_class($this), 'worker_callback'],
    +        'scheduler' => [
    +          'simple' => [
    +            'rules' => ['* * * * *'],
    

    This hook also doesn't exist anymore.

    What this basically seems to do is expose each queue implementation as a cron job, so you can customize how often they are called.

    That sounds useful. I think that's what the other queue task should do. Some notes on that:

    * to get queue plugins, we need $this->queueManager->getDefinitions()
    * Discovery and definition of cron jobs is currently in \Drupal\ultimate_cron\CronJobDiscovery::discoverCronJobs(), we could implement the queue logic there.
    * We need to extend the cron job entity somehow to be able to call a queue. I'm not exactly sure yet how, we currently only have a callback. We could support arguments for that callback and pass in the ID of the queue plugin.

  3. +++ b/src/Plugin/ultimate_cron/Settings/QueueSettings.php
    @@ -0,0 +1,380 @@
    +  static public function worker_callback($job) {
    +    $settings = $job->getPluginSettings('settings');
    +    $queue = \Drupal::queue($settings['queue']['name']);
    +    $function = $settings['queue']['worker callback'];
    +
    +    $end = microtime(TRUE) + $settings['queue']['time'];
    +    $items = 0;
    +    while (microtime(TRUE) < $end) {
    +      if ($job->getSignal('kill')) {
    

    I suppose that's what this code is doing. This would then live.. somewhere, possibly on its own service, see \Drupal\Core\Cron::processQueues() for the current core implementation.

Berdir’s picture

#2704543: Implement cron queue processing did a lot of this now, see issue summary there. The remaining parts are currently not a high priority, so lets wait until someone has a use case for this.

Berdir’s picture

#2728873: Ensure that cron jobs can have services as callbacks, refactor queue worker to one on the other hand would be a nice issue to have someone working on :)