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.
During some work extending this module to meet a client's requirement, it became apparent that the module did not have a way to clean up records for a deleted user. Having coded a function to purge a user's associations for another task, I want to give something back.
/**
* Respond to user deletion in order to clean up Workbench Access
* section association data that would otherwise become orphaned.
*
* @param $account
* The account that has been deleted.
*/
function workbench_access_user_delete($account) {
// Delete the entity's entry from a fictional table of all entities.
/*
db_delete('example_entity')
->condition('type', $entity
->getEntityTypeId())
->condition('id', $entity
->id())
->execute();
*/
//Get Access Schemes
$scheme_storage = \Drupal::entityTypeManager()->getStorage('access_scheme');
$scheme = $scheme_storage->loadMultiple();
foreach ($schemes as $scheme) {
$user_storage = \Drupal::service('workbench_access.user_section_storage');
//load section IDs for this user
$section_ids = $user_storage->getUserSections($scheme, $account, FALSE);
//remove user from all IDs
$user_storage->removeUser($scheme, $account, $section_ids);
}
}
There may be a better way to handle this, but I'll create a patch for testing!
Comment | File | Size | Author |
---|---|---|---|
#24 | 3023344-workbench_access-delete-24.patch | 3.59 KB | agentrickard |
#24 | 3023344-workbench_access-delete-test-only-24.patch | 2.47 KB | agentrickard |
#23 | 3023344-workbench_access-delete-test-only.patch | 2.46 KB | agentrickard |
#18 | user_delete-3023344-18.patch | 3.57 KB | ahebrank |
Comments
Comment #2
alienzed CreditAttribution: alienzed commentedComment #3
agentrickardI looked into this a bit, and the proper way to delete this is to load the SectionAssociation entity and then delete it.
So something like this:
This should load all sections that the user is assigned to and then delete them, since delete() here takes an array. The delete() also handles revisions.
See https://api.drupal.org/api/drupal/core%21includes%21entity.inc/function/...
See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...
Comment #4
alienzed CreditAttribution: alienzed commentedWonderful! Here is the patch, including a small formatting fix
Comment #5
agentrickardNow, of course, we have to write a test.
Comment #6
alienzed CreditAttribution: alienzed commentedWondering if we should write to the log prior to delete:
\Drupal::logger('nafr_authentication')->notice("Deleting user ". $account->id() . " from Workbench Access");
FYI I thought this wasn't working initially but I think CRON needs to be run for deletions to execute. Deletion of the user did not clear the database entries, but after running cron a few times manually the tables did clear.
Comment #7
agentrickardLogging shouldn't be necessary, since the deletion of the user is logged.
Not sure about Cron. That might be worth some research. Could also be a cache issue, but the delete() should handle that.
Comment #8
alienzed CreditAttribution: alienzed commentedGood to know, leaving the logging out then.
As for the test, I'm going a bit outside my experience level with this, but here's a first draft... any/all help appreciated here!
In plain english I've created a new file under tests/src/Functional called DeleteUserTest.php. It was based on NodeFormMenuTest.php and follows this procedure:
Create menu link
Create user
Associate user
Confirm Association
Delete User
Confirm Association is deleted
my apologies if that's super wrong :P
Comment #9
agentrickardWe should probably associate two users, delete one, and ensure that the second one still exists.
I would also do a direct Equals comparison (or Empty) after the deletion. It might not be equal for unrelated reasons, so stricter checking is better.
Otherwise, the test looks solid.
Comment #10
agentrickardComment #11
alienzed CreditAttribution: alienzed commentedSomething like this or do I have too many assertions? I'd appreciate a review on this as I'm too new to Drupal tests be confident that this test is complete/correct.
Comment #12
agentrickardI like this. It's thorough. The checks on individual users might be removed, but there is no harm in having extra assertions like that. What's missing is this bit, to run after the user deletion:
This is the piece that is critical to test: "We had two users and deleted one, now we should have one user."
Comment #13
alienzed CreditAttribution: alienzed commentedredundant tests removed and critical test replaced!
Comment #14
alienzed CreditAttribution: alienzed commentedtrying with the admin user as the second user
Comment #15
larowlanThis looks really good.
As you're not driving around a browser here, this should be a KernelTest (Extend from KernelTestBase, not BrowserTestBase) as it will be an order of magnitude faster. There are some examples in the module you can base it off.
nice!
Comment #16
alienzed CreditAttribution: alienzed commentedThanks! I've made some other improvements, but want to confirm that my test works locally before trying again, so as not to use drupal.org as my primary test environment.
With that said, the previous test failed, to the best of my knowledge, due to the array not being as expected, meaning user deletion failed, I suppose... If anyone sees something obviously wrong with the test, feel free to speak up, if not I'll get it eventually :)
Comment #17
ahebrank CreditAttribution: ahebrank commentedChanging to "bug" since this breaks the list of assigned users on the editor assignment form.
To reproduce:
- set up an access scheme
- make a few users eligible for section assignment
- add the one with lowest UID to a section
- delete that user
- try to add other users to the section and they won't show up
- if you add users with lower UIDs than the deleted user, the entire list should show up as expected but the deleted user will be represented by a blank row
I haven't tried the patch here but will look into it now.
Comment #18
ahebrank CreditAttribution: ahebrank commentedI've updated the patch. I don't think the previous implementation was correct to delete the entire association entity; this one just just unsets the relevant user_id "field" delta on the entity.
I've left the test as a BrowserTest since that has a bunch of needed traits for entity creation and user management that KernelTest does not.
Comment #19
agentrickardComment #20
damontgomery CreditAttribution: damontgomery at Palantir.net commentedI was able to reproduce that deleting a user breaks the section with the current dev release as mentioned in #18. You can no longer add users or view the existing users.
I am using:
- Drupal: 8.7.9
- Workbench Access: 1.x-dev
- Config Split: 1.4
- Palantir.net Drupal Skeleton: https://github.com/palantirnet/drupal-skeleton
- Drush: 8.1.18
- PHPUnit: 6.5.14
Steps to reproduce:
- Enable workbench_access
- Create three users: /admin/people/create
- Edit the permissions for authenticated users and allow, "Allow all members of this role to be assigned to Workbench Access sections"
- Create a Tags term, /admin/structure/taxonomy/manage/tags/overview
- Create a new schema for taxonomy "Tags" and content type "Article", /admin/config/workflow/workbench_access/access_scheme/add
- Add the two example users to the example term section at /admin/config/workflow/workbench_access/example_scheme/sections/1/users
- Delete the first example user, "Delete the account and its content."
- View the list of users in the section, /admin/config/workflow/workbench_access/example_scheme/sections/1/users
- They are all gone, but only the deleted user should be gone
- Try to add the third example user to the section, /admin/config/workflow/workbench_access/example_scheme/sections/1/users
- No users are shown
With the patch in #18, the above works as expected. The PHPUnit test also passed.
I did get a deprecation notice which is a separate issue. I think this is from a base class.
Comment #21
larowlanshould we use $editor->delete() here? I assume user_delete is one of those functions like entity_load that will eventually be deprecated
Comment #22
agentrickardWe should. All functions that depend on https://api.drupal.org/api/drupal/core%21includes%21entity.inc/function/... will be deprecated.
Comment #23
agentrickardTest-only patch.
Comment #24
agentrickardLast patch was malformed. Tests are green locally.
Comment #25
agentrickardComment #26
agentrickardWe would need something similar for Roles, but that can go in another issue.
Comment #27
agentrickardComment #29
agentrickardCommitted.