Replace variable_set() with config

scheduler.test remains

Applied the changes to scheduler.test and modified some parts of #2.

Best practice here is avoiding instantiating config objects multiple times within the same function, as this is a performance drain. The following code unnecessarily instantiates the config object 'scheduler.settings' twice.

\Drupal::config('scheduler.settings')->set('scheduler_publish_enable_scheduler_test', 1)->save();
\Drupal::config('scheduler.settings')->set('scheduler_unpublish_enable_scheduler_test', 1)->save();

A better solution is to instantiate the config object once, save it into a variable, and work with that variable for the rest of your code scope.

$config = \Drupal::config('scheduler.settings')
$config->set('scheduler_publish_enable_scheduler_test', 1);
$config->set('scheduler_unpublish_enable_scheduler_test', 1);
$config->save();

(Source: Simple Configuration API --> Best Practices)

I downloaded the Scheduler 8.x dev from 14th Oct to test the patch in #3, but it no longer applies:

mac2012:scheduler jonathan$ patch -b -p1 -V numbered < 2418527-replace-variable_set-with-config-2.patch
patching file scheduler.test
Hunk #1 succeeded at 600 with fuzz 2 (offset 564 lines).
Hunk #2 succeeded at 618 with fuzz 1 (offset 573 lines).
Hunk #3 FAILED at 684.
Hunk #4 FAILED at 702.
Hunk #5 FAILED at 713.
Hunk #6 FAILED at 728.
Hunk #7 FAILED at 755.
Hunk #8 FAILED at 775.
Hunk #9 FAILED at 792.
Hunk #10 FAILED at 947.
Hunk #11 FAILED at 1082.
Hunk #12 FAILED at 1134.
Hunk #13 FAILED at 1147.
Hunk #14 FAILED at 1165.
Hunk #15 FAILED at 1175.
Hunk #16 FAILED at 1184.
14 out of 16 hunks FAILED -- saving rejects to file scheduler.test.rej

patching file src/Form/SchedulerAdminForm.php
Hunk #1 succeeded at 136 (offset -7 lines).
Hunk #2 succeeded at 154 (offset -7 lines).

patching file tests/scheduler_api.test
Hunk #1 succeeded at 41 with fuzz 2 (offset 12 lines).
Hunk #2 succeeded at 76 with fuzz 2 (offset 12 lines).

Not surprising really, as you've all been doing great work and since 9th Feb the code base will have changed considerably.

The failures were only in the .test file, so I could test the scheduler admin settings. The date format is saved ok. But if I tick the 'date only' option and save, the value is not saved. The watchdog has

Warning: call_user_func_array() expects parameter 1 to be a valid callback, function 'admin_submit' not found or invalid function name
 in Drupal\Core\Form\FormSubmitter->executeSubmitHandlers()
  (line 116 of /Library/WebServer/Documents/drupal8/core/lib/Drupal/Core/Form/FormSubmitter.php).

Removing the submit handler line 82

$form['#submit'][] = 'admin_submit';

stops the error being produced, but the option and default time are not ssaved.

On the 'Lightweight cron' tab the log checkbox and cron access key are saved correctly.
So it seems to be just the 'date only' fieldset which is not catered for yet.

Just to try, I moved the checkbox out of the fieldset into a stand-alone item and it was saved correctly. So there must be a new way to cater for fields inside fieldsets. Hope that helps.

Maybe in function submitForm we need to traverse to the lower level within the fieldset?

    foreach (Element::children($form) as $variable) {
      $config->set($variable, $form_state->getValue($form[$variable]['#parents']));
    }

Does Element::children return only the immediate children, or all the lowest level form items? I have limited internet search capacity right now, but I expect this is simple to fix.

Thanks to the power of searching drupal core code, I think it is simply a case of naming the variables directly:

    $config->set('allow_date_only', $form_state->getValue('allow_date_only'));
    $config->set('default_time', $form_state->getValue('default_time'));

The above works. But do we leave the loop in and just do the fieldset ones as extra? or should we do this explicitly for each variable (there are not many). I would favour the explicit lines, as is done in core, as it makes it easier to read.

Oops, sorry joshi.rohit100 I did not see you had assigned this. Please go ahead, you are welcome to work on it :-)

It seems like tests are not properly implemented (or wrongly implemented), so I haven't changed those. I think, it will be good to have follow up to correct the test cases.

Thoughts ?

Hi joshi.rohit100,
Your change to instantiate the config object once at the top is good, thanks for doing that. Some points to note:

1. You have a typo when saving the date_only_format - both written to 'time'
   

   $config
     ->set('time_only_format', $time_only_format)
     ->set('time_only_format', $date_only_format)
     ->save();
   

