Page MenuHomePhabricator

HTMLUsersMultiselectField fails validation when exists is true, but required is false
Closed, ResolvedPublicMar 20 2020

Description

Steps to Reproduce

  1. Create a form with a HTMLUsersMultiselectField like the one in T238809
  2. Set exists to true but leave required to false
  3. Submit the form without any input

Actual Results
Validation Error:

does not exist.

Expected Results
Since the form field is not required and the form field is empty, it should pass validation.

Details

Event Timeline

Restricted Application added a subscriber: Aklapper. ยท View Herald TranscriptMar 5 2020, 12:02 AM

Change 577004 had a related patch set uploaded (by Dbarratt; owner: Dbarratt):
[mediawiki/core@master] HTMLUsersMultiselectField fails validation when exists is true

https://gerrit.wikimedia.org/r/577004

This affects HTMLUserTextField and its subclasses, HTMLUsersMultiselectField and HTMLGlobalUserTextField. Of these, HTMLUserTextField is widely used.

Current behaviour, on submitting empty HTMLUserTextField

exists=trueexists=false
required=trueError: '' does not existError: This value is required.
required=falseError: '' does not existNo error

The configs currently have the following meaning:

  • required - the field must be filled
  • exists - the field must be filled and it must be filled with an existing user

(i.e. exists=true takes priority over required=false)

Proposed behaviour, on submitting empty HTMLUserTextField

exists=trueexists=false
required=trueError: '' does not exist*Error: This value is required.
required=falseNo errorNo error

*or required field error

The meaning of these config options would now be:

  • required - the field must be filled
  • exists - if the field is filled, then it must be filled with an existing user

(i.e. required=false takes priority over exists=true)

Why change the behaviour now?

We have a use case in T238809 where an HTMLUsersMultiselectField should not be required, but if it is submitted, the value should be an existing user. (Arguably this should also apply to the widget for blacklisting users in SpecialPreferences, which currently has exists=false.)

It makes sense to be wary of changing the meaning of widely-used configs like this, so I searched for examples using existing=true and required=false. Those in production are:

  • SpecialEmailUser: The widget shows an error if submitted empty (page refresh but no error under the proposed change).
  • NewsletterEditPage: Shows an error if submitted empty (PHP error under the proposed change, because it assumes that, if the form was submitted, then the value contains existing users).

It looks as though both of these should probably be using required=true, in which case they should get an error if submitted empty.

We could make this change if we change the two use-cases, document the breaking change in the release notes, and document on the widget that exists is only checked if the field is not empty. Pinging @matmarex, @Mooeypoo - does that sound OK, or have we missed anything?

Thank you for the thorough analysis, @Tchanders !

I agree in general about the behavior, but I'd change the prioritization of the consideration of both config variables. It doesn't make sense to me for the user to see that empty string is an invalid user if the field is required; it's obvious, and also, not the *actual* problem (the conceptual problem in that case is that you didn't supply anything).

So, I'd make this behavior:

  • For required=true with an empty input string, no matter what exists value is, the message should always say The value is required.
  • For required=true with a non-empty string, the exists config should be taken into account. With exists=true the message should say something like The user must exist or provided username must exist in the system or whatever.
  • For required=false with an empty string, exists value should be ignored. I don't see logically how you'd have a non required value that requires a valid entry. If it's not required, it's not required.

This would also mean that you (as the widget user) can safely use exists=true with a required=false to achieve a case where the form can either have no users provided or a valid user only. Which, to me, sounds like a good behavior.

Does this make sense?

@Mooeypoo Thanks yes, that sounds the same as what's described in T246958#5949448. (Prioritize requires over exists; show errors accordingly.)

My main concern was about changing the behaviour of a long-standing and widely-used widget - if that's ok then let's go ahead with this.

@Mooeypoo Thanks yes, that sounds the same as what's described in T246958#5949448. (Prioritize requires over exists; show errors accordingly.)

Oh, I'm sorry if I misrepresented. I got confused with the table showing different messaging exists=true and exists=false... which is different than the suggestion. Sorry, it's really confusing -- I'm just glad we are on the same page!

My main concern was about changing the behaviour of a long-standing and widely-used widget - if that's ok then let's go ahead with this.

My personal opinion is that this is a good change. I really doubt anyone used these configuration options with the *intent* to have them do what they emit right now -- it sounds to me more plausible that either no one noticed, or no one put too much effort to understand the mistake.

@matmarex what do you think? You know the usage better...

Another option would be to change the default behavior for required on this widget to something like:

'required' => $params['required'] ?? $params['exists'] ?? false

If the consumer doesn't provide an explicit value for required then exists will be used instead (which is the current behavior). This means to get the new behavior you must explicitly set required to false.

@dbarratt I don't think we should make the default for required depend on another config option. Mostly that's because I think it would be more confusing to have required behave differently in this field compared to other fields. I also don't think there's a reason to assume that exists=true and required=false is likely to be a particularly unusual or infeasible combination, once it's working properly - e.g. we already know that Special:Investigate and Special:Preferences would both want that combination.

ARamirez_WMF changed the subtype of this task from "Bug Report" to "Deadline".Mar 18 2020, 5:08 PM
ARamirez_WMF set Due Date to Mar 20 2020, 4:00 AM.

Change 577004 merged by jenkins-bot:
[mediawiki/core@master] Fix HTMLUsersMultiselectField validation when exists is true

https://gerrit.wikimedia.org/r/577004

dom_walden added a subscriber: dom_walden.

NewsletterEditPage and filters on Special:Investigate Compare are tested in T247760 and T247279 resp.

On https://en.wikipedia.beta.wmflabs.org, testing different inputs to the Special:EmailUser form:

InputJSNo-JS
EmptyBrowser prevents submit*Browser prevents submit*
Non-existent"$user does not exist""$user does not exist"
ExistentSubmitsSubmits

* On Firefox, get a popup saying "Please fill out this field."

On my local vagrant (MediaWiki 1.35.0-alpha (5e1e15e)), testing different inputs to the Special:Investigate main form:

InputOutcome
Empty"This value is required."
Non-existent"This value is required."
Existent user/ipSubmits with existent user/IP
Existent + non-existentSubmits with existent user/IP, ignores non-existent