Make global setting ui dynamic and use it

I am working on the schema test problems.

@abhishek-anand I have to stop for the night but if you want to see what I working on: https://github.com/tedbow/ultimate_cron

I think my changes to modules/contrib/ultimate_cron/config/schema/ultimate_cron.schema.yml

are correct

Ok I hope I didn't stray to far from the issue but I think the schema for the plugins needed to be updated to make this work.

I will comment on that patch after I upload it

+++ b/config/schema/ultimate_cron.schema.yml
@@ -134,37 +137,25 @@ ultimate_cron.job.*:
     logger:
-      type: mapping
-      label: 'Scheduler'
-      mapping:
-        id:
-          type: string
-          label: 'Logger ID'
-        configuration:
-          type: ultimate_cron.plugin.logger.[%parent.id]
+      type: ultimate_cron.plugin.logger.[id]
+      label: 'Logger'

In the schema for each of the plugins on the job we just need to tell the schema what type of plugin it is going to reference. We don't actually need to store the configuration here. The schema will figure it out by the type.

So the job entity will not be
$job->scheduler[configuration][rules][0]
but
$job->scheduler[rules][0]

An example of this in core is block.schema.yml in how it references condition plugins

visibility:
      type: sequence
      label: 'Visibility Conditions'
      sequence:
        type: condition.plugin.[id]
        label: 'Visibility Condition'

It is not exactly the same because blocks can have multiple condition plugins but it is same idea in that the block schema just has a reference to a condition plugin and the actual configuration is defined in the condition schema.

The patch also added a new test module ultimate_cron_plugin_test

It simply provides a new schedule plugin that extends Simple called Simpler.

I added a test to check the "simpler" CronJobFormTest that new plugin shows up in the job edit form.

I did not add a test the plugin show up here "admin/config/system/cron/settings/scheduler"
and just noticed that it doesn't so it is not working fully yet.

Since this issue is to "Make global setting ui dynamic " I am going to set it to needs work because the new plugin should just show up there correct?

@abhishek-anand just wanted to give you an update if you looking at this

I am working here https://github.com/tedbow/ultimate_cron and should be able upload patch later.

I am changing the general settings forms to actual use the plugin configuration forms.

I'll try to have a look at the ultimate cron issues over the weekend. Thanks for working on this.

One thing I'm not sure about yet is how those global settings should actually be used. With the current config entity, each entity has its own storage anyway and has all settings duplicate there, so those global settings could at best be used for new sensors.

We have a similar but much simpler concept in https://www.drupal.org/project/sharemessage, there each config entity has a checkbox whether the global settings should be applied or not. We could have the same here for each plugin type, and if "[ ] Override default settings" isn't checked, then it would fall back to the global settings.

Thoughts?

Sorry I am starting to work on this module without probably having the best idea of how these settings are working

there each config entity has a checkbox whether the global settings should be applied or not.

@Berdir this would seem to make sense but we would want to change the forms like SchedulerSettingsForm.

Right now this forms shows the configuration form for all scheduler plugins. This could be useful if we want to keep a default for each scheduler plugin type.
Say if you editing a CronJob and you switch from Simple to Crontab scheduler it could load the defaults from what was set for Crontab SchedulerSettingsForm.

But this doesn't actually set which scheduler should be the default scheduler.

Maybe we should have dropdown at the top of all the forms that extend PluginSettingsFormBase that would let you pick which plugin should be the default for that type.

Here is another patch.

I am still trying to figure out schema changes so test will fail for now

I'm not sure, but I'd be OK with only having global default configuration for a single default type. The only thing that is usually customized is scheduling.

Yes, we'd definitely had to change the form, that's OK. I would even get the type from the default settings because otherwise you can't really change the default type. We can also do that part in a separate issue.

Ok here is cleaned up patch. That should pass.

Looking at the D7 code it does look like it is grabbing the defaults for a specific plugin when in the cron job form.

From D7 \UltimateCronPlugin::jobSettingsForm :

// Default values for current selection. If selection differs from current
    // job, then take the job into account.
    $defaults = $current_plugin->name == $original_plugin ? $job->settings : array();
    $defaults += $current_plugin->getDefaultSettings($job);

You can see in this D7 screenshot of the cronjob form

I had just switched the cronjob to "cache" logger. In d7 you can that for each individual setting you can leave it blank for the default
For example for the cached in bin

Select which cache bin to use for storing logs. (Blank = my_new_default).

I had just changed the global default for bin to "my_new_default"