2. need to remove the submit handler (see #8 above)
3. can remove the config->save() near the top formSubmit
4. We also need to save the two values in the fieldset (see #11 above)

Regarding the tests, yes there is already a separate issue #2594615: Automated testing in 8.x [meta] to get them running properly. However, that should not stop us making the variable_set() changes to the .test files within this issue, even if the tests do not run those code changes need to be made anyway in preparation.

Thanks,
Jonathan

Hi jonathan1055,
I have made the changes which you have mentioned. Kindly review it.

The typo & submit handler have been removed. The config->save() has been removed as well.
Instead of using a foreach, I decided to go for the line-per-line approach in order to set the values for the fieldset. The foreach loop we used previously was also storing stuff inside our config we really don't want to have in there (E.g. form_id, ...).

The issue you had that values weren't saved while submitting the form was because we didn't save the values from the form elements inside the fieldset. I've tested this patch and my values are getting stored now.

I also udpated the .test files as you requested.

Nice work, thanks for this.

Changes to test files are now being handled in #2594615: Automated testing in 8.x [meta] on a separate git branch, and the variable_get() changes are being handled in #2418523: Replace variable_get() with config. However, the other changes I think look good. If you could re-roll the patch without the .test and variable_get() changes it will be easier to make a final check, before committing the code.

Just spotted that the config->set of the main date format is missing. If you want me to re-roll the patch, say so, but as it is assigned to you it's fair to let you do it.

Re-rolled the patch and fixed #21 and #22

One minor change I did: I've removed the line where we set the variable $config because we're only using that variable once.

I guess patch is missing few things from #6

Joshi.rohit100 can you be a bit more specific?

In the buildForm (), initialising config object once.

 $config = $this->config('scheduler.settings');

I am not sure if it comes under variable_get() issue.

joshi.rohit100, I'm not initializing the $config object in the buildForm() method?

The only thing I forgot was removing the submit handler from the form. This is done in the attached patch.

+++ b/src/Form/SchedulerAdminForm.php
@@ -131,22 +128,19 @@ class SchedulerAdminForm extends ConfigFormBase {
-    $config = $this->config('scheduler.settings');

i think this should not be removed as you see we are just creating new instance every time by \Drupal::service()

Even if we don't remove it, we're not creating a new instance. ConfigFormBaseTrait comes with a config() method which we're using here. Calling this method will retrieve a configuration object.

Also, the code isn't completely removed. I just made the code easier to read. Why should we create a variable which we're only using once? Instead of:

<?php
    $config = $this->config('scheduler.settings');
    $config
      ->set('time_only_format', $time_only_format)
      ->set('date_only_format', $date_only_format)
      ->set('date_foramt', $format)
      ->set('allow_date_only', $form_state->getValue(['allow_date_only']))
      ->save();
?>

I'm doing:

<?php
    $this->config('scheduler.settings')
      ->set('time_only_format', $time_only_format)
      ->set('date_only_format', $date_only_format)
      ->set('date_foramt', $format)
      ->set('allow_date_only', $form_state->getValue(['allow_date_only']))
      ->save();
?>

I agree with your point that we have config() method in trait that we can use instead of creating new variable.

But

Why should we create a variable which we're only using once?

We are using \Drupal::config() three times in the buildForm method and I think, that can help there.

Ok, now I finally got your point. Sorry, I blame Monday! ;-)

+++ b/src/Form/SchedulerAdminForm.php
@@ -35,12 +35,13 @@ class SchedulerAdminForm extends ConfigFormBase {
+    $config = \Drupal::config('scheduler.settings');

use $this->config('config_name')

Done #33!

Legovaer,
Thanks for re-rolling the patch - it seems to have been a bit of a fuss. The part of the patch in 21 for this specific file was almost perfect, so I'm not sure why you did not start with that patch (removing the sections for the other files) then just make your additional changes on top. You've ended up missing then re-doing quite a few of the changes already accepted, so just offering a little suggestion to save yourself time next time :-)
Also I spotted a typo where you set ->set('date_foramt', $format), and you missed out ->set('default_time', $form_state->getValue('default_time')) so I am guessing you did not actually try out your changes and test them to see if they worked. Until we get the automated tests running manual testing is vital ;-)

joshi.rohit100
Thanks very much for your reviewing and guidance. Nice work.

Here's a patch with the two typos fixed, and an interdiff to show what the work was. I noticed that $form_state->getValue('default_time') works so is there any reason/benefit to also use [ and ] inside the getValue, as done in $form_state->getValue(['allow_date_only']). Both formats seem to work.

When we've answered the above, and if you are both happy with this, I will commit it, then we can unblock #2418523: Replace variable_get() with config

Jonathan,
You're 100% right. I made a mess with all my re-patching. I'm still getting used to the "patch" way-of-working. It would be so much nicer to have branches!

The patch in #35 looks OK to me.

Fixed !

Thank you everyone. That was my first commit to 8.x :-)

Marking this fixed, but if Pieter reviews our work and would like anything changed, I'm happy to do that. It's good to get this fixed, then we can move on to the other issues being blocked by it.

Jonathan