Page MenuHomePhabricator

Page Forms review checklist
Open, Needs TriagePublic

Description

The following issues were found in Page Forms by @Bawolff earlier this year. I'm putting them all in one place for convenience.

  • "PF_CreateForm.js" line 4 - The other options ajax is totally broken on Special:CreateForm if the url has a #target fragment in it, like http://localhost:8080/wiki/Special:CreateForm#foo
  • Parts of this extension don't follow MW code style conventions. This doesn't really matter, and its totally reasonable for a third party developed extension to deviate from MW style conventions. However, if you want it consistent with current MW style conventions, you may want to consider enabling phpcs.
  • Some uses of global state seem to assume only one form related page will be parsed in a given request. This is probably true, but not considered a best practise assumption. It would be better to store such state in the Parser object probably.
  • "PF_FormUtils.php" line 233 cancelLinkHTML - Use of $wgTitle is strongly discouraged by modern MediaWiki conventions. The calling function should pass in a title instead.
  • "PF_FormUtils.php" line 235 cancelLinkHTML - It would probably be better to detect MSIE in javascript, since then there is no chance of cache pollution (In the context of a special page, there should be no caching, so its unlikely to be an issue, but client side detection where possible is better).
  • "PF_FormUtils.php" line 235 cancelLinkHTML - Its preferred in modern MediaWiki to not use Javascript: URLs for Javascript, but instead attach the javascript from a RL module. Javascript: urls would break if we ever manage to adopt Content-Security-Policy (In fairness, unlikely to happen any time soon).
  • "PF_Hooks.php" line 45 - Should use $wgExtensionAssetPath
  • "PF_ParserFunctions.php" line 155 - no guarantee that global state corresponds to right page, should be done via RL.
  • "PF_FormUtils.php" line 54 - maxlength=200. Most MW has moved to using jquery.byteLimit so that its exactly 255 UTF-8 bytes.
  • "PF_FormEdit.php" line 145 - $targetTitle might be null. e.g. Special:FormEdit?form=Foo&target=%20
  • Somewhere (not sure where) {{Special:RunQuery}} is calling the parser recurively when its being transcluded. You should use $wgParser->getFreshParser() in such situations to avoid UNIQ-... tags.
  • [Not clear on the impact] "PF_AutocompleteAPI.php" line 316 - $whereStr = "$baseCargoTable.$baseCargoField = \"$baseValue\""; - The lack of escaping $baseValue seems concerning here. My understanding is that its processed by Cargo as untrusted input so this doesn't represent a security issue, but nonetheless, this seems kind of sketchy.
  • "PF_CreatePageJob.php" line 44 - Should maybe use RequestContext::importScopedSession() instead.
  • "PageForms.js" line 1265 - async: false is bad...
  • Previewing direct from url (e.g. action=edit&preview=yes) a form page does not show the form.

Event Timeline

@Himanshuc3: How is it "working"? Please be way more specific what you've tried and what did happen or not. The task description here states "The other options ajax is totally broken". It is unclear if you tested the AJAX of "Other options" and how exactly.

@Aklapper Sorry. A rookie mistake.

Screen Shot 2018-01-17 at 7.02.42 PM.png (511×1 px, 99 KB)

I don't see where the "other options Ajax" is.
Please point out the mistake.

This comment was removed by Sahajsk.

Change 592613 had a related patch set uploaded (by Sahajsk; owner: Sahajsk):
[mediawiki/extensions/PageForms@master] Update maxlength with byte limit

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

Will update the description after checking the feasibility and solving the issues.

Change 592613 merged by jenkins-bot:
[mediawiki/extensions/PageForms@master] Update maxlength with byte limit

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

This task has been assigned to the same task owner for more than two years. Resetting task assignee due to inactivity, to decrease task cookie-licking and to get a slightly more realistic overview of plans. Please feel free to assign this task to yourself again if you still realistically work or plan to work on this task - it would be welcome!

For tips how to manage individual work in Phabricator (noisy notifications, lists of task, etc.), see https://phabricator.wikimedia.org/T228575#6237124 for available options.
(For the records, two emails were sent to assignee addresses before resetting assignees. See T228575 for more info and for potential feedback. Thanks!)

Change 664646 had a related patch set uploaded (by Yaron Koren; owner: Yaron Koren):
[mediawiki/extensions/PageForms@master] Simplify form "cancel" link, remove IE support

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

Change 664646 merged by jenkins-bot:
[mediawiki/extensions/PageForms@master] Simplify form "cancel" link, remove IE support

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

Change 664647 had a related patch set uploaded (by Yaron Koren; owner: Yaron Koren):
[mediawiki/extensions/PageForms@master] Add trim of form and target name to Special:EditForm

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

Change 664647 merged by jenkins-bot:
[mediawiki/extensions/PageForms@master] Add trim of form and target name to Special:EditForm

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

Change 749881 had a related patch set uploaded (by Yaron Koren; author: Yaron Koren):

[mediawiki/extensions/PageForms@master] Fix CreateForm JS to work with '#' in URL

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

Change 749881 merged by jenkins-bot:

[mediawiki/extensions/PageForms@master] Fix CreateForm JS to work with '#' in URL

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