r98476 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r98475‎ | r98476 | r98477 >
Date:22:19, 29 September 2011
Author:jpostlethwaite
Status:resolved (Comments)
Tags:fundraising 
Comment:
Added GatewayForm::validateForm() so certain elements can be validated. The old method GatewayForm::fnValidateForm() always validates credit cards.
Modified paths:
  • /branches/fundraising/extensions/DonationInterface/gateway_common/GatewayForm.php (modified) (history)

Diff [purge]

Index: branches/fundraising/extensions/DonationInterface/gateway_common/GatewayForm.php
——@@ -30,6 +30,14 @@
3131 */
3232 public $errors = array( );
3333
 34+ /**
 35+ * The form is assumed to be successful. Errors in the form must set this to
 36+ * false.
 37+ *
 38+ * @var boolean
 39+ */
 40+ public $validateFormResult = true;
 41+
3442 function __construct() {
3543 $me = get_called_class();
3644 parent::__construct( $me );
——@@ -109,6 +117,241 @@
110118 }
111119
112120 /**
 121+ * Checks posted form data for errors and returns array of messages
 122+ *
 123+ * This is update to GatewayForm::fnValidateForm().
 124+ *
 125+ * @param array $data Reference to the data of the form
 126+ * @param array $error Reference to the error messages of the form
 127+ *
 128+ * @return 0|1 Returns 0 on success and 1 on failure
 129+ *
 130+ * @see GatewayForm::fnValidateForm()
 131+ */
 132+ public function validateForm( &$data, &$error, $options = array() ) {
 133+
 134+ extract( $options );
 135+
 136+ // Set which items will be validated
 137+ $address = isset( $address ) ? (boolean) $address : true ;
 138+ $amount = isset( $amount ) ? (boolean) $amount : true ;
 139+ $creditCard = isset( $creditCard ) ? (boolean) $creditCard : false ;
 140+ $email = isset( $email ) ? (boolean) $email : true ;
 141+ $name = isset( $name ) ? (boolean) $name : true ;
 142+
 143+ // These are set in the order they will most likely appear on the form.
 144+
 145+ if ( $name ) {
 146+ $this->validateName( $data, $error );
 147+ }
 148+
 149+ if ( $address ) {
 150+ $this->validateAddress( $data, $error );
 151+ }
 152+
 153+ if ( $amount ) {
 154+ $this->validateAmount( $data, $error );
 155+ }
 156+
 157+ if ( $email ) {
 158+ $this->validateEmail( $data, $error );
 159+ }
 160+
 161+ if ( $creditCard ) {
 162+ $this->validateCreditCard( $data, $error );
 163+ }
 164+
 165+ /*
 166+ * $error_result would return 0 on success, 1 on failure.
 167+ *
 168+ * This is done for backward compatibility.
 169+ */
 170+ return $this->getValidateFormResult() ? 0 : 1 ;
 171+ }
 172+
 173+ /**
 174+ * Validates the address
 175+ *
 176+ * @param array $data Reference to the data of the form
 177+ * @param array $error Reference to the error messages of the form
 178+ *
 179+ * @see GatewayForm::validateForm()
 180+ */
 181+ public function validateAddress( &$data, &$error ) {
 182+
 183+ if ( empty( $data['street'] ) ) {
 184+
 185+ $error['street'] = wfMsg( $this->adapter->getIdentifier() . '_gateway-error-msg-street' );
 186+
 187+ $this->setValidateFormResult( false );
 188+ }
 189+
 190+ if ( empty( $data['city'] ) ) {
 191+
 192+ $error['city'] = wfMsg( $this->adapter->getIdentifier() . '_gateway-error-msg-city' );
 193+
 194+ $this->setValidateFormResult( false );
 195+ }
 196+
 197+ if ( empty( $data['state'] ) ) {
 198+
 199+ $error['state'] = wfMsg( $this->adapter->getIdentifier() . '_gateway-error-msg-state' );
 200+
 201+ $this->setValidateFormResult( false );
 202+ }
 203+
 204+ if ( empty( $data['zip'] ) ) {
 205+
 206+ $error['zip'] = wfMsg( $this->adapter->getIdentifier() . '_gateway-error-msg-zip' );
 207+
 208+ $this->setValidateFormResult( false );
 209+ }
 210+ }
 211+
 212+ /**
 213+ * Validates the amount contributed
 214+ *
 215+ * @param array $data Reference to the data of the form
 216+ * @param array $error Reference to the error messages of the form
 217+ *
 218+ * @see GatewayForm::validateForm()
 219+ */
 220+ public function validateAmount( &$data, &$error ) {
 221+
 222+ if ( empty( $data['amount'] ) ) {
 223+
 224+ $error['amount'] = wfMsg( $this->adapter->getIdentifier() . '_gateway-error-msg-amount' );
 225+
 226+ $this->setValidateFormResult( false );
 227+ }
 228+
 229+ // check amount
 230+ $priceFloor = $this->adapter->getGlobal( 'PriceFloor' );
 231+ $priceCeiling = $this->adapter->getGlobal( 'PriceCeiling' );
 232+ if ( !preg_match( '/^\d+(\.(\d+)?)?$/', $data['amount'] ) ||
 233+ ( ( float ) $this->convert_to_usd( $data['currency'], $data['amount'] ) < ( float ) $priceFloor ||
 234+ ( float ) $this->convert_to_usd( $data['currency'], $data['amount'] ) > ( float ) $priceCeiling ) ) {
 235+
 236+ $error['invalidamount'] = wfMsg( $this->adapter->getIdentifier() . '_gateway-error-msg-invalid-amount' );
 237+
 238+ $this->setValidateFormResult( false );
 239+ }
 240+ }
 241+
 242+ /**
 243+ * Validates a credit card
 244+ *
 245+ * @param array $data Reference to the data of the form
 246+ * @param array $error Reference to the error messages of the form
 247+ *
 248+ * @see GatewayForm::validateForm()
 249+ */
 250+ public function validateCreditCard( &$data, &$error ) {
 251+
 252+ if ( empty( $data['card_num'] ) ) {
 253+
 254+ $error['card_num'] = wfMsg( $this->adapter->getIdentifier() . '_gateway-error-msg-card_num' );
 255+
 256+ $this->setValidateFormResult( false );
 257+ }
 258+
 259+ if ( empty( $data['cvv'] ) ) {
 260+
 261+ $error['cvv'] = wfMsg( $this->adapter->getIdentifier() . '_gateway-error-msg-cvv' );
 262+
 263+ $this->setValidateFormResult( false );
 264+ }
 265+
 266+ if ( empty( $data['expiration'] ) ) {
 267+
 268+ $error['expiration'] = wfMsg( $this->adapter->getIdentifier() . '_gateway-error-msg-expiration' );
 269+
 270+ $this->setValidateFormResult( false );
 271+ }
 272+
 273+ // validate that credit card number entered is correct and set the card type
 274+ if ( preg_match( '/^3[47][0-9]{13}$/', $data['card_num'] ) ) { // american express
 275+
 276+ $data['card'] = 'american';
 277+
 278+ } elseif ( preg_match( '/^5[1-5][0-9]{14}$/', $data['card_num'] ) ) { // mastercard
 279+
 280+ $data['card'] = 'mastercard';
 281+
 282+ } elseif ( preg_match( '/^4[0-9]{12}(?:[0-9]{3})?$/', $data['card_num'] ) ) {// visa
 283+
 284+ $data['card'] = 'visa';
 285+
 286+ } elseif ( preg_match( '/^6(?:011|5[0-9]{2})[0-9]{12}$/', $data['card_num'] ) ) { // discover
 287+
 288+ $data['card'] = 'discover';
 289+
 290+ } else { // an invalid credit card number was entered
 291+
 292+ $error[ 'card_num' ] = wfMsg( $this->adapter->getIdentifier() . '_gateway-error-msg-card-num' );
 293+
 294+ $this->setValidateFormResult( false );
 295+ }
 296+ }
 297+
 298+ /**
 299+ * Validates an email address.
 300+ *
 301+ * @todo
 302+ * - is this a bug? isEmail -> email :: This is fixed in this method.
 303+ *
 304+ *
 305+ * @param array $data Reference to the data of the form
 306+ * @param array $error Reference to the error messages of the form
 307+ *
 308+ * @see GatewayForm::validateForm()
 309+ */
 310+ public function validateEmail( &$data, &$error ) {
 311+
 312+ if ( empty( $data['email'] ) ) {
 313+
 314+ $error['email'] = "**" . wfMsg( $gateway_identifier . '_gateway-error-emailAdd' ) . "**<br />";
 315+
 316+ $this->setValidateFormResult( false );
 317+ }
 318+
 319+// // is email address valid?
 320+// $isEmail = User::isValidEmailAddr( $data['emailAdd'] );
 321+//
 322+// // create error message (supercedes empty field message)
 323+// if ( !$isEmail ) {
 324+// $error['emailAdd'] = wfMsg( $this->adapter->getIdentifier() . '_gateway-error-msg-email' );
 325+//
 326+// $this->setValidateFormResult( false );
 327+// }
 328+ }
 329+
 330+ /**
 331+ * Validates the name
 332+ *
 333+ * @param array $data Reference to the data of the form
 334+ * @param array $error Reference to the error messages of the form
 335+ *
 336+ * @see GatewayForm::validateForm()
 337+ */
 338+ public function validateName( &$data, &$error ) {
 339+
 340+ if ( empty( $data['fname'] ) ) {
 341+
 342+ $error['fname'] = wfMsg( $this->adapter->getIdentifier() . '_gateway-error-msg-fname' );
 343+
 344+ $this->setValidateFormResult( false );
 345+ }
 346+
 347+ if ( empty( $data['lname'] ) ) {
 348+
 349+ $error['lname'] = wfMsg( $this->adapter->getIdentifier() . '_gateway-error-msg-lname' );
 350+
 351+ $this->setValidateFormResult( false );
 352+ }
 353+ }
 354+
 355+ /**
113356 * Build and display form to user
114357 *
115358 * @param $data Array: array of posted user input
——@@ -325,6 +568,26 @@
326569 $wgOut->redirect( $this->adapter->getPaypalRedirectURL() );
327570 }
328571
 572+ /**
 573+ * Get validate form result
 574+ *
 575+ * @return boolean
 576+ */
 577+ public function getValidateFormResult() {
 578+
 579+ return (boolean) $this->validateFormResult;
 580+ }
 581+
 582+ /**
 583+ * Set validate form result
 584+ *
 585+ * @param boolean $validateFormResult
 586+ */
 587+ public function setValidateFormResult( $validateFormResult ) {
 588+
 589+ $this->validateFormResult = empty( $validateFormResult ) ? false : (boolean) $validateFormResult;
 590+ }
 591+
