After I manually run lightweight cron on admin/config/content/scheduler/cron
I have the message 'Lightweight cron run completed - see log for details.'
Since I am using syslog, the link to dblog doesn't exist.

We need to add a check and only show the dblog message if the module is enabled.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcelovani created an issue. See original summary.

marcelovani’s picture

Status: Active » Needs review
FileSize
2.11 KB

D7 patch

marcelovani’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
FileSize
1.89 KB

D8 Patch

joekers’s picture

+ if (\Drupal::moduleHandler()->moduleExists('block')) {

I think this should be checking for the dblog module instead of block? :)

Other than that it looks good to me.

Status: Needs review » Needs work

The last submitted patch, 3: dblog-2611686-d8-3.patch, failed testing.

marcelovani’s picture

ha, good spot, copy > paste > forgot to change it LOL

marcelovani’s picture

Status: Needs work » Needs review
FileSize
1.89 KB

Status: Needs review » Needs work

The last submitted patch, 7: dblog-2611686-d8-7.patch, failed testing.

marcelovani’s picture

Status: Needs work » Needs review

The last submitted patch, 3: dblog-2611686-d8-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: dblog-2611686-d8-7.patch, failed testing.

marcelovani’s picture

Version: 8.x-1.x-dev » 7.x-1.x-dev
Status: Needs work » Needs review
marcelovani’s picture

The last submitted patch, 3: dblog-2611686-d8-3.patch, failed testing.

marcelovani’s picture

Looks like D8 tests are not passing at all.

Status: Needs review » Needs work

The last submitted patch, 7: dblog-2611686-d8-7.patch, failed testing.

marcelovani’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Needs work » Needs review
joshi.rohit100’s picture

Status: Needs review » Needs work
+++ b/src/Form/SchedulerCronForm.php
@@ -120,11 +120,11 @@ class SchedulerCronForm extends ConfigFormBase {
-    try {
+    if (\Drupal::moduleHandler()->moduleExists('dblog')) {
       $url = Url::fromRoute('dblog.overview')->toString();

Should be injected

marcelovani’s picture

err, did you forget to attach your path?

joshi.rohit100’s picture

Nope! I just reviewed your patch at #7.

marcelovani’s picture

Since you know a better way of doing it, it would be nice if you could collaborate with a patch

joshi.rohit100’s picture

I didn't provide the patch as it was assigned to you. Now assigning myself :)

marcelovani’s picture

cool

joekers’s picture

Status: Needs work » Needs review
FileSize
2.93 KB

Something like this @joshi.rohit100?

Sorry to jump in there but I've never used DI in Drupal 8 so I thought it would be a good opportunity to try :)

Status: Needs review » Needs work

The last submitted patch, 24: only_display_dblog-2611686-24.patch, failed testing.

joshi.rohit100’s picture

Assigned: joshi.rohit100 » Unassigned
  1. +++ b/src/Form/SchedulerCronForm.php
    @@ -19,6 +20,30 @@ use Symfony\Component\Routing\Exception\RouteNotFoundException;
    +  /**
    +   * Creates a SchedulerCronForm.
    +   *
    

    should be "Creates a SchedulerCronForm instance."

  2. +++ b/src/Form/SchedulerCronForm.php
    @@ -19,6 +20,30 @@ use Symfony\Component\Routing\Exception\RouteNotFoundException;
    +   * @var \Drupal\Core\Extension\ModuleHandlerInterface
    +   *   The module handler service.
    +   */
    

    missing parameter name

Missing the use statement.

Also it would be nice if we just change module_handler to moduleHandler (only property not in constructor) as core uses this name.

Patch is there, so unassigning!

joekers’s picture

Thanks for the feedback @joshi.rohit100.

joshi.rohit100’s picture

Status: Needs work » Needs review
pfrenssen’s picture

Status: Needs review » Fixed
FileSize
1 KB

This is much better, nice improvement! Thanks a lot guys!

I removed an unused use statement and fixed some minor coding standards before committing.

  • pfrenssen committed ace78d8 on 8.x-1.x authored by marcelovani
    Issue #2611686 by marcelovani, joekers, joshi.rohit100, pfrenssen: Only...
jonathan1055’s picture

Version: 8.x-1.x-dev » 7.x-1.x-dev
Status: Fixed » Patch (to be ported)

Thanks, good work.

As this issue was originally raised at 7.x I think it will be worth making the fix there too. Seems like the original patch at #1 is correct? Are you ok if I commit it?

pfrenssen’s picture

Yes, absolutely! I didn't intend to bypass this for D7. Patch looks good!

jonathan1055’s picture

Status: Patch (to be ported) » Fixed

Done. Thanks marcelovani for the original 7.x patch

Status: Fixed » Closed (fixed)

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

The last submitted patch, 24: only_display_dblog-2611686-24.patch, failed testing.

Status: Closed (fixed) » Needs work

The last submitted patch, 27: only_display_dblog-2611686-27.patch, failed testing.

jonathan1055’s picture

Status: Needs work » Closed (fixed)

Ignore the failed patch above. It was 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.