Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kmoll created an issue. See original summary.

kmoll’s picture

Issue tags: +Vienna2017
kmoll’s picture

Initial port of drush commands to drush 9 syntax.

kmoll’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: 2912708-port-drush-3.patch, failed testing. View results

kmoll’s picture

oops uploaded wrong patch. This is the right one.

kmoll’s picture

Status: Needs work » Needs review
kmoll’s picture

I've updated the patch to move the drush command service to drush.services.yml to follow best practices.

Status: Needs review » Needs work

The last submitted patch, 8: 2912724-port-drush-8.patch, failed testing. View results

GaëlG’s picture

Status: Needs work » Needs review
FileSize
13.14 KB
704 bytes

Thanks kmoll! The previous patch did not work for "drush cron-run" (all jobs). Here's an update.

avogler’s picture

I manually tested the patch and it works for me. As far as I can tell the code looks solid.

+RTBC

GaëlG’s picture

Status: Needs review » Reviewed & tested by the community
avogler’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
13.13 KB

Sorry to reopen, but I found a minor bug. The --force option on cron-run was not working as $option was overwritten with $option['option]. I have attached a corrected patch.

Christopher Riley’s picture

Any word on getting this committed its been a couple of months and no word on this.

Berdir’s picture

Status: Needs review » Needs work
+++ b/src/Commands/UltimateCronCommands.php
@@ -0,0 +1,439 @@
+
+    if (!$name) {
+      // Run all jobs.
+      $jobs = CronJob::loadMultiple();
+
+      /** @var \Drupal\ultimate_cron\Entity\CronJob $job */
+      foreach ($jobs as $job) {

This copies the known an existing bugs from the current commands.

This doesn't make sense and it skips a ton of logic by not going through the runner.

Ther is no need for this option,use the core cron command instead.

The 9.x switch is a good moment to remove support for not passing a $name. just point reference the core command instead in the documentation.

gchauhan’s picture

FileSize
6.06 KB

Hi all,
Adding the patch for drush9 commands. I have followed this blog.

rgpublic’s picture

Ok, #16 was of course trash, so I'm building on #13. I've added Berdir's wishes from #15. Do we need anything else to get this committed?

rgpublic’s picture

Come to think of it, we should probably remove the second @usage annotation, right? I wonder: Do we still need the options at all? If yes, we might need a better example, because the --options=thread=1 doesnt make much sense to me in connection with a single cronjob run.

jsst’s picture

Could you explain how you intend to make it possible to run jobs in parallel?

For example, I would like to:

- run some import jobs in thread 1
- run some export jobs in thread 2
- run all other core jobs seperately

This patch currently seems unable to run threads, and when using 'core:cron' to run all other jobs, won't it run my import/export jobs as well? I don't want the core jobs to be waiting for imports!

rgpublic’s picture

I'm now thinking this patch should be comitted as-is. Perfectionism all good and well, but in fact we don't have any usable ultimate_cron drush commands with Drush9 for over 2 years now. I'm using the patch without any major problems for quite some time. I think it's still better to be able to start cronjobs from CLI *at all* - even without any cool threading capabilities etc.

Berdir’s picture

Status: Needs work » Needs review
FileSize
13.06 KB

Rerolled.

  • Berdir committed d219980 on 8.x-2.x authored by kmoll
    Issue #2912724 by kmoll, GaëlG, rgpublic, Berdir, avogler, gchauhan:...
Berdir’s picture

Status: Needs review » Fixed

Removed the second usage example and committed. If someone needs other options they can add them later on.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.