Move user cancellation functions to a service

Interestingly despite the comment in \user_cancel() claiming third parties (via hooks) can modify the cancel batch...

// Allow modules to add further sets to this batch.

...they do not get an opportunity to do cancellation in the batch. Which seems to be the case since the original patch in #8: Let users cancel their accounts → . It seems the batching in this system is not particularly useful.

I wonder if it's useful to turn this system into batching or remove it entirely.

Testing how things work without batch. I imagine the tests specifically looking for batch UI to appear in browser will fail, of course.

Alright so fails show instead of the original batch being passed around, functions like node_mass_update() are creating extra batches.

Yeah, the user module sets up a batch. It then call the cancel hooks who add their own batch sets if they want to and then after calling all the hooks user module adds its own batch set and then batch API black magic kicks in to execute all of them till it hits _user_cancel_session_regenerate.

• Operation is largely the same, introducing a new service with user cancellation methods suitable for programmatic execution or for spinning up a set of batches to run on form submission.
• New cancellation service.
• New user logger service using factory.
• Converted various cancellation-method string to interface constants.
• Injected new service on cancellation forms.
• Deprecated old global functions.
• Removed usage of old functions.
• Changed callback so it takes a UID, then loads a user based on that UID, instead of when using batch set, having to serialise an entire user entity. A new log message is output if the user is deleted when the cancellation callback runs.
• Converted batch creation to new BatchBuilder
• Converted batch callbacks to PHP callbacks (array of class + method).
• Callbacks are marked as @internal, in line with older underscore-prefixed _user_cancel method. The callbacks on service are not on the interface.
• Callbacks call a protected method, the callbacks and protected methods themselves are considered implementation details, and are not on the interface.
• New $silent param on callbacks, to suppress when cancellation is not triggered via UI, in which case messenger messages are not used.
• Various protected proxy methods included to make testing easier.
• The act of blocking and cancelling (do* methods) are separate methods for readability and possible extension.
• Renamed occurrences of $edit to $options, and removed mentions of form values.
• Add notes so options for progressive method that the options need to be serializable.
• Tests for new cancelUser method.
• Tests exist for testing cancellation via UI / Batch API UI.

Work is also available as a Github branch for reviews.

The last submitted patch, 8: 3079085-user-cancellation-service.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Changed callback so it takes a UID, then loads a user based on that UID, instead of when using batch set, having to serialise an entire user entity.

This is a potential BC break for any implementation of hook_batch_alter interacting with this batch.

+++ b/core/modules/user/src/UserCancellation.php
@@ -0,0 +1,338 @@
+    return \_user_mail_notify($operation, $user);
...
+    return \batch_get();
...
+    \batch_set($batchDefinition);
...
+    return \batch_process();

We don't add \ prefix to global methods in core.

Changed callback so it takes a UID, then loads a user based on that UID, instead of when using batch set, having to serialise an entire user entity.

This is a potential BC break for any implementation of hook_batch_alter interacting with this batch.

Batches are very internal-y, not sure how we handle backwards compatibility for them. I'll leave this up to maintainer to decide, but I'd imagine not serialising a user entity to be a significant advantage. Back when the code was originally written (2009), user objects were much simpler.

We don't add \ prefix to global methods in core.

Fair enough. Patch addresses request.

Thanks, for addressing the feedback.

1. +++ b/core/modules/user/src/UserCancellation.php
   @@ -0,0 +1,338 @@
   +   * @param bool $silent
   +   *   Whether messages should be emitted.
   

   Drupal only allow UI deletion. CLI app should collect messages if the want to show them or they can ignore them as well. IMO, this new addition is not needed.

2. +++ b/core/modules/user/user.module
   @@ -690,51 +691,16 @@ function user_pass_rehash(UserInterface $account, $timestamp) {
   +  @trigger_error('\user_cancel() is deprecated in Drupal 8.8.x and will be removed before Drupal 9.0.0.', E_USER_DEPRECATED);
   
   @@ -752,43 +718,15 @@ function user_cancel($edit, $uid, $method) {
   +  @trigger_error('\_user_cancel() is deprecated in Drupal 8.8.x and will be removed before Drupal 9.0.0.', E_USER_DEPRECATED);
   
   @@ -797,8 +735,12 @@ function _user_cancel($edit, $account, $method) {
   +  @trigger_error('\_user_cancel_session_regenerate() is deprecated in Drupal 8.8.x and will be removed before Drupal 9.0.0.', E_USER_DEPRECATED);
   

   Some more \ prefixes.

Additionally to 15.2 deprecation message should use new format https://www.drupal.org/core/deprecation#how-function →

Thanks for the review!

Drupal only allow UI deletion. CLI app should collect messages if the want to show them or they can ignore them as well. IMO, this new addition is not needed.

The service specifically provides a cancelUser method, which is meant to be invoked programmatically. As a developer I expect that calling this method will not emit a series of messages I have no control over, the same as when calling most other services.

Keep in mind that log messages are always emitted, so in the use case of CLI, you can capture and re-route logger output as usual, for eventual output to IO if desirable.

Some more \ prefixes.

Additionally to 15.2 deprecation message should use new format https://www.drupal.org/core/deprecation#how-function →

Updated deprecation messages, and reformatted according to #3024461: Adopt consistent deprecation format for core and contrib deprecation messages → .

-

Also fixed up deprecated _user_cancel_session_regenerate() function not calling new static callback.

Drupal 8.9.0-beta1 → was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

Had a quick scan and had some initial feedback:

1. +++ b/core/modules/user/src/UserCancellation.php
   @@ -0,0 +1,338 @@
   +class UserCancellation implements UserCancellationInterface {
   

   I find using verbs for interface names makes it clearer on what it does (and nouns for value objects). 'UserCancellerInterface' maybe?

   e.g.

   $userCanceller->cancelUser()

2. +++ b/core/modules/user/src/UserCancellationInterface.php
   @@ -0,0 +1,67 @@
   +  public function progressiveUserCancellation(UserInterface $user, $method, array $options = []);
   

   I think the naming should line up more with cancelUser(). Perhaps cancelUserProgressive()?

3. +++ b/core/modules/user/tests/src/Kernel/UserCancellationTest.php
   @@ -0,0 +1,350 @@
   +    $this->user->expects($this->any())
   

   I prefer prophecy but there is no core policy (as far as I know) on this.

There's another approach in ✨ Provide a Entity Handler for user cancelation Needs work

Probably we could close this one

I've transferred credit, along with the patch, to the other issue. Closing as a duplicate.

Probably we could close this one

Since the other issue has committer buy-in on its approach, I agree. I'm going to take the initiative on this and close this out and transfer credit. I hope I'm not stepping on any toes by doing that. (Apologies if I am; my intentions are pure.)

I think we also want to look carefully through this nice patch and merge as much of the good work as we can into the other patch. I will re-post the latest patch into that issue to facilitate that.

Reverted changes made in #25.