You can also see that when I choose the logger a can choose Database or Cache or the "Default(Database)"

Presumably if I choose "Default(Database)" it would just follow the default plugin for logger.

But if I choose "Default(Database)" it still gives me the option to override individual settings for the Database logger.

There is also "change default settings" like on each of the plugin forms on the CronJob form.

I don't have a lot of experience administering this module to a solid a opinion on if we should keep the D7 functionality or simplify it.

If it was my module I would vote for simplifying it.

re @Bedir's comment

I'm not sure, but I'd be OK with only having global default configuration for a single default type. The only thing that is usually customized is scheduling.

So if I am understanding this then on admin/config/system/cron/settings/logger instead of showing a tab or each Logger plugin we would just have

1. A dropdown to select the default logger plugin
2. Dynamically show the settings form for the chosen logger plugin

Then on an individual cronjob form in the Logger section we would show checkbox that would say "Use Default Logger" checked by default and show in markup what those default settings are.

If they uncheck "Use Default Logger" then we show the dropdown to choose the logger plugin and the settings form for the Logger Plugin they choose.

@Bedir let me know if that sounds right and you want me to implement that.

Fairly sure this needs a reroll now.

Re #20. Yes, that's exactly what I was suggesting.

I should be able to find time to work a bit on this myself unless you get to it first. If you do, make sure to leave a note on the issue and/or assign it to you. I'm unassigning for now.

Considering how much this is doing, I think we also need quite some tests. That would also help to understand what the patch is doing, by following the steps in the test.

First review, getting familar with the work here..

1. +++ b/config/install/ultimate_cron.settings.yml
   @@ -11,23 +11,36 @@ queue:
        threads: 4
        threshold: 10
   +default_plugins:
   +  launcher: serial
   +  scheduler: simple
   +  logger: database
    launcher:
   -  thread: 'any'
   -  max_threads: 1
   -  lock_timeout: 3600
   -  max_execution_time: 3600
   +  serial:
   +    id: 'serial'
   

   Per my suggestions above, I'd expect that the each plugin type (launcher) etc. will have just store configuration for one plugin, the one that was selected as default. The default will be stored in the id that you have now.

   The default_plugins part will then go away again.

2. +++ b/config/schema/ultimate_cron.schema.yml
   @@ -45,73 +45,37 @@ ultimate_cron.settings:
   -              label: Maximum number of logs to keep
   +      type: sequence
   +      label: Logger Settings
   +      sequence:
   +        type: ultimate_cron.plugin.logger.[id]
   +        label: Launcher
   

   This will then change from a sequence of that type to just that type.

3. +++ b/config/schema/ultimate_cron.schema.yml
   @@ -131,37 +95,25 @@ ultimate_cron.job.*:
   +    uuid:
   +      type: string
   +      label: 'UUID'
        rules:
   

   not sure what the uuid is for?