329592 }
330593
331594 //end of GatewayForm class definiton
\ No newline at end of file

Follow-up revisions

RevisionCommit summaryAuthorDate
r98483Added documentation on $options parameter.jpostlethwaite22:51, 29 September 2011
r98486Enabled User::isValidEmailAddr() validator.jpostlethwaite23:06, 29 September 2011
r98915Removed GatewayForm::fnValidateForm(). GatewayForm::validateForm() is now the...jpostlethwaite20:58, 4 October 2011

Comments

#Comment by Awjrichards (talk | contribs)   22:43, 29 September 2011

At quick glance this looks good. In: 

public function validateForm( &$data, &$error, $options = array() )

can you explain $options in the documentation? I assume this is where you define the fields to be validated, but especially since you're using extract( $options ), it would be helpful to make that explicit.

#Comment by Awjrichards (talk | contribs)   22:50, 29 September 2011

Also, in 

$address	= isset( $address )		? (boolean) $address	: true ;
+		$amount		= isset( $amount )		? (boolean) $amount		: true ;
+		$creditCard	= isset( $creditCard )	? (boolean) $creditCard	: false ;
+		$email		= isset( $email )		? (boolean) $email		: true ;
+		$name		= isset( $name )		? (boolean) $name		: true ;

if I am reading this correctly, you are essentially hardcoding required fields here. Perhaps it would be more flexible to pre-populate the default $options array with things that will generally be required. As it is now, if you were to define $options = array('name');, it would still try to validate 'amount'. If I'm not reading this correctly, then consider rewriting this section for clarity, or providing some explicit comments on what it does.

