Support from Acquia helps fund testing for Drupal Acquia logo

Comments

abhishek-anand created an issue. See original summary.

abhishek-anand’s picture

This will have a conflict with ISSUE #2688313

abhishek-anand’s picture

Status: Active » Needs review
tedbow’s picture

Status: Needs review » Needs work

@abhishek-anand sorry getting familiar with this module

+++ b/src/EventSubscriber/UltimateCronSubscriber.php
@@ -0,0 +1,81 @@
+    $GLOBALS['ultimate_cron_original_user'] = \Drupal::currentUser();

Why is global variable used here?

These should avoid if at all possible but also I don't see global variable being read anywhere.

I assume you are saving the original user and planning to switch back at some point.

Could you do something like core does with \Drupal\Core\Session\AccountSwitcher?

abhishek-anand’s picture

Hi tedbow,

In d7 version the module switches the user using $GLOBALS['ultimate_cron_original_user'] in hook_cron_queue_info which is being set in hook_init

function ultimate_cron_cron_queue_info() {
  $debug = debug_backtrace();
  if (!empty($debug[3]['function']) && $debug['3']['function'] == 'drupal_cron_run') {
  ...
  ...
      }
      catch (Throwable $e) {
        // Restore the user.
        $GLOBALS['user'] = $GLOBALS['ultimate_cron_original_user'];
        drupal_save_session($GLOBALS['ultimate_cron_original_session_saving']);
        throw $e;
      }
      catch (Exception $e) {
        // Restore the user.
        $GLOBALS['user'] = $GLOBALS['ultimate_cron_original_user'];
        drupal_save_session($GLOBALS['ultimate_cron_original_session_saving']);
        throw $e;
      }

My idea was, if we have to port this to drupal 8 we can use \Drupal\Core\Session\AccountSwitcher but $accountSwitcher->switchTo will require a $account to switch, which is being set in the evenSubscriber.
I think we cannot use $accountSwitcher->switchBack() unless we have used a $accountSwitcher->switchTo is the same method?

tedbow’s picture

I think we cannot use $accountSwitcher->switchBack() unless we have used a $accountSwitcher->switchTo is the same method?

The 2 calls don't have to be in the same because every time we get the @account_switcher service from the container it will be the same instance within the same request. So calling switchBack from anywhere in the request should be safe as long as we have called switchTo somewhere in the request.

abhishek-anand’s picture

Thanks Ted for the clarification. Patch updated.

abhishek-anand’s picture

Status: Needs work » Needs review
tedbow’s picture

Status: Needs review » Needs work
FileSize
3.47 KB
1.17 KB

The patch looks good. I just 3 very minor phpdoc coding standard changes.

I think it would be good if there was a comment in there as far why we are saving this connection. As new developer to the project I am not really sure.

Also the remaining code in the d7 hook_init

$GLOBALS['ultimate_cron_original_session_saving'] = drupal_save_session();
  $GLOBALS['ultimate_cron_original_user'] = $GLOBALS['user'];

  _ultimate_cron_variable_load('cron_last');

Do we not need this anymore? My points in #4 and #6 was just that we shouldn't be using global variable not that we shouldn't be storing the session and user to get them back later.

It seems this was needed in D7 to get the user back later if you search for in the D7 module. Does this not apply anymore?

abhishek-anand’s picture

@tedbow, you suggestion looks good to me, we would not need $GLOBAL as this would be taken care by \Drupal\Core\Session\AccountSwitcher

Also ultimate_cron_variable_load could be implemented by state API, so this is not required as well.

abhishek-anand’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work

I'm not a big fan of having code that runs on every request to handle the rare exception case.

See \Drupal\dblog\Logger\DbLog::log(), while pretty ugly, that only runs when something actually did go wrong.

Unless you use poormanscron, which is not recommended anyway, no normal request will ever need this.

See _ultimate_cron_get_transactional_safe_connection(). We don't we just implement that function so that it creates the connection if it doesn't exist yet when it's actually accessed?

I guess we could also make it a service while we touch it but since the database isn't really a service, that does't matter too much to me.

abhishek-anand’s picture

Status: Needs work » Needs review
FileSize
4.41 KB

Reimplementation as per the suggestion by Berdir.

Berdir’s picture

Status: Needs review » Fixed

Thanks, fixed a typo and committed.

  • Berdir committed 395d406 on 8.x-2.x authored by abhishek-anand
    Issue #2691545 by abhishek-anand, tedbow, Berdir: Port hook_init to...

Status: Fixed » Closed (fixed)

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