Replace variable_get() with config

I made the first iteration.

I've replaced the occurences of variable_get. Still some variables coming from other modules like Date have unknown location and need reviewed.

After analyzing your patch, I think we still have some work to do. The approach is good, but we still need to rework some parts. In general, I think we need to re-think about the naming of all the variables. Using variables with more then 1 underscore can be added into a nested parent variable. Here is an overview of my detailed comments;

Weird parameter provided in config() method.

Maybe this is a typo? I think it needs to be replaced with date.
if (\Drupal::config('??')->get('configurable_timezones')) { ...

Usage of scheduler.schema

I noticed that you are using scheduler.schema instead of scheduler.settings. This needs to be changed, otherwise Drupal won't be able to interact with the configuration file (scheduler.settings.yml).

Wrong usage of the config parameter.

In some parts of your code you are using the wrong configuration parameter structure. Instead of a . (dot), you're using a _ (underscore).
$create_publishing_revision = \Drupal::config('scheduler_schema')->get('scheduler_publish_revision_' . $n->type) == 1;
Should become
$create_publishing_revision = \Drupal::config('scheduler.settings')->get('scheduler_publish_revision_' . $n->type) == 1;

Investigation of vertical tabs

You added a comment saying that we have to investigate the yaml file of the vertical tabs. This seems unnecessary to me because we have to store this as a variable inside our variables.
+ $use_vertical_tabs = \Drupal::config('vertical_tabs')->get('use_vertical_tabs_' . $form['type']['#value']);
Should become;
+ $use_vertical_tabs = \Drupal::config('scheduler.schema')->get('use_vertical_tabs_' . $form['type']['#value']);

Thanks for the detailed analysis. I posted a new iteration.

My questions are:

1. You mention will be using only one setting yml file and that will be always: scheduler.settings.yml. In this case in your latest suggestion is it a typo: 'scheduler.schema'?
2. What is the real purpose of scheduler.schema?
3. I expect I need to fill in scheduler.settings with default values of the all variables over the project.
How should be handled the defaults for variables like use_vertical_tabs_' . $form['type']['#value'] ? You mention this 'Using variables with more then 1 underscore can be added into a nested parent variable'. Could you point me to a module where I can see this?

Tiny fix over @todo comments.

#2418527: Replace variable_set() with config has landed so this isssue can now be unblocked.

I was not able to apply the patch from #6. Already too many changes went in after this patch has been created. I created a new patch from scratch.

I have updated most of patch in comment #11 to use the ThirdPartySettings. The only thing I'm not sure on is where I've used it in scheduler_feeds_processor_targets_alter() and scheduler_i18n_sync_options()?

@@ -536,9 +537,9 @@ function scheduler_field_extra_fields() {
+ $publishing_enabled = $type->type->entity->getThirdPartySetting('scheduler', 'scheduler_publish_enable', 0);
+ $unpublishing_enabled = $type->type->entity->getThirdPartySetting('scheduler', 'scheduler_unpublish_enable', 0);
+ $use_vertical_tabs = $type->type->entity->getThirdPartySetting('scheduler', 'scheduler_use_vertical_tabs', 1);

We should store this entity inside a local variable. Something like:

$entity = $type->type->entity;

@@ -579,17 +580,14 @@ function scheduler_preprocess_node(&$variables) {
if ($entity_type->getThirdPartySetting('scheduler', 'scheduler_publish_enable')) {

I suggest to keep this as a separate variable. Now that I'm reviewing it myself, I notice that it is hard to read.

diff --git a/scheduler.test b/scheduler.test
index 87f601b..6db0c9c 100644
@@ -176,6 +176,7 @@ class SchedulerFunctionalTest extends SchedulerTestBase {

Testcases are getting fixed in #2594615: Automated testing in 8.x [meta] so it's not necessary to update them in this issue.

Thanks for the feedback :)

Not sure how the changes to scheduler.test crept in there but I've removed them and made the other changes in this patch.

Thanks for working on this patch. I had a look and was just wondering the correct way to get thirdparty settings. In scheduler.module you have

$type->getThirdPartySetting('scheduler', 'scheduler_publish_past_date', 'error')

but it scheduler.cron.inc you have

$node->type->entity->getThirdPartySetting('scheduler', 'unpublish_revision', FALSE)

So the questions is whether the extra prefix 'scheduler' is needed in the second parameter or not?

I think that this problem may be hindering the test results in #2594615: Automated testing in 8.x [meta] so it would be good to get this sorted.

Yeah it shouldn't have the 'scheduler' prefix. I'll update the patch as soon as I get chance.

I've removed the 'scheduler' prefix on all of the getThirdPartySetting calls.

As I said in #15, I'm not sure on where I've used it in scheduler_feeds_processor_targets_alter() and scheduler_i18n_sync_options(), if we have access to getThirdPartySetting method? But the other guys have reviewed it so I'm guessing it's ok.

This all looks good in general, just a few points:

1. in rules.inc return ($node->type->entity->getThirdPartySetting('scheduler', 'publish_enable', 0) == 1); we can remove the == 1 as the value returned will be boolean.
2. It would help readability and reuse if we had consistent use of name for $type or $entity object when created by us. Maybe $entity is the better choice as it is derived from $node->type->entity
3. On a general point, if we have default values defined in the schema, do we actually need to supply a default in the various calls?

But overall this is good and I'd like to get it committed soon, as it is holding up progress on #2594615: Automated testing in 8.x [meta]. I can commit the code this evening if you do not have time to make a new patch.

1 & 2. Changes in the patch supplied.
3. I'm not really sure, I thought maybe it's good practice but then there are a couple where we don't supply a default value.

Thanks for this. I'm happy to make further changes for 3 after Pfressnen has reviewed later as he may have a strong opinion on this, but the main changes that this patch solves are vital so I'm going to commit it.

I have made two small changes from the patch you provided

4. Removed another '== 1', this time from scheduler_node_presave()
if ($type->getThirdPartySetting('scheduler', 'publish_touch', 0) == 1)<
5. Remove redundant comment from scheduler_i18n_sync_options()
// $bundle_name holds the content_type.

I will mark this as fixed. I think we should discuss the default values for config->get anf entity->getThirdPartySetting in a separate issue.

Thank you all for your input on this. Good to get it done and moving on.

Jonathan

Ignore the failed patches above. They were queued but not run until the 8.x committed codebase passed all tests. That happened with my commit a few minutes ago, and then the untested patches have suddenly come to life and been run. This issue is already fixed.