Page MenuHomePhabricator

VisualEditor: TitleInputWidget should validate inputs
Closed, ResolvedPublic

Description

We have isValid() functions for similar inputs, but not this one, so it's possible to try to create a redirect to "|" (which Parsoid turns into MediaWiki:Badtitle).


Version: unspecified
Severity: normal

Details

Reference
bz71249

Event Timeline

bzimport raised the priority of this task from to High.
bzimport set Reference to bz71249.

Currently the redirect section of the page settings dialog and the template placeholder page use MWTitleInputWidget.

The template placeholder page can be fixed with a simple change from value === '' to !mw.Title.newFromText( value ) in MWTemplatePlaceholderPage#onTemplateInputChange. It will no longer allow you to add a template with an invalid name. No need for isValid here.

As for the redirect settings, that seems more difficult. I don't think we should just silently ignore the value if it was set to an invalid title... And I don't think we should disable the apply button either.

(In reply to Alex Monk from comment #1)

Currently the redirect section of the page settings dialog and the template
placeholder page use MWTitleInputWidget.

The template placeholder page can be fixed with a simple change from value
=== '' to !mw.Title.newFromText( value ) in
MWTemplatePlaceholderPage#onTemplateInputChange. It will no longer allow you
to add a template with an invalid name. No need for isValid here.

What happens when you use a parser function or a magic word?

As for the redirect settings, that seems more difficult. I don't think we
should just silently ignore the value if it was set to an invalid title...
And I don't think we should disable the apply button either.

We show "invalid title" for link searches; presumably that's evaluated server-side?

(In reply to James Forrester from comment #3)

(In reply to Alex Monk from comment #1)
> Currently the redirect section of the page settings dialog and the template
> placeholder page use MWTitleInputWidget.
>
> The template placeholder page can be fixed with a simple change from value
> === '' to !mw.Title.newFromText( value ) in
> MWTemplatePlaceholderPage#onTemplateInputChange. It will no longer allow you
> to add a template with an invalid name. No need for isValid here.

What happens when you use a parser function or a magic word?

I imagine it would break. So let's not do that.

> As for the redirect settings, that seems more difficult. I don't think we
> should just silently ignore the value if it was set to an invalid title...
> And I don't think we should disable the apply button either.

We show "invalid title" for link searches; presumably that's evaluated
server-side?

That is shown when the page does not seem to exist (based on the search results) and we get a false-y value from mw.Title.newFromText on the given title.

This is scarily close to what I filed bug 72468 about. While filing it I was thinking that an isValid() implementation should also be provided. I'll add that to that bug.

Change 169623 had a related patch set uploaded by SuchetaG:
Introducing isValid() in MWTitileInputWidget

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

Change 169623 merged by jenkins-bot:
Introducing isValid() in MWTitileInputWidget

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

test2 has the same validation that is in betalabs:

  • cannot add Talk: as a template - 'Add template' is disabled
  • cannot add a template or a category with |(a pipe), < and > chars
  • if 'Page settings' redirection has aforementioned symbols - MediaWiki:Badtitletext is displayed. Does it need some improvement?

(In reply to etonkovidova from comment #8)

test2 has the same validation that is in betalabs:

  • cannot add Talk: as a template - 'Add template' is disabled
  • cannot add a template or a category with |(a pipe), < and > chars
  • if 'Page settings' redirection has aforementioned symbols - MediaWiki:Badtitletext is displayed. Does it need some improvement?

That sounds like it covers all our needs.