From d8dd33dcb879ca34d40313d0a3826473f0ac9183 Mon Sep 17 00:00:00 2001 From: Corinna Hillebrand Date: Tue, 5 May 2020 19:48:17 +0200 Subject: [PATCH] Adjust validation for higher accuracy The static data might need to go somewhere else, also it is not clear to me yet (because the tests say otherwise) what value for the field "country" can actually be expected here. https://phabricator.wikimedia.org/T247227 --- src/Validators/AddressValidator.php | 41 ++++++++++++++----- .../Unit/Validators/AddressValidatorTest.php | 36 +++++++++++----- 2 files changed, 57 insertions(+), 20 deletions(-) diff --git a/src/Validators/AddressValidator.php b/src/Validators/AddressValidator.php index 57aa51a..a6f3ef5 100644 --- a/src/Validators/AddressValidator.php +++ b/src/Validators/AddressValidator.php @@ -36,9 +36,16 @@ class AddressValidator { self::SOURCE_STREET_ADDRESS => 100, self::SOURCE_CITY => 100, self::SOURCE_COUNTRY => 8, + self::SOURCE_POSTAL_CODE => 16, ]; - public function validatePostalAddress( string $streetAddress, string $postalCode, string $city, string $country ): ValidationResult { + private $countriesPostcodePatterns; + + public function __construct( array $countriesPostcodePatterns ) { + $this->countriesPostcodePatterns = $countriesPostcodePatterns; + } + + public function validatePostalAddress( string $streetAddress, string $postalCode, string $city, string $countryCode ): ValidationResult { $violations = []; if ( $streetAddress === '' ) { @@ -51,12 +58,15 @@ public function validatePostalAddress( string $streetAddress, string $postalCode $violations[] = $this->validateFieldLength( $streetAddress, self::SOURCE_STREET_ADDRESS ); } - if ( !preg_match( '/^\\d{4,5}$/', $postalCode ) ) { - $violations[] = new ConstraintViolation( - $postalCode, - self::VIOLATION_NOT_POSTCODE, - self::SOURCE_POSTAL_CODE - ); + if ( isset( $this->countriesPostcodePatterns[$countryCode] ) ) { + $violations[] = $this->validatePostalCode( $this->countriesPostcodePatterns[$countryCode], $postalCode ); + } else { + $postalCodeLengthViolation = $this->validateFieldLength( $postalCode, self::SOURCE_POSTAL_CODE ); + if ( $postalCodeLengthViolation === null ) { + $violations[] = $this->validatePostalCode( '/^.+$/', $postalCode ); + } else { + $violations[] = $postalCodeLengthViolation; + } } if ( $city === '' ) { @@ -69,19 +79,30 @@ public function validatePostalAddress( string $streetAddress, string $postalCode $violations[] = $this->validateFieldLength( $city, self::SOURCE_CITY ); } - if ( $country === '' ) { + if ( $countryCode === '' ) { $violations[] = new ConstraintViolation( - $country, + $countryCode, self::VIOLATION_MISSING, self::SOURCE_COUNTRY ); } else { - $violations[] = $this->validateFieldLength( $country, self::SOURCE_COUNTRY ); + $violations[] = $this->validateFieldLength( $countryCode, self::SOURCE_COUNTRY ); } return new ValidationResult( ...array_filter( $violations ) ); } + private function validatePostalCode( string $pattern, string $postalCode ): ?ConstraintViolation { + if ( !preg_match( $pattern, $postalCode ) ) { + return new ConstraintViolation( + $postalCode, + self::VIOLATION_NOT_POSTCODE, + self::SOURCE_POSTAL_CODE + ); + } + return null; + } + public function validatePersonName( string $salutation, string $title, string $firstname, string $lastname ): ValidationResult { $violations = []; diff --git a/tests/Unit/Validators/AddressValidatorTest.php b/tests/Unit/Validators/AddressValidatorTest.php index e92ba7a..8279eff 100644 --- a/tests/Unit/Validators/AddressValidatorTest.php +++ b/tests/Unit/Validators/AddressValidatorTest.php @@ -11,29 +11,39 @@ */ class AddressValidatorTest extends \PHPUnit\Framework\TestCase { + private const COUNTRY_POSTCODE_PATTERNS = [ + 'DE' => '/^[0-9]{5}$/', + 'AT' => '/^[0-9]{4}$/', + 'CH' => '/^[0-9]{4}$/', + 'BE' => '/^[0-9]{4}$/', + 'IT' => '/^[0-9]{5}$/', + 'LI' => '/^[0-9]{4}$/', + 'LU' => '/^[0-9]{4}$/', + ]; + public function testGivenValidPostalAddress_noViolationsAreReturned(): void { - $validator = new AddressValidator(); + $validator = new AddressValidator( self::COUNTRY_POSTCODE_PATTERNS ); $validationResult = $validator->validatePostalAddress( 'Test 1234', '12345', 'Test City', 'Germany' ); $this->assertTrue( $validationResult->isSuccessful() ); } public function testGivenValidPersonName_noViolationsAreReturned(): void { - $validator = new AddressValidator(); + $validator = new AddressValidator( self::COUNTRY_POSTCODE_PATTERNS ); $validationResult = $validator->validatePersonName( 'Herr', 'Prof. Dr.', 'Tester', 'Testing' ); $this->assertTrue( $validationResult->isSuccessful() ); } public function testGivenValidCompany_noViolationsAreReturned(): void { - $validator = new AddressValidator(); + $validator = new AddressValidator( self::COUNTRY_POSTCODE_PATTERNS ); $validationResult = $validator->validateCompanyName( 'Test Company GmbH & Co. KG' ); $this->assertTrue( $validationResult->isSuccessful() ); } public function testGivenTooLongPostalValues_correctViolationsAreReturned(): void { - $validator = new AddressValidator(); + $validator = new AddressValidator( self::COUNTRY_POSTCODE_PATTERNS ); $validationResult = $validator->validatePostalAddress( str_repeat( 'a', 101 ), - str_repeat( '1', 6 ), + str_repeat( '1', 17 ), str_repeat( 'a', 101 ), str_repeat( 'a', 9 ) ); @@ -46,7 +56,7 @@ public function testGivenTooLongPostalValues_correctViolationsAreReturned(): voi } public function testGivenTooLongNameValues_correctViolationsAreReturned(): void { - $validator = new AddressValidator(); + $validator = new AddressValidator( self::COUNTRY_POSTCODE_PATTERNS ); $validationResult = $validator->validatePersonName( str_repeat( 'a', 17 ), str_repeat( 'a', 17 ), @@ -62,7 +72,7 @@ public function testGivenTooLongNameValues_correctViolationsAreReturned(): void } public function testGivenTooLongCompanyValues_correctViolationsAreReturned(): void { - $validator = new AddressValidator(); + $validator = new AddressValidator( self::COUNTRY_POSTCODE_PATTERNS ); $validationResult = $validator->validateCompanyName( str_repeat( 'a', 101 ) ); @@ -72,7 +82,7 @@ public function testGivenTooLongCompanyValues_correctViolationsAreReturned(): vo } public function testGivenEmptyPostalValues_correctViolationsAreReturned(): void { - $validator = new AddressValidator(); + $validator = new AddressValidator( self::COUNTRY_POSTCODE_PATTERNS ); $validationResult = $validator->validatePostalAddress( '', '', @@ -89,7 +99,7 @@ public function testGivenEmptyPostalValues_correctViolationsAreReturned(): void } public function testGivenEmptyNameValues_correctViolationsAreReturned(): void { - $validator = new AddressValidator(); + $validator = new AddressValidator( self::COUNTRY_POSTCODE_PATTERNS ); $validationResult = $validator->validatePersonName( '', '', @@ -105,10 +115,16 @@ public function testGivenEmptyNameValues_correctViolationsAreReturned(): void { } public function testGivenEmptyCompanyValues_correctViolationsAreReturned(): void { - $validator = new AddressValidator(); + $validator = new AddressValidator( self::COUNTRY_POSTCODE_PATTERNS ); $validationResult = $validator->validateCompanyName( '' ); $this->assertFalse( $validationResult->isSuccessful() ); $this->assertCount( 1, $validationResult->getViolations() ); $this->assertSame( 'companyName', $validationResult->getViolations()[0]->getSource() ); } + + public function testGivenBadPostcodeForCountry_correctViolationsAreReturned(): void { + $validator = new AddressValidator( self::COUNTRY_POSTCODE_PATTERNS ); + $validationResult = $validator->validatePostalAddress( 'Test 1234', '123', 'Test City', 'DE' ); + $this->assertSame( 'postcode', $validationResult->getViolations()[0]->getSource() ); + } }