Skip to content

[6.x] Fix default: current Users fieldtype behavior - #15072

Open
joshuablum wants to merge 5 commits into
6.xfrom
fix-users-default-current-user
Open

[6.x] Fix default: current Users fieldtype behavior#15072
joshuablum wants to merge 5 commits into
6.xfrom
fix-users-default-current-user

Conversation

@joshuablum

Copy link
Copy Markdown
Member

The Users fieldtype supports current as a default value which sets the currently logged in user as the default. However, this is only available by editing the blueprint's YAML directly and not an option in the UI. There is also another issue with it right now. If you set default: current in the YAML and then opened the blueprint and field in the CP, the current user was shown but saving would then hardcode that user's id into the blueprint which is obviously not the intended behavior of that setting.

This PR introduces both selecting it via the UI and also fixes the hardcoding of the ID on save.

The default config field is flagged internally with allow_current, which is what tells the picker to treat current as a real, selectable value in this context (and only this context). With that flag set the fieldtype:

  • offers a synthetic Current User option in the dropdown,
  • keeps the value as current instead of resolving it to an id, so it round-trips
    cleanly through the CP, and
  • renders it as a proper "Current User" item when the field is re-opened.

For actual users fields (no allow_current), current continues to resolve to the logged-in user as before, so existing default: current blueprints keep working exactly the same. The resolution logic now also handles current inside an array, matching how augment() already worked.


While working on those fixes I noticed another potential bug: The default picker ignored the field's own max_items config value so you could select (and save) more default users than the field itself allowed.

  • The default picker previously had no max_items of its own, so it was always unbounded. It now reactively mirrors the field's configured max items in the field's settings UI so you can't select more default users than the field permits. It also adds server side validation so you can't circumvent it easily. Otherwise you can allow three users, select three, lower it to allow 2, and still successfully save the blueprint.

It now respects that config:

image

Closes #8796.

@jackmcdade

Copy link
Copy Markdown
Member

Automated review via an internal PR review skill — a human still needs to sanity check this.

The approach here is sound and the preProcess / toItemArray / getIndexItems plumbing looks right. default: current round-trips correctly, and the ?->id() in preProcess quietly fixes the fatal the old User::current()->id() would throw with no authenticated user. A few things worth addressing before merge though.


The "Current User" chip opens a broken edit stack

currentUserOption() returns only id and title, but RelationshipInput explicitly treats an absent editable key as editable:

:editable="canEdit && (item.editable || item.editable === undefined)"

Users has $canEdit = true, and the default picker renders selected items as chips whenever maxItems !== 1 (shouldShowSelectedItems) — the common case, since the parent field usually has no max_items. So the Current User chip renders its title as a clickable edit link plus an "Edit" dropdown item. Clicking either mounts InlineEditForm with itemUrl: this.item.edit_urlundefinedthis.$axios.get(undefined) in InlinePublishForm.getItem(), which GETs the current page, hands HTML to data_get(), and mounts user-publish-form with undefined props.

Compare toItemArray() for a real user, which sets both edit_url and editable. Fix:

private function currentUserOption(): array
{
    return [
        'id' => 'current',
        'title' => __('Current User'),
        'editable' => false,
    ];
}

maxItems can now return a string

if (this.publishContainer?.asConfig && this.handle === 'default') {
    return this.publishContainer.values?.max_items || Infinity;
}

config.max_items arrives from the server as an int, but publishContainer.values.max_items is whatever the live Integer field emitted — and ui/Input/Input.vue emits $event.target.value, a string, even for type="number". While editing (before a reload re-hydrates it as an int):

  • RelationshipInput declares maxItems: { type: Number } and SelectField declares maxSelections: Number, so you get Vue prop-type warnings.
  • shouldShowSelectedItems's this.maxItems === 1 never matches "1", so setting max items to 1 renders chips and a single-select simultaneously.

return Number(this.publishContainer.values?.max_items) || Infinity; should do it.

No coverage for the server-side validation

All five new tests live in UsersTest and exercise the fieldtype only. There's no FieldsControllerTest anywhere in the suite, so the new default max-items rule and its lang string ship untested — including the exact scenario the description calls out as motivation (allow 3, select 3, lower to 2, save). That's the half most likely to regress, since it's a string-keyed rule injected by request type.

Lowering max items can strand unremovable selections

Once the string coercion above is fixed, dropping max items to 1 while two defaults are selected makes shouldShowSelectedItems false (chips hidden) and SelectField's #selected-option slot render nothing (it requires items.length === 1). The picker looks empty, the value still holds two ids, and saving fails with a message about defaults the user can't see or unlink. Worth truncating the value when maxItems drops, or keeping chips visible while over the limit.


Smaller notes:

  • The current option only exists on the unpaginated branch of getIndexItems. Works today because the default picker is a select dropdown and SelectField sends paginate: false, but the option silently vanishes in stack-selector mode. Fine for now given allow_current is internal — just fragile.
  • strtolower() isn't multibyte-safe and the haystack is a translated string. Str::lower() on both sides.
  • Bare ->filter() in preProcess() discards 0 / '0' alongside the nulls you actually want gone. Practically unreachable and augment() already does the same, so it's consistent — but an explicit closure says what you mean.
  • Blueprint editors without view users can't pick Current User. getIndexItems() early-returns collect() when the user can't index users, before prependCurrentUserOption() runs — even though the option is synthetic and exposes nothing.

@jackmcdade

Copy link
Copy Markdown
Member

Follow-up to narrow that list — I went back and diffed my notes against 6.x to separate what this PR actually introduces from what it merely inherits. Two of the smaller notes were noise and one was flat wrong. Sorry for the extra reading.

Retract entirely:

  • strtolower(). I implied it deviates from house style. It doesn't — strtolower appears in 54 files under src/ versus 3 for Str::lower. Ignore it.
  • The view users gate. The early return in getIndexItems() is untouched by this diff and predates the PR. The synthetic option just inherits it. Out of scope here.

Downgrade:

  • Bare ->filter(). Already the pattern in augment() on 6.x, so this is a copied idiom rather than anything new. Take it or leave it.
  • Stranded selections when max items drops. Hiding chips at maxItems === 1 is generic relationship behavior that already affects any entry whose value exceeds a newly-lowered max_items. This PR is the first thing to point the default picker at it, so it's a newly reachable rough edge rather than a bug you introduced. Still worth a thought, but not a blocker.

Unchanged — these are genuinely this PR's:

  • The editable chip. The item.editable === undefined fallback is old, but nothing previously fed it an item array lacking editabletoItemArray() always set it, and invalidItemArray() items are gated separately by item.invalid. currentUserOption() is the first shape to hit that path.
  • String maxItems. 6.x read this.config.max_items, which in a config publish form is the static server-defined config and always an int. Reading live publishContainer.values is what lets a string through.
  • Untested validation. The missing FieldsControllerTest is a pre-existing gap in the suite, but the new rule and lang string are yours.

So: the first item is the one that needs fixing, the second is a one-word Number(), and the third is a judgement call on how much test scaffolding is worth standing up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Users field default current cannot be used through UI

2 participants