Page MenuHomePhabricator

Prefilling the protection form does not work as expected
Closed, ResolvedPublic

Description

Here's a list of the issues found so far after the protection form has been converted to OOUI (T235424):

  • Prefilling the mwProtect-reason input field leaves the field blank.
  • Prefilling the mwProtect-edit-expires input field also populates the mwProtect-reason input field with the same value.
  • Prefilling the mwProtect-level-move select field also checks the mwProtect-cascade checkbox.
  • Prefilling any field also checks the mwProtectUnchained checkbox.

None of this should happen.

Developer notes

Previously it was possible to prefill all fields in the form using query string parameters

e.g.
http://localhost:8888/w/index.php?title=Map_test&action=protect&mwProtectExpirySelection-move=othertime&mwProtect-level-edit=sysop&wpProtectExpirySelection-edit=othertime&mwProtect-expiry-edit=5%20weeks&mwProtect-level-move=sysop&wpProtectExpirySelection-move=1%20month&mwProtect-cascade=1&mwProtect-reason=hello&mwProtectWatch=

resulted in

Screen Shot 2021-02-17 at 8.29.16 AM.png (1×1 px, 154 KB)

now:

Screen Shot 2021-02-17 at 8.29.54 AM.png (1×1 px, 131 KB)

QA

On a wiki where you have permission privileges

  • On URL "other reason" should be hello and cascade should be checked
  • On URL "cascade" should NOT be checked
  • On URL the reason field should be prefilled with "hello"
  • On URL Unlock further protect options should not be checked

Event Timeline

Daimona subscribed.

I think most (all?) of these bugs are caused by the form using sections for some fields, but not for others. You can reproduce similar bugs with the following:

$this->mOut->enableOOUI();
$fields = [
	[
		'type' => 'text',
		'default' => 'a',
		'section' => 'first',
	],
	[
		'type' => 'text',
		'default' => $this->mRequest->getText( 'mwProtect-reason' ),
		//'section' => 'second' // Uncomment this line to fix
	]
];
HTMLForm::factory( 'ooui', $fields, $this->mContext )->showAlways();

If the second field doesn't have a section, you'll see that the default for the first field is also used for the second field, and the GET value is ignored.

I'm not sure if ProtectionForm is doing everything correctly, but I wouldn't be surprised to find out that this is a bug within HTMLForm itself. I remember facing some issues a few weeks ago while switching some forms in AbuseFilter to use HTMLForm: I had to use a form with a section, and not adding a section to each field would cause bizarre issues (can't recall which ones exactly).

Jdlrobson subscribed.

Untagging HTML Form as OOUI is being used here now, and it likely works slightly different.

Krinkle subscribed.

MediaWiki hasn't transitioned from HTMLForm to OOUI, its transitioned from HTMLForm using Html::element, to HTMLForm using OOUI. All the form-related logic remains exactly where it always has been. OOUI acts as a component library here, it provides classes elements to configure and render as HTML.

It's theoretically possible that a bug there could in some element confguration cause parts to misbehave, but I don't think its productive to discard issues always to the lowest/least probable level. Its significantly more likely, and our past experience confirms, that this is an issue in the Special page and its HTMLForm logic – as determined by @Daimona. Unless and until investigation shows clear sign to follow the issue elsewhere.

Change 664920 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] field descriptors in HTMLForm must have keys

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

Its significantly more likely, and our past experience confirms, that this is an issue in the Special page and its HTMLForm logic

I had forgot that page protection is counter-intuitively not a special page.

I exhausted my timebox and haven't got time to explore further but I found a fix in the protection form, which was what I suspected. It seems HTMLForm counterintuitively requires keys for its descriptor fields for the prefilling to work and doesn't validate what it's given.

@matmarex do you have any ideas why? This could bite us when we migrate other forms, so we should also add some kind of validation safeguard or support for flat arrays once this requirement is better understood.

I don't know why, but it's documented on HTMLForm.php ("The constructor input is an associative array of $fieldname => $info"), so it seems intentional and not a bug to me.

IIRC the field names used for the form field "name" attributes, and URL parameters, so they would have to be required for that? I'm surprised that this form worked at all.

I don't know why, but it's documented on HTMLForm.php ("The constructor input is an associative array of $fieldname => $info"), so it seems intentional and not a bug to me.

It does seem intentional, but I think it shuold validate the input, rather than exploding on non-associative arrays.

IIRC the field names used for the form field "name" attributes, and URL parameters, so they would have to be required for that? I'm surprised that this form worked at all.

AFAICS, every field in ProtectionForm is explicitly setting a name, which is why this form sort of worked.

Change 665173 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@wmf/1.36.0-wmf.30] field descriptors in HTMLForm must have keys

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

Change 665174 had a related patch set uploaded (by Urbanecm; owner: Jdlrobson):
[mediawiki/core@wmf/1.36.0-wmf.31] field descriptors in HTMLForm must have keys

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

Change 664920 merged by jenkins-bot:
[mediawiki/core@master] field descriptors in HTMLForm must have keys

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

Change 665173 merged by jenkins-bot:
[mediawiki/core@wmf/1.36.0-wmf.30] field descriptors in HTMLForm must have keys

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

Change 665174 merged by jenkins-bot:
[mediawiki/core@wmf/1.36.0-wmf.31] field descriptors in HTMLForm must have keys

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

Mentioned in SAL (#wikimedia-operations) [2021-02-19T01:02:06Z] <urbanecm@deploy1001> Synchronized php-1.36.0-wmf.31/includes/ProtectionForm.php: rMW2487c253b090: field descriptors in HTMLForm must have keys (T275018; T274980) (duration: 01m 10s)

Mentioned in SAL (#wikimedia-operations) [2021-02-19T01:03:40Z] <urbanecm@deploy1001> Synchronized php-1.36.0-wmf.30/includes/ProtectionForm.php: rMWd305308a5d46: field descriptors in HTMLForm must have keys (T275018; T274980) (duration: 01m 08s)

Jdlrobson closed this task as Resolved.EditedFeb 19 2021, 1:05 AM
Jdlrobson claimed this task.

This should be fixed now and backported to all wikis. Please open a new ticket if any follow ups are needed.