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.
Problem/Motivation
The configuration option to notify users by role doesn't work for the 'authenticated user' role. This because users can't be fetched with the current lookup implementation as the 'authenticated user' role is an implicit role:
// Roles.
foreach ($notification->getRoleIds() as $role) {
/** @var \Drupal\user\UserInterface[] $role_users */
$role_users = $this->entityTypeManager
->getStorage('user')
->loadByProperties(['roles' => $role]);
foreach ($role_users as $role_user) {
$data['to'][] = $role_user->getEmail();
}
}
Proposed resolution
Either discard the option in the configuration form or alter the lookup to query the authenticated users as recipients.
Comment | File | Size | Author |
---|---|---|---|
#5 | interdiff_3-5.txt | 1.3 KB | baikho |
#5 | content_moderation_notifications-3035410-5.patch | 1.53 KB | baikho |
| |||
#3 | content_moderation_notifications-3035410-3.patch | 1.44 KB | baikho |
|
Comments
Comment #2
Rob Holmes CreditAttribution: Rob Holmes commentedNice catch, thanks. I think we should support the 'Authenticated user option' so we need to add some extra code to handle this situation.
Comment #3
baikhoSee added patch to allow for authenticated user processing.
Comment #4
jhedstromWe should filter out blocked users here I think (
->condition('status', 1)
. Also, I think we want to turn off access checking on the entity query since this could be triggered by users that don't have access to view other users (->accessCheck(FALSE)
). Otherwise this looks great!Comment #5
baikhoThanks for the review @jhedstrom, see updated patch with addressed remarks.
Comment #7
jhedstromThanks!