Bug #104423
open
Refetch groups recursively following AfterGroupsResolvedEvent dispatching
0%
Description
In the "GroupResolver" method "resolveGroupsForUser", we only fetch groups recursively before the event "AfterGroupdResolvedEvent".
It has the consequence of any groups added afterwards, will not have any subgroups fetched.
$resolvedGroups = $this->fetchGroupsRecursive($originalGroupIds);
$event = $this->eventDispatcher->dispatch(new AfterGroupsResolvedEvent($sourceTable, $resolvedGroups, $originalGroupIds, $userRecord));
We could redo the fetching afterwards, to ensure all groups are loaded recursively
$resolvedGroups = $this->fetchGroupsRecursive($originalGroupIds);
$event = $this->eventDispatcher->dispatch(new AfterGroupsResolvedEvent($sourceTable, $resolvedGroups, $originalGroupIds, $userRecord));
$resolvedGroupsAfterEvent = $this->fetchGroupsRecursive(array_map(fn(array $group) => $group['uid'], $event->getGroups()));
Updated by Garvin Hicking 9 days ago
- Status changed from New to Needs Feedback
I'm wondering if maybe the better place to do this recursively would be to do it inside your event? You can DI inject the GroupResolver to call the method fetchGroupsRecursive on your custom event result?
My reasoning is, that the event might be specifically be used to not include recursive settings. If we would generally add this after the event, no longer could integrators use the event to restrict groups to main groups for example - so we'd actually lose flexibility with this.
What do you think?
Updated by Soren Malling 6 days ago
That is a plausible usecase you describe. In terms of API, the method fetchGroupsRecursive is currently protected, so it can't be called from a DI class.
And, it's important to stress that one should not call resolveGroupsForUser as it would turn into a endless loop :-)
Updated by Garvin Hicking 5 days ago
Ah, of course, I didn't check thoroughly enough.
Then probably the event would need a new "setRecursive" / "isRecursive" property, so that it can be toggled whether the recursive operation should be fired or not after the event.
Would you be interested creating a patch for this?
Updated by Soren Malling 3 days ago
Garvin Hicking wrote in #note-3:
Would you be interested creating a patch for this?
I will come up with something 👌