Change 596853 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Add fallback default edit summary for content model change form.

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

Change 596854 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Ensure string is passed to SpamChecker

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

Note that the patch for injecting the service everywhere (https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/596287/) also fixes this issue

In T252963#6153825, @DannyS712 wrote:

Note that the patch for injecting the service everywhere (https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/596287/) also fixes this issue

How does it fix the issue? Does this commit have an unrelated change that fixes the line causing it to pass null?

In T252963#6161286, @Krinkle wrote:

In T252963#6153825, @DannyS712 wrote:

Note that the patch for injecting the service everywhere (https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/596287/) also fixes this issue

How does it fix the issue? Does this commit have an unrelated change that fixes the line causing it to pass null?

See https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/596287/10/includes/specials/SpecialChangeContentModel.php - only checks when the reason is truthy, and casts to string

The issue can be reproduced at https://commons.wikimedia.org/wiki/Special:ChangeContentModel/Commons:Sandbox.

It was not previously mentioned on this task, but the problem is not about using an empty edit summary. The form cannot even be displayed or opened in the first place. This is a critical difference because it means there is probably not a bug in the code about how the parameters are procesed from form submissions.

Rather, there is likely a problem with how the form is displayed. This is why I have marked this UBN and train block.

The task also did not have a stack trace yet, which I have added now from production. Although it can be captured from a local wiki as well.

From task description

#1 SpecialChangeContentModel.php(150): EditPage::matchSummarySpamRegex(NULL)
#2 htmlform/HTMLFormField.php(315): SpecialChangeContentModel->{closure}(NULL, array, OOUIHTMLForm)
#3 htmlform/HTMLFormField.php(897): HTMLFormField->validate(NULL, array)
#4 includes/htmlform/HTMLFormField.php(606): HTMLFormField->getErrorsRaw(NULL)
#5 htmlform/HTMLForm.php(1696): HTMLFormField->getOOUI(NULL)
#6 htmlform/HTMLForm.php(1292): HTMLForm->displaySection(array, string)

It can be seen here that the null value is coming all the way from HTMLForm->displaySection.

Some questions:

• Why does HTMLForm->displaySection consider the value to be null. Is the default not empty string for text fields?

				$v = array_key_exists( $key, $this->mFieldData )
					? $this->mFieldData[$key]
					: $value->getDefault();

This shows that if the field was submitted, it will use its value. Otherwise the default is used. I was looking at this yesterday and spent some time trying to figure out why the field is missing. However, per the above, we now know that the issue is not with submissions at all. It is with the form display.

And for reasons I don't understand, it seems HTMLTextField has no custom default. Its default is inherited from HTMLFormField – which is null.

• Why is the validator called when displaying the form? The user has not entered anything yet. Does this always happen? Is there a reason for it?

Probably a good reason, but worth checking.

Distinguishing submission from display is not as easy as it sounds because it is possible to set preset values via query parameters (which informs the display, it does not make a submission). For example, https://commons.wikimedia.org/wiki/Special:ChangeContentModel/Commons:Sandbox?reason=example does work today. And it presets the reason field to "example". This could be used as part of a custom workflow where you follow a sequence of steps documented in a page, and then a link can send you here with some contextual reason already entered for convenience.

But.. here comes the confusing part

When opening http://default.web.mw.localhost:8080/mediawiki/index.php?title=Special:ChangeContentModel/Main_Page locally, it fails by default due to the NULL error which means the validation-callback is called on opening (as we know already). Then, I configured the following:

$wgSummarySpamRegex = [ '/bar/' ];

... and opened http://default.web.mw.localhost:8080/mediawiki/index.php?title=Special:ChangeContentModel/Main_Page&reason=bar. To my surprise, this did not show the filter error message about spam. Which means either it only calls it when empty (why?) or it calls it always, but ignores the result?

By placing an exception throw in the callback locally, I found that it does in fact always call the validation callback, but it ignores the result. Its messages are not displayed. instead, the user has the chance to modify the edit summary (or leave as is) and if submitted in a way that is considered spam, they will find out then. This seems fine.

For now, I guess we need to accept that it is normal for HTMLTextField to call the validator on display and accept that is normal for it to have the impossible value of null by default (I call this impossible because form data is always string, null does not make sense.) As such, it would not be weird to check for null in the validation callback, because it is the standard behaviour of HTMLTextField. The latest patch set at for change 596287 does this now.

I suggest we split that out into its own patch and deploy that immediately.

More questions for later:

• Today HTMLTextField calls validators with null. Has this caused problems before? Or are we the first one? How do others avoid this problem?
• Why are values validated during form display? It seems we don't want to show the errors during display, which makes sense from a UX perspective, but then why call them at all? Skipping that could also be a signifant performance improvements as I believe we would generally expect backend validation services for user input to not be involed during GET requests.
• Why is the default of HTMLTextField null? Does stuff rely on this to distinguish between display and submission somehow? Or can we change this to empty string? For comparison, WebRequest::getText defaults to empty string.

Change 598286 had a related patch set uploaded (by Krinkle; owner: DannyS712):
[mediawiki/core@master] SpecialChangeContentModel: Only call spam checks on non-empty reasons

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

Change 598287 had a related patch set uploaded (by Krinkle; owner: DannyS712):
[mediawiki/core@wmf/1.35.0-wmf.32] SpecialChangeContentModel: Only call spam checks on non-empty reasons

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

Change 598286 merged by jenkins-bot:
[mediawiki/core@master] SpecialChangeContentModel: Only call spam checks on non-empty reasons

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

Change 598287 merged by jenkins-bot:
[mediawiki/core@wmf/1.35.0-wmf.32] SpecialChangeContentModel: Only call spam checks on non-empty reasons

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

Mentioned in SAL (#wikimedia-operations) [2020-05-26T03:20:00Z] <tstarling@deploy1001> Synchronized php-1.35.0-wmf.32/includes/specials/SpecialChangeContentModel.php: for UBN T252963 (duration: 01m 07s)

Thank you very much @Krinkle , @tstarling and @DannyS712

Change 596854 abandoned by Krinkle:
Ensure string is passed to SpamChecker

Reason:
No longer needed.

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

Change 596853 abandoned by Krinkle:
SpamChecker: Skip spam regex when input is null or empty string

Reason:
No longer needed.

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