Skip to content

[Validator] Improved ISBN validator #10546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[Validator] Improved ISBN validator #10546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 19 additions & 15 deletions src/Symfony/Component/Validator/Constraints/Isbn.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like type. What if there is another one like isbn15 you cannot specify that you allow isbn13 and isbn15 but not isbn10 with a "type". But with the booleans you can.
Also it's not clear what values "type" actualyl accept. Much easier with booleans.
Also why deprecating stuff when it's not necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original issue #10386 shows, that it's not so clear with the booleans.
Also, the chance that there will be an ISBN15 type or so is very small. ISBN13 has been introduced in 2007, after 35 years of ISBN10.

public $message;

public function __construct($options = null)
{
if (null !== $options && !is_array($options)) {
$options = array(
'isbn10' => $options,
'isbn13' => $options,
);
}
/**
* @deprecated Deprecated since version 2.5, to be removed in 3.0. Use option "type" instead.
* @var Boolean
*/
public $isbn10 = false;

parent::__construct($options);
/**
* @deprecated Deprecated since version 2.5, to be removed in 3.0. Use option "type" instead.
* @var Boolean
*/
public $isbn13 = false;

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'));
}
/**
* {@inheritDoc}
*/
public function getDefaultOption()
{
return 'type';
}
}
91 changes: 68 additions & 23 deletions src/Symfony/Component/Validator/Constraints/IsbnValidator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@
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
Expand All @@ -45,11 +45,43 @@ public function validate($value, Constraint $constraint)
$value = str_replace('-', '', $value);
}

$validation = 0;
$value = strtoupper($value);
if (null == $constraint->type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

===

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);
Expand All @@ -59,13 +91,21 @@ public function validate($value, Constraint $constraint)
}

if ($validation % 11 != 0) {
if (null !== $constraint->isbn13) {
$this->context->addViolation($constraint->bothIsbnMessage);
} else {
$this->context->addViolation($constraint->isbn10Message);
}
return false;
} else {
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]);
}
Expand All @@ -74,20 +114,25 @@ public function validate($value, Constraint $constraint)
}

if ($validation % 10 != 0) {
if (null !== $constraint->isbn10) {
$this->context->addViolation($constraint->bothIsbnMessage);
} else {
$this->context->addViolation($constraint->isbn13Message);
}
}
} else {
if (null !== $constraint->isbn10 && null !== $constraint->isbn13) {
$this->context->addViolation($constraint->bothIsbnMessage);
} elseif (null !== $constraint->isbn10) {
$this->context->addViolation($constraint->isbn10Message);
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') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'isbn10' === $type

return $constraint->isbn10Message;
} elseif ($type == 'isbn13') {
return $constraint->isbn13Message;
} else {
return $constraint->bothIsbnMessage;
}
}
}
14 changes: 8 additions & 6 deletions src/Symfony/Component/Validator/Tests/Constraints/IsbnValidatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public function getValidIsbn10()
array('0321812700'),
array('0-45122-5244'),
array('0-4712-92311'),
array('0-9752298-0-X')
);
}

Expand All @@ -58,6 +59,7 @@ public function getInvalidIsbn10()
array('0-4X19-92611'),
array('0_45122_5244'),
array('2870#971#648'),
array('0-9752298-0-x')
);
}

Expand Down Expand Up @@ -145,7 +147,7 @@ public function testExpectsStringCompatibleType()
*/
public function testValidIsbn10($isbn)
{
$constraint = new Isbn(array('isbn10' => true));
$constraint = new Isbn(array('type' => 'isbn10'));
$this->context
->expects($this->never())
->method('addViolation');
Expand All @@ -158,7 +160,7 @@ public function testValidIsbn10($isbn)
*/
public function testInvalidIsbn10($isbn)
{
$constraint = new Isbn(array('isbn10' => true));
$constraint = new Isbn(array('type' => 'isbn10'));
$this->context
->expects($this->once())
->method('addViolation')
Expand All @@ -172,7 +174,7 @@ public function testInvalidIsbn10($isbn)
*/
public function testValidIsbn13($isbn)
{
$constraint = new Isbn(array('isbn13' => true));
$constraint = new Isbn(array('type' => 'isbn13'));
$this->context
->expects($this->never())
->method('addViolation');
Expand All @@ -185,7 +187,7 @@ public function testValidIsbn13($isbn)
*/
public function testInvalidIsbn13($isbn)
{
$constraint = new Isbn(array('isbn13' => true));
$constraint = new Isbn(array('type' => 'isbn13'));
$this->context
->expects($this->once())
->method('addViolation')
Expand All @@ -199,7 +201,7 @@ public function testInvalidIsbn13($isbn)
*/
public function testValidIsbn($isbn)
{
$constraint = new Isbn(array('isbn10' => true, 'isbn13' => true));
$constraint = new Isbn();
$this->context
->expects($this->never())
->method('addViolation');
Expand All @@ -212,7 +214,7 @@ public function testValidIsbn($isbn)
*/
public function testInvalidIsbn($isbn)
{
$constraint = new Isbn(array('isbn10' => true, 'isbn13' => true));
$constraint = new Isbn();
$this->context
->expects($this->once())
->method('addViolation')
Expand Down