Replace drupal_cron_run() with a Cron service

1. +++ b/core/lib/Drupal/Core/Cron.php
   @@ -0,0 +1,136 @@
   +    $original_user = $GLOBALS['user'];
   +    $GLOBALS['user'] = drupal_anonymous_user();
   

   oh, can we clean this up?

2. +++ b/core/modules/system/lib/Drupal/system/CronController.php
   @@ -22,8 +23,8 @@ class CronController {
   +    // @todo inject this as a service?
   

   ?Yes?

1. +++ b/core/lib/Drupal/Core/Cron.php
   @@ -0,0 +1,136 @@
   +  /**
   +   *
   +   */
   ...
   +  /**
   +   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   +   */
   +  protected $moduleHandler;
   +
   +  /**
   +   * @var \Drupal\Core\Lock\LockBackendInterface
   +   */
   +  protected $lock;
   +
   +  /**
   +   * @var \Drupal\Core\Queue\QueueFactory
   +   */
   +  protected $queueFactory;
   +
   +  /**
   +   * @var \Drupal\Core\KeyValueStore\StateInterface
   +   */
   +  protected $state;
   +
   ...
   +/**
   + * @todo
   + */
   

   We usually try to document a bit more.

2. +++ b/core/lib/Drupal/Core/Cron.php
   @@ -0,0 +1,136 @@
   +    // Force the current user to anonymous to ensure consistent permissions on
   +    // cron runs.
   +    $original_user = $GLOBALS['user'];
   +    $GLOBALS['user'] = drupal_anonymous_user();
   +
   

   It would be great to at least at a todo. In additional we could at least get the current user from the container.

3. +++ b/core/modules/locale/locale.module
   @@ -386,7 +386,7 @@ function locale_themes_disabled($themes) {
   + * @see \Drupal\Core\Cron
   ...
   - * @see drupal_cron_run()
   

   This @see is quite pointless to be honest, this is just hook_cron implementation. if you want to understand more, look into hook_cron.

This is indeed something which have to tackle in a an additional issue (which is not a follow up technically).

Is it worth writing a unit test?

1. +++ b/core/lib/Drupal/Core/Cron.php
   @@ -0,0 +1,146 @@
   +  /**
   +   *
   +   */
   

   Missing param doc blocks

2. +++ b/core/lib/Drupal/Core/Cron.php
   @@ -0,0 +1,146 @@
   +    // @todo This currently does not work, as it will affect the current user
   +    //   being injected into services.
   +    $original_user = $GLOBALS['user'];
   +    $GLOBALS['user'] = drupal_anonymous_user();
   ...
   +    $GLOBALS['user'] = $original_user;
   

   Um, whaaaat? I guess the bottom line should have a @todo as well. Is this what is causing #2108623: Installing via the UI no longer logs in user 1 every time?

We can add this at a later date (I'll add a follow up issue when this is in). How does that sound?

+1!!

+++ b/core/lib/Drupal/Core/Cron.php
@@ -0,0 +1,158 @@
+    // Force the current user to anonymous to ensure consistent permissions on
+    // cron runs.
+    // @todo This currently does not work, as it will affect the current user
+    //   being injected into services.
+    $original_user = $GLOBALS['user'];
+    $GLOBALS['user'] = new UserSession();
...
+    // Restore the user.
+    // @todo This currently does not work, as it will affect the current user
+    //   being injected into services.
+    $GLOBALS['user'] = $original_user;
+    drupal_save_session($original_session_saving);

Just inject the current_user service...?

When we wrote the current_user service I know that people told me that switching the current user during a request forth and back is an edge case. HAHAH

When we wrote the current_user service I know that people told me that switching the current user during a request forth and back is an edge case. HAHAH

Then you were told by the wrong people ;) I know there are use cases like this, an issue to add a function to wrap this has been open since forever. I have no clue how to be able to do this with the current_user service.

The only thing I can think of would be to stop injecting the user and inject the authentication manager and get the user from that, not offering a service for the current user (we can still have Drupal::currentUser()), then we can replace the user there and also offer something like #287292: Add functionality to impersonate a user on the authentication manager. But that would be another non-trivial API change around all this.

current_user service is synchronized, so all you need is a setCurrentUser() method in your object and it will be updated by the DIC.
we just need to make sure that every object that has current_user as a dependency has this method. Then the service would be updated everywhere, no?
Or thats not the problem?

I am not sure i agree

The correct thing to do imo is make Cron service ContainerAware and instead of manipulating the $user global it should do sth like:

$original_user = clone $this->currentUser;
$this->container->set('current_user', new UserSession());
......
$this->container->set('current_user', $original_user);

but a followup is fine with me, because this might open a can of worms

probably though we already have this followup created #2108623: Installing via the UI no longer logs in user 1 every time
i suspect the same with @tim.plunkett

so, lets move on here

Agreed. Already making the cron path a normal route was a win for modules like elysia_cron, as they don't have to ship their own cron.php anymore, this is even better, they can just replace the service and it then all cron executions will use their implemention. Right now, they have to do all kinds of tricks to prevent the default cron being executed.

+1 to RTBC.

Committed 9da7e4c and pushed to 8.x. Thanks!

current_user service is synchronized, so all you need is a setCurrentUser() method in your object and it will be updated by the DIC.

... Well this just works in cases of services, but sadly not on plugins which also uses the accounts all over the place.

ah, yes didnt think of that.
well then we need to make plugin managers do this job, or something like what Berdir suggested in #15

Posted change notice: https://drupal.org/node/2181921

Thanks for the change notice @Les Lim.

Ooo... I only just saw this because of the change notice. I like. As a follow-up, could we refactor the run() method? It's still extremely fugly. (Switching hook_cron to an event would probably be harder to get away with at this point, but something we should still look into doing.)

@Crell: I don't think events is a good match here, I'd rather see plugins. Events would make it hard for modules like Elysia cron/Ultimate cron, that allow to configure each hook implementation differently. But I doubt that's a 8.x task :)