Awight has uploaded a new change for review.
https://gerrit.wikimedia.org/r/281084
Change subject: [WIP] Explore encapsulating validation along with
transformations
......................................................................
[WIP] Explore encapsulating validation along with transformations
I'm not sure which is more egregious, but this approach gives us several
easy ways to modify the validations, even subclassing an existing validation.
Change-Id: I011f094981510accdb7fb66d3b4292f6c9ae4de9
---
M gateway_common/DataValidator.php
M gateway_common/DonationData.php
M gateway_common/FiscalNumber.php
A gateway_common/ValidationHelper.php
M gateway_common/gateway.adapter.php
M tests/DataValidatorTest.php
6 files changed, 103 insertions(+), 93 deletions(-)
git pull
ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/DonationInterface
refs/changes/84/281084/1
diff --git a/gateway_common/DataValidator.php b/gateway_common/DataValidator.php
index a6e2457..100b380 100644
--- a/gateway_common/DataValidator.php
+++ b/gateway_common/DataValidator.php
@@ -13,13 +13,6 @@
*/
class DataValidator {
- // because of the hacky way we test for translation existence in
getErrorMessage,
- // we need to explicitly specify which fields we want to try to get
country-
- // specific names for.
- protected static $countrySpecificFields = array(
- 'fiscal_number',
- );
-
/**
* getErrorToken, intended to be used by classes that exist relatively
close
* to the form classes, returns the error token (defined on the forms)
that
@@ -43,7 +36,6 @@
case 'card_num':
case 'card_type':
case 'cvv':
- case 'fiscal_number':
case 'fname':
case 'lname':
case 'city':
@@ -62,6 +54,7 @@
/**
* getEmptyErrorArray
+ * @deprecated
* This only exists anymore, to make badly-coded forms happy when they
start
* pulling keys all over the place without checking to see if they're
set or
* not.
@@ -120,7 +113,7 @@
$error_message_field_key = 'donate_interface-error-msg-' .
$message_field;
- if ( $country !== null && in_array( $message_field,
self::$countrySpecificFields ) ) {
+ if ( $country !== null ) {
$translated_field_name =
MessageUtils::getCountrySpecificMessage( $error_message_field_key, $country,
$language );
} else if ( WmfFramework::messageExists(
$error_message_field_key, $language ) ) {
$translated_field_name = WmfFramework::formatMessage(
$error_message_field_key );
@@ -181,17 +174,13 @@
* Run all the validation rules we have defined against a (hopefully
* normalized) DonationInterface data set.
* @param GatewayType $gateway
- * @param array $data The DonationInterface data set, or a subset
thereof.
- * @param array $check_not_empty An array of fields to do empty
validation
- * on. If this is not populated, no fields will throw errors for being
empty,
- * UNLESS they are required for a field that uses them for more complex
- * validation (the 'calculated' phase).
+ * @param array $data Normalized donation data.
* @throws BadMethodCallException
* @return array An array of errors in a format ready for any
derivitive of
* the main DonationInterface Form class to display. The array will be
empty
* if no errors were generated and everything passed OK.
*/
- public static function validate( GatewayType $gateway, $data,
$check_not_empty = array() ){
+ public static function validate( GatewayType $gateway, $data ) {
//return the array of errors that should be generated on
validate.
//just the same way you'd do it if you were a form passing the
error array around.
@@ -201,12 +190,7 @@
* First: If we need to validate that some things are not
empty, do that.
* Second: Do regular data type validation on things that are
not empty.
* Third: Do validation that depends on multiple fields (making
sure you
- * validated that all the required fields exist on step 1,
regardless of
- * $check_not_empty)
- *
- * $check_not_empty should contain an array of values that need
to be populated.
- * One likely candidate for a source there, is the required
stomp fields as defined in DonationData.
- * Although, a lot of those don't have to have any data in them
either. Boo.
+ * validated that all the required fields exist on step 1).
*
* How about we build an array of shit to do,
* look at it to make sure it's complete, and in order...
@@ -251,18 +235,8 @@
// Depends on currency_code and gateway.
'amount' => 'validate_amount',
-
- // Depends on country
- 'fiscal_number' => 'validate_fiscal_number',
),
);
-
- // Additional fields we should check for emptiness.
- if ( $check_not_empty ) {
- $validations['not_empty'] = array_unique( array_merge(
- $check_not_empty, $validations['not_empty']
- ) );
- }
$errors = array();
$results = array();
@@ -318,9 +292,6 @@
break;
case 'validate_currency_code':
$result = call_user_func(
$callable, $data[$field], $gateway->getCurrencies( $data ) );
- break;
- case 'validate_fiscal_number':
- $result = call_user_func(
$callable, $data[$field], $data['country'] );
break;
default:
$result = call_user_func(
$callable, $data[$field] );
@@ -595,61 +566,6 @@
) {
return false;
}
- return true;
- }
-
- /**
- * Rudimentary tests of fiscal number format for different countries
- * @param string $value The alleged fiscal number
- * @param array $country The country whose format we care about
- * @return boolean True if the $value is a valid fiscal number, false
otherwise
- */
- public static function validate_fiscal_number( $value, $country ) {
- $countryRules = array(
- 'AR' => array(
- 'numeric' => true,
- 'min' => 8,
- 'max' => 8,
- ),
- 'BR' => array(
- 'numeric' => true,
- 'min' => 11,
- 'max' => 14,
- ),
- 'CO' => array(
- 'numeric' => true,
- 'min' => 11,
- 'max' => 14,
- ),
- 'CL' => array(
- 'min' => 8,
- 'max' => 9,
- ),
- 'UY' => array(
- 'numeric' => true,
- 'min' => 6,
- 'max' => 8,
- ),
- );
-
- if ( !isset( $countryRules[$country] ) ) {
- return true;
- }
-
- $rules = $countryRules[$country];
- $unpunctuated = preg_replace( '/[^A-Za-z0-9]/', '', $value );
-
- if ( !empty( $rules['numeric'] ) ) {
- if ( !is_numeric( $unpunctuated ) ) {
- return false;
- }
- }
-
- $length = strlen( $unpunctuated );
- if ( $length < $rules['min'] || $length > $rules['max'] ) {
- return false;
- }
-
return true;
}
diff --git a/gateway_common/DonationData.php b/gateway_common/DonationData.php
index b426952..8171ba5 100644
--- a/gateway_common/DonationData.php
+++ b/gateway_common/DonationData.php
@@ -31,7 +31,7 @@
* places in the request, or 'false' to gather the data the usual way.
* Default is false.
*/
- function __construct( $gateway, $data = false ) {
+ function __construct( GatewayType $gateway, $data = false ) {
$this->gateway = $gateway;
$this->gatewayID = $this->gateway->getIdentifier();
$this->logger = DonationLoggerFactory::getLogger( $gateway, '',
$this );
@@ -958,9 +958,18 @@
* If it is not, it will return an array of errors ready for any
* DonationInterface form class derivitive to display.
*/
- public function getValidationErrors( $recalculate = false,
$check_not_empty = array() ){
+ public function getValidationErrors( $recalculate = false ) {
if ( is_null( $this->validationErrors ) || $recalculate ) {
- $this->validationErrors = DataValidator::validate(
$this->gateway, $this->normalized, $check_not_empty );
+ // Run legacy validations
+ $this->validationErrors = DataValidator::validate(
$this->gateway, $this->normalized );
+
+ // Run modular validations. FIXME: Move this...
somewhere.
+ $transformers = $this->gateway->getDataTransformers();
+ foreach ( $transformers as $transformer ) {
+ if ( $transformer instanceof ValidationHelper )
{
+ $transformer->validate(
$this->normalized, $this->validationErrors );
+ }
+ }
}
return $this->validationErrors;
}
diff --git a/gateway_common/FiscalNumber.php b/gateway_common/FiscalNumber.php
index 3fe8cec..cfd616e 100644
--- a/gateway_common/FiscalNumber.php
+++ b/gateway_common/FiscalNumber.php
@@ -18,4 +18,69 @@
$stagedData['fiscal_number'] = preg_replace(
'/[^a-zA-Z0-9]/', '', $unstagedData['fiscal_number'] );
}
}
+
+ /**
+ * Rudimentary tests of fiscal number format for different countries
+ * @param string $value The alleged fiscal number
+ * @param array $country The country whose format we care about
+ * @return boolean True if the $value is a valid fiscal number, false
otherwise
+ */
+ public function validate( $normalized, &$errors ) {
+ if ( !isset( $normalized['country'] ) ) {
+ return;
+ }
+ $value = $normalized['fiscal_number'];
+ $country = $normalized['country'];
+ $has_error = false;
+
+ $countryRules = array(
+ 'AR' => array(
+ 'numeric' => true,
+ 'min' => 8,
+ 'max' => 8,
+ ),
+ 'BR' => array(
+ 'numeric' => true,
+ 'min' => 11,
+ 'max' => 14,
+ ),
+ 'CO' => array(
+ 'numeric' => true,
+ 'min' => 11,
+ 'max' => 14,
+ ),
+ 'CL' => array(
+ 'min' => 8,
+ 'max' => 9,
+ ),
+ 'UY' => array(
+ 'numeric' => true,
+ 'min' => 6,
+ 'max' => 8,
+ ),
+ );
+
+ if ( !isset( $countryRules[$country] ) ) {
+ return;
+ }
+
+ $rules = $countryRules[$country];
+ $unpunctuated = preg_replace( '/[^A-Za-z0-9]/', '', $value );
+
+ if ( !empty( $rules['numeric'] ) ) {
+ if ( !is_numeric( $unpunctuated ) ) {
+ $has_error = true;
+ }
+ }
+
+ $length = strlen( $unpunctuated );
+ if ( $length < $rules['min'] || $length > $rules['max'] ) {
+ $has_error = true;
+ }
+
+ if ( $has_error ) {
+ // This looks bad.
+ $errors['fiscal_number'] =
DataValidator::getErrorMessage( 'fiscal_number', 'calculated',
$normalized['language'], $country );
+ }
+ }
}
diff --git a/gateway_common/ValidationHelper.php
b/gateway_common/ValidationHelper.php
new file mode 100644
index 0000000..8926315
--- /dev/null
+++ b/gateway_common/ValidationHelper.php
@@ -0,0 +1,9 @@
+<?php
+
+interface ValidationHelper {
+ /**
+ * Run validation on whatever normalized data we're responsible for,
+ * and set errors per field, or under the "general" key.
+ */
+ function validate( $normalized, &$errors );
+}
diff --git a/gateway_common/gateway.adapter.php
b/gateway_common/gateway.adapter.php
index 5bbb267..f902667 100644
--- a/gateway_common/gateway.adapter.php
+++ b/gateway_common/gateway.adapter.php
@@ -362,6 +362,11 @@
}
}
+ // FIXME: Not convinced we need this.
+ public function getDataTransformers() {
+ return $this->data_transformers;
+ }
+
/**
* Determine which account to use for this session
*/
diff --git a/tests/DataValidatorTest.php b/tests/DataValidatorTest.php
index c106ac7..651444e 100644
--- a/tests/DataValidatorTest.php
+++ b/tests/DataValidatorTest.php
@@ -132,6 +132,12 @@
* @dataProvider fiscalNumberProvider
*/
public function testValidateFiscalNumber( $country, $value, $valid ) {
- $this->assertEquals( $valid,
DataValidator::validate_fiscal_number( $value, $country ) );
+ $validator = new FiscalNumber();
+ $errors = array();
+ $validator->validate(
+ array( 'country' => $country, 'fiscal_number' =>
$value, 'language' => 'en' ),
+ $errors
+ );
+ $this->assertEquals( $valid, empty( $errors['fiscal_number'] )
);
}
}
--
To view, visit https://gerrit.wikimedia.org/r/281084
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I011f094981510accdb7fb66d3b4292f6c9ae4de9
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/DonationInterface
Gerrit-Branch: master
Gerrit-Owner: Awight <awi...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
