A Member's GridField and a delete button will make a lot of duplicate queries

[lekoala]

Module version(s) affected

5.x

Description

I just noticed a lot of duplicate queries on a Member's GridField. This is due to the canDelete checks being made that will always check to see if a non admin member can delete an admin

silverstripe-framework/src/Security/Member.php

Lines 1559 to 1566 in e7cccf6

	// HACK: if you want to delete a member, you have to be a member yourself.
	// this is a hack because what this should do is to stop a user
	// deleting a member who has more privileges (e.g. a non-Admin deleting an Admin)
	if (Permission::checkMember($this, 'ADMIN')) {
	if (!Permission::checkMember($member, 'ADMIN')) {
	return false;
	}
	}

note that the comment is not accurate, because that's in fact what its doing : checking if a non admin tries to delete an admin

This is the current situation. 68 queries + a lot of duplicate Permission::groupList calls.

this can be mitigated easily by checking before hand if the user is an admin. if so, the hack is not necessary. This reduces the number of queries to 17

This is not super slow as such (reducing queries from 68 to 17 is great, but it doesn't result in a huge speedup) so this seems low priority.

How to reproduce

• Have a GridField with a Member's DataList
• Check queries being made due to canDelete checks if you have a GridFieldDeleteAction

Possible Solution

• Disabling the hack for admins helps, to a certain degree. It doesn't help for regular users
• I'm not sure the hack belongs there in the first place. It could be removed entirely. Security checks like this could be enforced at orm level in onBeforeDelete that can throw an exception
• Better caching could also mitigate the issue. Currently, the Permission::groupList is called multiple times. Improving this would limit the extra costs of checking each user permissions.

Additional Context

No response

Validations

• Check that there isn't already an issue that reports the same bug
• Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

[lekoala]

Note for others: removing the GridFieldDeleteAction works well in th meantime
I'm not sure it makes a lot of sense anyway to delete things like a Member record from a GridField list

Simply add this:

            $config = $config->removeComponentsByType(GridFieldDeleteAction::class);

[GuySartorelli]

Note that while this isn't optimal behaviour, it's also not really a bug. Just sub-optimal code that can be improved.

[lekoala]

Note that while this isn't optimal behaviour, it's also not really a bug. Just sub-optimal code that can be improved.

yes, i fully agree

and it only happens when you have a GridFieldDeleteAction (or any piece of code that calls canDelete)