#Comment by Awjrichards (talk | contribs)   22:55, 29 September 2011

You might consider reverting back to using User::isValidEmailAddr(). It used to be kind of a lame check (I think it just looked for an '@' and a '.' in the string...), but now it actually follows the HTML5 spec (http://svn.wikimedia.org/doc/classUser.html#a9a37902b64b64299add7627596b6d093)

#Comment by Jpostlethwaite (talk | contribs)   23:08, 29 September 2011

Documentation was added for $options in r98483.

#Comment by Jpostlethwaite (talk | contribs)   23:10, 29 September 2011

I enabled the validator User::isValidEmailAddr() in r98486.

#Comment by Jpostlethwaite (talk | contribs)   23:27, 29 September 2011

All items have been addressed. Please verify.

#Comment by Awjrichards (talk | contribs)   20:55, 30 September 2011

validateForm() also seems to replace fnValidateForm(). The only place currently referencing fnValidateForm() is in payflowpro_gateway.body.php. We may as well wipeo ut fnValidatForm() and use what you've done here, it's a hell of a lot better than the crufty old fnValidateForm()!

#Comment by Jpostlethwaite (talk | contribs)   21:00, 4 October 2011

Removed GatewayForm::fnValidateForm(). GatewayForm::validateForm() is now the preferred method to validate forms: r98915

Status & tagging log