Skip to content

[Validator] Improved ISBN validator #10542

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 #10542

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
46 changes: 32 additions & 14 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,52 @@
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>
*
* @api
Copy link
Member

Choose a reason for hiding this comment

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

must be removed

*/
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 $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;

/**
* @deprecated Deprecated since version 2.3.12, to be removed in 2.5. Use option "type" instead.
* @var bool
*/
public $isbn10 = false;

/**
* @deprecated Deprecated since version 2.3.12, to be removed in 2.5. Use option "type" instead.
* @var bool
*/
public $isbn13 = false;

public function __construct($options = null)
{
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'));
if ($this->isbn10 || $this->isbn13) {
trigger_error(
'The options "isbn10" and "isbn13" are deprecated since version 2.3.12 and will be removed in 2.5. Use option "type" instead.',
E_USER_DEPRECATED
);
}
}

/**
* {@inheritDoc}
*/
public function getDefaultOption()
{
return 'type';
}
}
92 changes: 69 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,11 +16,12 @@
use Symfony\Component\Validator\Exception\UnexpectedTypeException;

/**
* Validates whether the value is a valid ISBN-10 or ISBN-13.
* Validates wether the value is a valid ISBN-10 or ISBN-13
Copy link
Member

Choose a reason for hiding this comment

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

whether is correct.

*
* @author The Whole Life To Learn <thewholelifetolearn@gmail.com>
* @author Manuel Reinhard <manu@sprain.ch>
*
Copy link
Member

Choose a reason for hiding this comment

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

what was it removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By mistake. Putting back in.

* @see https://en.wikipedia.org/wiki/Isbn
* @api
Copy link
Member

Choose a reason for hiding this comment

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

must be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. What is it for? Some validators have this annotation.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sprain these annotations are there to mark a stable api.

*/
class IsbnValidator extends ConstraintValidator
{
Expand All @@ -41,11 +42,43 @@ public function validate($value, Constraint $constraint)
$value = str_replace('-', '', $value);
}

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

Choose a reason for hiding this comment

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

should be a strict comparison

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 @@ -55,13 +88,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;
}
Copy link
Member

Choose a reason for hiding this comment

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

this can be changed to return $validation % 11 === 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can. But is it more readable? Or is there a coding standard for this?
IMO the current code is easier to understand.

} elseif (13 === $valueLength && null !== $constraint->isbn13) {
}

return false;
}

protected function validateIsbn13($value)
Copy link
Member

Choose a reason for hiding this comment

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

should be private

{
$validation = 0;
$valueLength = strlen($value);

if (13 === $valueLength) {
for ($i = 0; $i < 13; $i += 2) {
$validation += intval($value[$i]);
}
Expand All @@ -70,20 +111,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)
Copy link
Member

Choose a reason for hiding this comment

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

should be private, and the $constraint argument should be typehinted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why private?

Copy link
Member

Choose a reason for hiding this comment

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

because the rule in Symfony is that we use private everywhere until we decide that a method needs to be protected to be a supported extension point (for which we then guarantee BC until 3.0)

{
if (null !== $constraint->message) {
return $constraint->message;
} elseif ($type == 'isbn10') {
Copy link
Member

Choose a reason for hiding this comment

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

this should simply be if as the previous if returns

Copy link
Member

Choose a reason for hiding this comment

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

and it should be a strict comparison

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