4. +++ b/src/CronJobDiscovery.php
   @@ -21,13 +22,19 @@ class CronJobDiscovery {
   +   * @var \Drupal\Core\Config\ConfigFactoryInterface
   +   */
   +  protected $configFactory;
   +
   +  /**
       * CronJobDiscovery constructor.
       *
       * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
       *   The module handler.
       */
   -  public function __construct(ModuleHandlerInterface $module_handler) {
   +  public function __construct(ModuleHandlerInterface $module_handler, ConfigFactoryInterface $config_factory) {
        $this->moduleHandler = $module_handler;
   +    $this->configFactory = $config_factory;
      }
    
      /**
   @@ -57,6 +64,14 @@ class CronJobDiscovery {
   
   @@ -57,6 +64,14 @@ class CronJobDiscovery {
            'callback' => $info['callback'],
          );
    
   +      /*
   +      // Set defaults
   +      $config = $this->configFactory->get('ultimate_cron_general_settings');
   +      $values['scheduler'] = [
   +        'id' => 'simple',
   +        'rule' => '',
   +      ];
   

   this needs a reroll now since this class now has the config factory injected more or less just like this.

   Also, given my suggestions, I think this will change to instead check the defaults at runtime unless an override flag is set.

5. +++ b/src/Form/LoggerSettingsForm.php
   @@ -1,20 +1,23 @@
    /**
   + * @file
     * Contains \Drupal\ultimate_cron\Form\LoggerSettingsForm.
   

   classes no longer need @file. I'd rather not touch it in existing issues, but if you do, just remove it instead of fixing it.

   I think the best thing would be a separate issue that does nothing but remove all @file docblocks in classes.

6. +++ b/src/Form/LoggerSettingsForm.php
   @@ -1,20 +1,23 @@
   +  const CRON_PLUGIN_TYPE = 'logger';
   +  /**
   +   * {@inheritdoc}
   

   missing empty line.

7. +++ b/src/Form/PluginSettingsFormBase.php
   @@ -0,0 +1,151 @@
   +/**
   + * @file
   + * Contains \Drupal\ultimate_cron\Form\PluginSettingsFormBase.
   + */
   

   yep, new classes shouldn't have it.

8. +++ b/src/Form/PluginSettingsFormBase.php
   @@ -0,0 +1,151 @@
   +  /**
   +   * The type of plugin this form should handle.
   +   *
   +   * @var string
   +   */
   +  const CRON_PLUGIN_TYPE = '';
   

   we can't really enforce that a constant is set/overriden.

   It requires a bit more glue code, but we could do so with an abstract protected method that subclasses have to implement.

   If these classes really all end up doing nothing, we could even pass it as an argument to the same non-abstract form class?

9. +++ b/src/Form/PluginSettingsFormBase.php
   @@ -0,0 +1,151 @@
   +   */
   +  public static function create(ContainerInterface $container) {
   +    return new static(
   +      $container->get('config.factory'),
   +      $container->get('plugin.manager.ultimate_cron.' . static::CRON_PLUGIN_TYPE)
   +    );
   

   We couldn't do this then as it would be too late, though. so not sure about that yet.

Straight reroll of #25 after #2704543: Implement cron queue processing
Assigning to myself to work on #27

unassigning from myself. I debugged it but did not found the schema exception error. So leaving this open for others to pick.

This fixes the test fails.

I'm not convinced by the configuration change. So far, I don't understand why it is necessary. The mentioned example in #12 doesn't explain that. Will talk to @tedbow about it.

Yes, it is possible, but the mentioned block config entity actually also has a different kind of example, with the main plugin + settings, which are separate keys. I don't really like to merge those things together, the bugfix shows how things can get more complicated if you do that.

I re-rolled this patch so it will apply; it seems there were quite a few changes elsewhere since this was last worked on.

I've updated the patch to pass tests. It was missing some ids in the install config, and I updated one schema definition.

Here's a new patch for the 8.x-2.0-alpha2 release.

Patch #40 doesn't apply on 8.x-2.0-alpha4.
Rerolled patch attached.

#44 not applying against 8.x-2.x, rerolled.

I'm noticing that the configuration the logger gets when a cron job is executed are just the defaults, its ignoring whatever you save in the configuration form.

Note that his also happens currently on HEAD, though this patch fails to address this still.

FWIW, attaching a patch adding a new logger plugin which is an attempt to use core's logger for those that need to stream logs somewhere else.

With #46 applied I am able to have it registered, shows up in the admin ui, and can be selected as default. However it doesnt seem to be getting called when a cron job is executed.

Rerolled patch #46, no longer applies on 8.x-2.x.

Corrected patch attached.

We're using this patch and get these schema errors, not 100% sure its because of the patch but it looks like it?

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for
ultimate_cron.job.simplenews_cron with the following errors:
ultimate_cron.job.simplenews_cron:scheduler.configuration missing schema,
ultimate_cron.job.simplenews_cron:launcher.configuration missing schema,
ultimate_cron.job.simplenews_cron:logger.configuration missing schema

Looks like this is now committed. Patch won't apply, and when I compare it to files, it looks like the changes are already there.

Patch #52 is not applying against 8.x-2.x. Rerolled.

Corrected scheduler field name in CronJobFormTest.php, interdiff attached.

The values that I saved in /admin/config/system/cron/settings/launcher not used anywhere.
Always used SerialLauncher::defaultConfiguration().

I hided patches because dorgflow tryes to apply them all.

Dirty fix for this.

#56 rerolled.

This should fix the info file problem.

Does this fix the tests?

@super_romeo: The settings form is saving correctly for me, could you please try again?

Does this need more tests? Or is this enough?

Rerolled.

Needs reroll for ultimate_cron 2.0@alpha8

Converted to a merge request and fix tests, but once again, I'm not convinced by what I see here.

Fundamentally, the whole thing still doesn't do anything. The default values are not used when creating new jobs. And this still does change the configuration structure of jobs, which would break existing configuration. Untangling that would require quite a bit of work.

Instead, I created #3489476: Disable unused global settings forms and other dead settings plugin classes as an intermediate step to unblock the beta release.