Remove variable_set()/variable_get() from cron_example

Please review.

Any reason that REQUEST_TIME is not used and time() is? Was going to create a separate D7 issue related for this, but I see time() is used a lot in this patch.

@Alan D. You are right!
The changes in this patch.

Suggestion -
1. As drupal core is going near to short array syntax, I think we should also do the same.
2. In Form class, we are extending configformbase and still we are using old style t() not $this->t().

1. +++ b/cron_example/lib/Drupal/cron_example/Forms/CronExampleForm.php
   @@ -0,0 +1,223 @@
   +   */
   +  public function addItems(array &$form, FormStateInterface &$form_state) {
   +    $queue = $form_state['values']['queue'];
   

   I think $form_state should not be referenced.

2. +++ b/cron_example/lib/Drupal/cron_example/Forms/CronExampleForm.php
   @@ -0,0 +1,223 @@
   +    $queue = \Drupal::queue($queue);
   

   it will be good if service is injected.

@joshi.rohit100 thank you for your review and your suggestions.

Here the patch.

Wrong patch extension.

Upss sorry :)

+++ b/cron_example/lib/Drupal/cron_example/Forms/CronExampleForm.php
@@ -0,0 +1,233 @@
+ * @file
+ * Contains \Drupal\cron_example\Form\CronExampleForm

namespace is wrong.

Also as I noticed, cron_example module is not using PSR-4 standard (as currently drupal 8 is using).

Here the fix :)

Right!

I was bug hunting so I did not resolve any of the things from #19.

1) Fixed a couple of minor nitpicks in /cron_example/src/Form/CronExampleForm.php

2) Replaced a couple of reference to $this->t in a *.module file.

(2) should bring the error count down.

+++ b/cron_example/src/Form/CronExampleForm.php
@@ -0,0 +1,234 @@
+  protected function getEditableConfigNames() {
+    // TODO: Implement getEditableConfigNames() method.
+  }

I think, config name should be defined here for 'examples.cron'

1) #22 Yep, I also think that....
2) it is best practise to call your parent - and let it do its things, rather that implement what the parent currently does. Things change and that creates and maintenance problem.

If we are going to call set, we need to call getEditable() first

Here is the relevant change record.
https://www.drupal.org/node/2407153

This next change record points out that hook_cron_queue_info() has been removed and we should use plugins to define our queue workers

https://www.drupal.org/node/2341649

I have made a start on the change. - Somewhere I have made a mistake and I don't think the new queue workers are being registered as plugins

I am posting early, in the hope others may easily see the flaw in my work and suggest a fix :-)

I will have more time on the weekend to fix this.

Also I have made a series of minor coding standard changes

so $cron_config becomes cronConfig where appropriate etc.

1) I was using the old function name.
2) Improved a comment.

The same failures persist.

this patch should at least fix the test

Thank you

This is the code I wanted and was missing, willzyx++

-    $queue_1 = \Drupal::queue('cron_example_queue_1');
-    $queue_2 = \Drupal::queue('cron_example_queue_2');

+    $queue_1 = $this->queue->get('cron_example_queue_1');
+    $queue_2 = $this->queue->get('cron_example_queue_2');

All the MANY other fixes look good also :)

There were some comments in the code before I started making changes ... that as I changed things have gotten lost.

I will have a look tomorrow and put similar comments in the new code as appropriate.

This is demonstration quality code and so I want to criticize my own work ...

Having ReportWorkerBase is simple and gets the job done, but I think there is a better solution!

Should we use a Trait? ... or should the 2 plugins be using a decorator pattern to provide the functionality found in ReportWorkerBase::reportWork?

I will have to have a think...

the real problem was in the QueueWorker plugins.

+++ b/cron_example/src/Plugin/QueueWorker/ReportWorkerOne.php
@@ -6,10 +7,9 @@
- *   id = "cron_example_queue_1_worker",
+ *   id = "cron_example_queue_1",

+++ b/cron_example/src/Plugin/QueueWorker/ReportWorkerTwo.php
@@ -6,11 +7,9 @@
- *   id = "cron_example_queue_2_worker",
+ *   id = "cron_example_queue_2",

The id was cron_example_queue_*_worker but in CronExampleForm and in test was used cron_example_queue_* :)

the interdiff

Just making notes on #34

Visually the form looks much better so +1 to that change
The dependencies of the sub module are more correctly defined so +1.
All other changes look good.

So for changes in #36

1) Instead of using the $GLOBALS to pass messages - I have changed to using the state system.
2) I am using the string translation trait - as in the fullness of time t() will be deprecated.
3) As per #32 I have gone back and looked at #15 and reintroduced/refreshed some comments.

regarding #32 and a potential mini redesign ... after sleeping on it I think let just keep things simple and keep ReportWorkerBase.

So remaining tasks?

We need to implement the suggestions from #19 - I am out of time today but will try and get to it soon :)

I can see a flaw, that I will fix next week

The tests don't find this but once the new state variable is set to TRUE there is no mechanism to set it back to FALSE!

1) As per #37 - cron_example_show_status_message now resets correctly each time.

2) I have add comments as per the review in 19.

@Miles

Inline comments for why we're doing this, and why it's before the parent call.

I am not sure why it's called before the parent. - but then again apart from stylistic reasons I am not sure that is important.
the form state is not modified.

Please let me know if I have made a mistake.

I regard the text as a starting point I will happily tweak the language as people see fit.