Change 551001 had a related patch set uploaded (by markahershberger; owner: markahershberger):
[mediawiki/extensions/PageForms@master] Ensure null behaves correctly on a mandatory field

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

Is "None" an ok value for a _boo field?

Right, this issue has come up before. You probably saw it in the code, but the current logic is that the "None" value is only removed when the field is mandatory *and* it has a default value. (I think the same approach is used for the "dropdown" input.) The thinking there is that, when a new page is being created, we don't want the form to accidentally dictate a value for that field when the user hasn't consciously selected one. I think the current behavior is that, if it's a mandatory field (with no default value) and the user keeps the field selection at "None", when the user goes to submit the form the JavaScript kicks in and tells them they have to select a value.

This does cause confusion for admins, but I think it's the right approach, because it avoids accidental creation of data. What do you think?

As for your other question: yes, a boolean field can meaningfully have values of "", "true" and "false". The checkbox input doesn't allow it, but I think if you use a dropdown or radiobutton for a boolean field, "None" does show up as an option.

Yaron_Koren <no-reply@phabricator.wikimedia.org> writes:

This does cause confusion for admins, but I think it's the right
approach, because it avoids accidental creation of data. What do you
think?

Right now, I think that if there is a mandatory field, the form is not
complete unless the user has chosen a valid option. Right now, I do not
see "None" as a valid option for a mandatory field -- especially if the
form creator did not specify "None" as an option.

That said, I'm writing tests to ensure that forms get created in a
consistent way. I'm doing this so I can re-submit the changes I made
earlier but were far too large to review--especially since the behavior
of the affected code couldn't be easily checked.

Except for T238397 (this task) and this commit,
there are no changes to the code.

Could you please merge these changes? If you still want to discuss this
change, I can separate the test from this task.

Well, it's true that "None" is not valid, but I believe (and hope!) that the JS prevents the form from being submitted if that invalid value is chosen.

Which changes do you want me to merge?

Changes to merge: the ones tagged radiobutton-tests

If you want to keep the behavior of null on a mandatory field the same
for now, DO NOT merge Ensure null behaves correctly on a mandatory field.

I'm willing to rewrite the test and leave this task open until I've
looked at this issue further and done some more thinking.

Well, it's true that "None" is not valid, but I believe (and hope!)
that the JS prevents the form from being submitted if that invalid
value is chosen.

I disagree strongly with leaving validation up to JS only. I am not
comfortable yet, though, offering an alternative.

So, for applying my rewrite change, I see that I need to update my tests. Let me get that rewrite backported first.

Great - I'm looking forward to checking in the additional tests. Page Forms could really use more tests!

What's the problem with JS validation? That's how mandatory text and textarea inputs are handled...

What's the problem with JS validation? That's how mandatory text and textarea inputs are handled...

Client-side-only validation is wrong. This SO Q&A has a good summary of why:

Never — under any circumstances — trust data from the browser and always validate request data on the server-side.

Of course, JS validation is good for instant feedback and should be used for that. But input has to be checked server-side as well.

But, like I said, I'm not in a position yet to offer any solutions here, so all I can do right now is offer my point of view.

Change 551001 abandoned by markahershberger:
Ensure null behaves correctly on a mandatory field

Reason:
not right now

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

Ok, I've put all the tests into a single, easy to verify commit: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/PageForms/+/550990/

Please +2 that and I'll revisit the issue in this task later.

Great, thank you!! Unit tests such as these are a big step in bringing Page Forms to the next level.

As for the validation thing: this may be a moot discussion, but - it's true that client-side validation is not secure, but I don't think that matters. After all, 99% of the time, users who can edit a page using a form can also edit its text directly - so if a malicious user really wants to put bad data on a page, they can do it anyway. As long as the code works correctly for non-malicious users, I think it's fine. (There may be a use case for needing Page Forms validation to be completely unbreakable, but I haven't heard of it yet.)