diogo/symfony
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

feature #10546 [Validator] Improved ISBN validator (sprain)

Browse Source 
This PR was merged into the 2.5-dev branch.

Discussion
----------

[Validator] Improved ISBN validator

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #10386, #10387, #10542
| License       | MIT
| Doc PR        | symfony/symfony-docs/pull/3724

This is a new PR of #10542, now on master branch.

Todos:
- [x] Discuss and determine deprecation versions
- [x] Update docs

After following some discussion in the tickets mentioned above I have improved the ISBN validator, which has had some inconsistencies:

* Use a `type` which can be set to `isbn10` or `isbn13` or `null` (checks if value matches any type) instead of the current boolean `isbn10` and `isbn13` options which cause confusion (e.q. if both are true, does this mean the value must match any or both? You could think it's the latter, but that's actually impossible.).
* In the IBAN validator we recently agreed to be strict about upper- and lowercase handling (#10489). Therefore this should be also the case with the ISBN validator. Some ISBN10 numbers may end with an uppercase `X` (representing the check-digit 10), while a lowercase `x` is considered wrong (see [here](http://www.goodreads.com/topic/show/1253500-a-question-about-isbn-10-s---ending-in-x) and [here](http://en.wikipedia.org/wiki/Category:Pages_with_ISBN_errors)). I did not have access to the actual specifications as I have only found documentation which costs about $100 (e.q. [here](http://www.iso.org/iso/catalogue_detail?csnumber=36563)).

To avoid bc breaks I suggest to introduce deprecations for current constraint options. [In the documentation](http://symfony.com/doc/current/contributing/code/conventions.html#deprecations) I haven't found any information about which versions may introduce deprecations, so you might have to help me out here with hints on how to handle it correctly. I'll be happy to provide the code with the deprecated parts removed after that.

Commits
-------

ec42844 Improved ISBN validator
This commit is contained in:
Fabien Potencier 2014-03-27 08:46:44 +01:00
commit caabd415b1
3 changed files with 96 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
src/Symfony/Component/Validator/Constraints/Isbn.php
View File

@ -12,34 +12,38 @@
namespace Symfony\Component\Validator\Constraints;
use Symfony\Component\Validator\Constraint;
use Symfony\Component\Validator\Exception\MissingOptionsException;
/**
* @Annotation
*
* @author The Whole Life To Learn <thewholelifetolearn@gmail.com>
* @author Manuel Reinhard <manu@sprain.ch>
*/
class Isbn extends Constraint
{
public $isbn10Message = 'This value is not a valid ISBN-10.';
public $isbn13Message = 'This value is not a valid ISBN-13.';
public $bothIsbnMessage = 'This value is neither a valid ISBN-10 nor a valid ISBN-13.';
public $isbn10;
public $isbn13;
public $type;
public $message;
public function __construct($options = null)
/**
* @deprecated Deprecated since version 2.5, to be removed in 3.0. Use option "type" instead.
* @var Boolean
*/
public $isbn10 = false;
/**
* @deprecated Deprecated since version 2.5, to be removed in 3.0. Use option "type" instead.
* @var Boolean
*/
public $isbn13 = false;
/**
* {@inheritDoc}
*/
public function getDefaultOption()
{
if (null !== $options && !is_array($options)) {
$options = array(
'isbn10' => $options,
'isbn13' => $options,
);
}
parent::__construct($options);
if (null === $this->isbn10 && null === $this->isbn13) {
throw new MissingOptionsException(sprintf('Either option "isbn10" or "isbn13" must be given for constraint "%s".', __CLASS__), array('isbn10', 'isbn13'));
}
return 'type';
}
}

81
src/Symfony/Component/Validator/Constraints/IsbnValidator.php
View File

@ -16,10 +16,10 @@ use Symfony\Component\Validator\ConstraintValidator;
use Symfony\Component\Validator\Exception\UnexpectedTypeException;
/**
* Validates whether the value is a valid ISBN-10 or ISBN-13.
* Validates whether the value is a valid ISBN-10 or ISBN-13
*
* @author The Whole Life To Learn <thewholelifetolearn@gmail.com>
*
* @author Manuel Reinhard <manu@sprain.ch>
* @see https://en.wikipedia.org/wiki/Isbn
*/
class IsbnValidator extends ConstraintValidator
@ -45,11 +45,43 @@ class IsbnValidator extends ConstraintValidator
$value = str_replace('-', '', $value);
}
$validation = 0;
if (null == $constraint->type) {
if ($constraint->isbn10 && !$constraint->isbn13) {
$constraint->type = 'isbn10';
$value = strtoupper($value);
} elseif ($constraint->isbn13 && !$constraint->isbn10) {
$constraint->type = 'isbn13';
$value = strtoupper($value);
}
}
if ('isbn10' === $constraint->type) {
if (!$this->validateIsbn10($value)) {
$this->context->addViolation($this->getMessage($constraint, 'isbn10'));
return;
}
} elseif ('isbn13' === $constraint->type) {
if (!$this->validateIsbn13($value)) {
$this->context->addViolation($this->getMessage($constraint, 'isbn13'));
return;
}
} else {
if (!$this->validateIsbn10($value) && !$this->validateIsbn13($value)) {
$this->context->addViolation($this->getMessage($constraint));
return;
}
}
}
protected function validateIsbn10($value)
{
$validation = 0;
$valueLength = strlen($value);
if (10 === $valueLength && null !== $constraint->isbn10) {
if (10 === $valueLength) {
for ($i = 0; $i < 10; $i++) {
if ($value[$i] == 'X') {
$validation += 10 * intval(10 - $i);
@ -59,13 +91,21 @@ class IsbnValidator extends ConstraintValidator
}
if ($validation % 11 != 0) {
if (null !== $constraint->isbn13) {
$this->context->addViolation($constraint->bothIsbnMessage);
return false;
} else {
$this->context->addViolation($constraint->isbn10Message);
return true;
}
}
} elseif (13 === $valueLength && null !== $constraint->isbn13) {
return false;
}
protected function validateIsbn13($value)
{
$validation = 0;
$valueLength = strlen($value);
if (13 === $valueLength) {
for ($i = 0; $i < 13; $i += 2) {
$validation += intval($value[$i]);
}
@ -74,20 +114,25 @@ class IsbnValidator extends ConstraintValidator
}
if ($validation % 10 != 0) {
if (null !== $constraint->isbn10) {
$this->context->addViolation($constraint->bothIsbnMessage);
return false;
} else {
$this->context->addViolation($constraint->isbn13Message);
return true;
}
}
return false;
}
protected function getMessage($constraint, $type=null)
{
if (null !== $constraint->message) {
return $constraint->message;
} elseif ($type == 'isbn10') {
return $constraint->isbn10Message;
} elseif ($type == 'isbn13') {
return $constraint->isbn13Message;
} else {
if (null !== $constraint->isbn10 && null !== $constraint->isbn13) {
$this->context->addViolation($constraint->bothIsbnMessage);
} elseif (null !== $constraint->isbn10) {
$this->context->addViolation($constraint->isbn10Message);
} else {
$this->context->addViolation($constraint->isbn13Message);
}
return $constraint->bothIsbnMessage;
}
}
}

14
src/Symfony/Component/Validator/Tests/Constraints/IsbnValidatorTest.php
View File

@ -44,6 +44,7 @@ class IsbnValidatorTest extends \PHPUnit_Framework_TestCase
array('0321812700'),
array('0-45122-5244'),
array('0-4712-92311'),
array('0-9752298-0-X')
);
}
@ -58,6 +59,7 @@ class IsbnValidatorTest extends \PHPUnit_Framework_TestCase
array('0-4X19-92611'),
array('0_45122_5244'),
array('2870#971#648'),
array('0-9752298-0-x')
);
}
@ -145,7 +147,7 @@ class IsbnValidatorTest extends \PHPUnit_Framework_TestCase
*/
public function testValidIsbn10($isbn)
{
$constraint = new Isbn(array('isbn10' => true));
$constraint = new Isbn(array('type' => 'isbn10'));
$this->context
->expects($this->never())
->method('addViolation');
@ -158,7 +160,7 @@ class IsbnValidatorTest extends \PHPUnit_Framework_TestCase
*/
public function testInvalidIsbn10($isbn)
{
$constraint = new Isbn(array('isbn10' => true));
$constraint = new Isbn(array('type' => 'isbn10'));
$this->context
->expects($this->once())
->method('addViolation')
@ -172,7 +174,7 @@ class IsbnValidatorTest extends \PHPUnit_Framework_TestCase
*/
public function testValidIsbn13($isbn)
{
$constraint = new Isbn(array('isbn13' => true));
$constraint = new Isbn(array('type' => 'isbn13'));
$this->context
->expects($this->never())
->method('addViolation');
@ -185,7 +187,7 @@ class IsbnValidatorTest extends \PHPUnit_Framework_TestCase
*/
public function testInvalidIsbn13($isbn)
{
$constraint = new Isbn(array('isbn13' => true));
$constraint = new Isbn(array('type' => 'isbn13'));
$this->context
->expects($this->once())
->method('addViolation')
@ -199,7 +201,7 @@ class IsbnValidatorTest extends \PHPUnit_Framework_TestCase
*/
public function testValidIsbn($isbn)
{
$constraint = new Isbn(array('isbn10' => true, 'isbn13' => true));
$constraint = new Isbn();
$this->context
->expects($this->never())
->method('addViolation');
@ -212,7 +214,7 @@ class IsbnValidatorTest extends \PHPUnit_Framework_TestCase
*/
public function testInvalidIsbn($isbn)
{
$constraint = new Isbn(array('isbn10' => true, 'isbn13' => true));
$constraint = new Isbn();
$this->context
->expects($this->once())
->method('addViolation')