Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#13 | hook_init-2691545-13.patch | 4.41 KB | abhishek-anand |
| |||
#9 | interdiff.txt | 1.17 KB | tedbow |
#9 | hook_init-2691545-9.patch | 3.47 KB | tedbow |
| |||
#7 | interdiff.txt | 555 bytes | abhishek-anand |
#7 | hook_init-2691545-7.patch | 3.47 KB | abhishek-anand |
|
Comments
Comment #2
abhishek-anand CreditAttribution: abhishek-anand at Acquia commentedThis will have a conflict with ISSUE #2688313
Comment #3
abhishek-anand CreditAttribution: abhishek-anand at Acquia commentedComment #4
tedbow@abhishek-anand sorry getting familiar with this module
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?
Comment #5
abhishek-anand CreditAttribution: abhishek-anand at Acquia commentedHi 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
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?
Comment #6
tedbowThe 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.
Comment #7
abhishek-anand CreditAttribution: abhishek-anand at Acquia commentedThanks Ted for the clarification. Patch updated.
Comment #8
abhishek-anand CreditAttribution: abhishek-anand at Acquia commentedComment #9
tedbowThe 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
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?
Comment #10
abhishek-anand CreditAttribution: abhishek-anand at Acquia commented@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.
Comment #11
abhishek-anand CreditAttribution: abhishek-anand at Acquia commentedComment #12
BerdirI'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.
Comment #13
abhishek-anand CreditAttribution: abhishek-anand at Acquia commentedReimplementation as per the suggestion by Berdir.
Comment #14
BerdirThanks, fixed a typo and committed.