Allow blocked users to be filtered from UserSelection for users with 'administer users' permission

Looking at the patch, it is likely implemented in the reverse. The default behaviour is to exclude blocked, so the option should probably be "Include blocked".

Tackling test failures, and there is a HTML issue with the form field not closing its <em> tag.

• Tests added.
• Fixed HTML for checkbox.
• Added schema for entity reference selection plugin settings
• Inverted text to be "include blocked", since the current and default behaviour is to exclude blocked.

Interdiff with #2

The initial intent was to allow fields to be configured such that even admins cannot reference blocked accounts...

Reading your patch, it seemed you wanted to retain the existing behaviour somewhat, because the >hasPermission('administer users') snippet was still present.

even admins cannot reference blocked

If this patch is a binary yes/no you can/t reference blocked users then this would be a behaviour change.

The current behaviour could be retained by having the radio preference:

• Allow users with administer users to reference blocked users. (default)
• Allow anyone to reference blocked users.
• Reference blocked users is prohibited.

This issue is confusing

I see the intent of the original patch was to make it so blocked users would always be excluded, regardless of permission. But then this does not cover situations where you want non-admin to reference blocked.

admin = user with `administer permissions`

All these multiple binary/negating conditions are confusing, these are the options I see are available.

Option A

Proposed by this issue.

• UI: options are (boolean, checkbox, 2-states)
  • FALSE: Admin can reference blocked users (default)
  • TRUE: Admin cannot reference blocked users.
• Effect:
  • admin can sometimes reference blocked users
  • non-admin can never reference blocked users

Option B

• UI: options are (radio, 3-states)
  • 0: Only admin can reference blocked (default)
  • -1: Nobody can reference blocked
  • 1: Anybody can reference blocked
• Effect:
  • Sometimes no-one can reference blocked users.
  • Sometimes only admins can reference blocked users.
  • Sometimes everyone can reference blocked users.

Option C

Proposed by #6

• UI: options are (boolean, checkbox, 2-states)
  • FALSE: Non-admin cannot reference blocked users (default)
  • TRUE: Non-admin can reference blocked users
• Effect:
  • Admin can always view blocked users.
  • Non-admin can sometimes reference blocked users.

Submitting a patch based on option A as proposed by this issue.

Patch did not apply so cudnt get an interdiff

1. Test path has changed, so made changes in the new file. Added one more assertion based on the use cases.
2. Ported patch to 8.5.

Hmm, I think the included test is giving a false positive (i.e. it's not actually testing what it needs to) because $handler_settings was refactored to $configuration at some point and so this patch no longer works the way it's supposed to.

I've made a number of modifications to the last patch:

1. Rerolled to work with 8.6
2. Removed redundant test cases.
3. Fixed new test case, which was not hitting any assertions due to missing arguments in $referenceable_tests.
4. Added #states to the "Include anonymous" field to make it clear that if you exclude blocked users, you won't get the anonymous user either.
5. Changed default setting for include_blocked to TRUE, as that is how it functions prior to this feature and we don't want unexpected behavior to be introduced.
6. For a slight performance improvement, the include_blocked check is now performed before checking for the administer users permission, which is more expensive.

Looks like some random testbot issue caused those failures, so retry.

The referenced users are validated in \Drupal\user\Plugin\EntityReferenceSelection\UserSelection::validateReferenceableNewEntities(). This method still validates the user is active based on the permission 'administer users':

if (!$this->currentUser->hasPermission('administer users')) {
  $entities = array_filter($entities, function ($user) {
    /** @var \Drupal\user\UserInterface $user */
    return $user->isActive();
  });
}

This means this patch needs at least two changes:

1. The validation method \Drupal\user\Plugin\EntityReferenceSelection\UserSelection::validateReferenceableNewEntities() needs to be updated to allow for the configuration option 'include blocked'.
2. The test coverage needs to be extended so it actually saves a reference to a blocked user through a form, so it passes all the relevant form validations.

This is also related to #2836245: Unable to reference blocked user on node save in that users that don't have the "Administer users" permission are unable to submit the edit form for nodes created by a now blocked user. Editing a node should not require you to alter the history about who created it.

IMO user reference fields should default to allowing all users, unless overridden with the configuration in the attached patch.

Disregard #25. It looks like that particular bug is fixed in #2791269: Allow saving pre-existing references to inaccessible items

The patch don't apply to Core 10.3.1
The patch in comment #20 is already included in Core 10: https://api.drupal.org/api/drupal/core%21modules%21user%21src%21Plugin%2...

Rerolled patch for 10.3.2

The fix had not been included in core yet!

I applied the rerolled patch in #39, which still applies cleanly even to Drupal 10.3.6, and it worked fine, it just needs some clarification in the field help text imo..

When creating Entity reference fields on Users, this becomes the default:

You would assume from the description that the checked option will make users without the 'administer users' permission able to select blocked users. The opposite is true: the checked option, ie. by default, leaves core behaving like currently (you can't select blocked users unless you have the 'admin users' perm), but if unchecked it hides the blocked users on that field even to roles with the 'aminister users' permission.

So the feature is consistent with the original request, but we can improve the intuitiveness by reverting the label & description:

[ ] Exclude blocked users (unchecked by default - also, leave the "Include the anonymous user." as independent of this option)
Even if this option is not set, only users with the administer users permission may (view and) reference blocked users.

Anyway, I'm not an expert developer so I can't say if technically correct. For example, this contrib module behaviour might be relevant: reference_blocked_users adds a permission to allow referencing blocked users even without the 'administer users' permission, even in the "Authored by" base field. The module worked fine but started producing warning & error after applying patch #20, only after excluding blocked users from the new option (unchecking the current "include" option).

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

Please move this to a MR in order to be properly reviewed

the current patch has incorrect comparison:
the status = 1 condition needs to be added if the include_blocked is false AND the user has no administer users permission
if the include_blocked is true OR the user has administer users permission, then it should not be added
so I re-rolled it for 10.x

11.x already has an "include_blocked" setting on the selection handler, but it defaults to FALSE
that approach works for entity reference fields, where you can configure it,
but it does not work for the "Name (autocomplete)" views exposed filter
so IMHO the MR for 11.x should only contain a change for the default value to TRUE
but first we need to check why it was introduced with default FALSE

I created https://git.drupalcode.org/project/drupal/-/merge_requests/14549 from the patch in #43.

There were some changes pushed but never made into an MR that are different from the patch https://git.drupalcode.org/issue/drupal-2849620/-/compare/main...2849620...

Setting to needs review to see if the MR is all we need to move forwards with this.

As an admin on Drupal.org, adding people for issue credit offering blocked spam accounts is disorienting.

There are test failures that must be resolved.

1. The issue summary now reflects the change only affects users with the 'administer users' permission
2. Fixed test failures

Seems like it already got a review from @joachim