Provide Scheduler drush commands for Drush 9

This is interesting. Thanks for the link to Moshe's blog and the patch. I have not used drush 9 yet. Is this issue really 'critical'? I'm not saying it isn't, but I would like to hear your views and background info.

Jonathan

Hi jonathan, we can't use the cron url as we have a reverse proxy before the webserver. So Drush is our only option. Also Drush 8 is no longer supported for Drupal 8.3+.
You can set it to normal or major or keep it critical but without this patch this module becomes useless in our situation.

Regards

Thanks for the info. Yes I understand now. Given that you have this patch, and no-one else has raised it yet, I will set it down to major. I just need to test it, but I don't see any problem in getting this committed.

'Port' in the issue summary suggests the old code is removed, but this patch only adds new code. Let's either:
- Update the issue title
- Remove the drush 8 command

@idebr, updated this issues title. It would be useful if you could provide feedback on the patch.

The patch works fine. Some minor remarks:

+++ b/src/Commands/SchedulerCommands.php
@@ -0,0 +1,55 @@
+ * See these files for an example of injecting Drupal services:
+ *   - http://cgit.drupalcode.org/devel/tree/src/Commands/DevelCommands.php
+ *   - http://cgit.drupalcode.org/devel/tree/drush.services.yml

There is not much added value in referring to another contributed module's implementation of dependency injection, since it is already implemented a few lines below. The comment has a Drupal code violation as well (Doc comment long description must end with a full stop), so I would suggest removing these lines altogether.

+++ b/src/Commands/SchedulerCommands.php
@@ -0,0 +1,55 @@
+   * Lighweight cron to process scheduler tasks.

Lighweight -> Lightweight

Good point, fixed the typo and removed the redundant documentation.

@gchauhan: the cron() method in your patch is not doing anything, not sure what you're trying to do here?

Could you provide an interdiff?

The change to composer.json looks mostly like re-formatting, is this for coding standards? If so, it should be in a separate issue as it confuses things here. An interdiff would be very helpful.

@jonathan1055, the composer file now contains a link to the drush service:
+ "license": "GPL-2.0+",
+ "extra": {
+ "drush": {
+ "services": {
+ "drush.services.yml": "^9"
+ }
+ }
}

Attached is a new patch I made to add this functionality, since a client will be needing it soon. I had some trouble applying the previous ones so I started from scratch.

I also fixed a small typo in the drush 8 scheduler.drush.inc file.

Just a followup to my submitted patch. As I mentioned I had trouble with previous patch. That was #12 which didn't work for me. Looking at the code for #10 however it looks like that would have worked fine. As such my patch and @mpp #12 patch seem to accomplish the same thing.

Thank you @PCate and @mpp. I have compared the patches in #10 and #17 and taken the best from each (even though they mostly did the same thing). I have also enhanced the command by adding another option --nolog which will inhibit the 'started' and 'completed' lines from being written to the dblog (and hence also the terminal) even if the site config setting has this on, i.e. it can override the site setting. I also added an indication in the log message to show if the cron had been activated from drush, admin user form or the url.

Thanks for spotting the typos, I have moved them out of this patch and already committed them - see #3033108-24: Miscellaneous changes before Scheduler 8.x-1.1

One question, in patch #10 we had:

+  public function __construct(SchedulerManager $schedulerManager) {
+    parent::__construct();
...

But in the #17 patch the call to parent::__construct() is missing. What is the best practice here? Should this be in? Works OK without at the moment.

This patch has all the changes, plus additions to composer.json in an attempt to have green passing tests. The problem is caused by #2985051: Patching composer.json shows we need to add module dependencies

I would like to commit the drush9 changes within the next few days, as the new release of Scheduler is imminent and it would be very good to have this feature included. Please test and let me have any feedback asap. Thanks!

I would like to commit the drush9 changes within the next few days, as the new release of Scheduler is imminent and it would be very good to have this feature included. Please test and let me have any feedback asap. Thanks!

I just finished testing the patch. It applied cleanly to the latest dev version and worked fine for me.

Thanks @PCate for testing, good to hear it worked, and thank you for the prompt reply.

I found this stackoverflow answer to my question about parent::__construct so have decided to leave it in.

Thanks all. It is good that this addition will now make it into Scheduler 8.x-1.1 which is coming very soon.

Thansk @jonathan1055. And yes, we should leave it in :-)

Somehow in using code from both the patches I managed to get the property names $manager and $schedulerManager muddled. The commands did work, and I tested it all, but must have then renamed the property from PCate's patch to match mpp's but not got it right.

Issue #3078674: Call to function runLightweightCron() on null in SchedulerCommands->cron() has the fix.

I had been wondering about how to write phpunit tests to check drush commands. If we had some, then this typo could have been detected.