r78425 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r78424‎ | r78425 | r78426 >
Date:01:17, 15 December 2010
Author:neilk
Status:ok (Comments)
Tags:
Comment:
refactored HTMLForm show() into three methods, in case you want to process a form in some way separate from showing it.
Modified paths:
  • /trunk/phase3/includes/HTMLForm.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/HTMLForm.php
@@ -184,23 +184,26 @@
185185 }
186186
187187 /**
188 - * The here's-one-I-made-earlier option: do the submission if
189 - * posted, or display the form with or without funky valiation
190 - * errors
191 - * @return Bool or Status whether submission was successful.
 188+ * Prepare form for submission
192189 */
193 - function show() {
 190+ function prepareForm() {
194191 # Check if we have the info we need
195192 if ( ! $this->mTitle ) {
196193 throw new MWException( "You must call setTitle() on an HTMLForm" );
197194 }
198195
 196+ // FIXME shouldn't this be closer to displayForm() ?
199197 self::addJS();
200198
201199 # Load data from the request.
202200 $this->loadData();
 201+ }
203202
204 - # Try a submission
 203+ /**
 204+ * Try submitting, with edit token check first
 205+ * @return Status|boolean
 206+ */
 207+ function tryAuthorizedSubmit() {
205208 global $wgUser, $wgRequest;
206209 $editToken = $wgRequest->getVal( 'wpEditToken' );
207210
@@ -208,12 +211,23 @@
209212 if ( $wgUser->matchEditToken( $editToken ) ) {
210213 $result = $this->trySubmit();
211214 }
 215+ return $result;
 216+ }
212217
 218+ /**
 219+ * The here's-one-I-made-earlier option: do the submission if
 220+ * posted, or display the form with or without funky valiation
 221+ * errors
 222+ * @return Bool or Status whether submission was successful.
 223+ */
 224+ function show() {
 225+ $this->prepareForm();
 226+
 227+ $result = $this->tryAuthorizedSubmit();
213228 if ( $result === true || ( $result instanceof Status && $result->isGood() ) ){
214229 return $result;
215230 }
216231
217 - # Display form.
218232 $this->displayForm( $result );
219233 return false;
220234 }
@@ -528,7 +542,6 @@
529543 $this->mSubmitTooltip = $name;
530544 }
531545
532 -
533546 /**
534547 * Set the id for the submit button.
535548 * @param $t String. FIXME: Integrity is *not* validated

Follow-up revisions

RevisionCommit summaryAuthorDate
r84748MFT r78425 (bug 28236)demon15:09, 25 March 2011
r85080MFT r78108, r78179, r78344, r78347, r78350, r78365, r78380, r78425, r78539, r...demon19:09, 31 March 2011

Comments

#Comment by Nikerabbit (talk | contribs)   08:04, 15 December 2010

funky valiation

#Comment by NeilK (talk | contribs)   10:48, 15 December 2010

I don't understand this comment.

I think the design of this class isn't flexible enough, but I also didn't want to subclass and override the handling of wpEditToken myself. So I refactored it a bit. It does exactly what it used to do, only now in three methods.

#Comment by Nikerabbit (talk | contribs)   10:59, 15 December 2010

It's a spelling error you move around. I agree with you that the class is too inflexible to be useful.

#Comment by NeilK (talk | contribs)   11:10, 15 December 2010

Yeah. I had heard that HTMLForm was our new standard for processing forms, so naively I tried to use it. "Now you have two problems."

It has a serious issue of conflating presentation and logic. The change I made mitigates that, but only somewhat.

The other alternative was to validate the edit token and so on myself, which I was doing anyway before I decided to use HTMLForm. Oh well.

#Comment by Bryan (talk | contribs)   14:19, 15 December 2010

It can be useful, but the point you make about separation between presentation and logic is very valid. I would personally recommend doing the presentation by HTMLForm, and handle the logic yourself (as I did in SpecialUpload)).

Status & tagging log