8000 feature #10546 [Validator] Improved ISBN validator (sprain) · symfony/symfony@caabd41 · GitHub
[go: up one dir, main page]
Skip to content

Commit caabd41

Browse files
fabpotfabpot
committed
feature #10546 [Validator] Improved ISBN validator (sprain)
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
2 parents 711788b + ec42844 commit caabd41

File tree

3 files changed

+95
-44
lines changed

3 files changed

+95
-44
lines changed

‎src/Symfony/Component/Validator/Constraints/Isbn.php

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -12,34 +12,38 @@
1212
namespace Symfony\Component\Validator\Constraints;
1313

1414
use Symfony\Component\Validator\Constraint;
15-
use Symfony\Component\Validator\Exception\MissingOptionsException;
1615

1716
/**
1817
* @Annotation
1918
*
2019
* @author The Whole Life To Learn <thewholelifetolearn@gmail.com>
20+
* @author Manuel Reinhard <manu@sprain.ch>
2121
*/
2222
class Isbn extends Constraint
2323
{
2424
public $isbn10Message = 'This value is not a valid ISBN-10.';
2525
public $isbn13Message = 'This value is not a valid ISBN-13.';
2626
public $bothIsbnMessage = 'This value is neither a valid ISBN-10 nor a valid ISBN-13.';
27-
public $isbn10;
28-
public $isbn13;
27+
public $type;
28+
public $message;
2929

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

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

41-
if (null === $this->isbn10 && null === $this->isbn13) {
42-
throw new MissingOptionsException(sprintf('Either option "isbn10" or "isbn13" must be given for constraint "%s".', __CLASS__), array('isbn10', 'isbn13'));
43-
}
42+
/**
43+
* {@inheritDoc}
44+
*/
45+
public function getDefaultOption()
46+
{
47+
return 'type';
4448
}
4549
}

‎src/Symfony/Component/Validator/Constraints/IsbnValidator.php

Lines changed: 68 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,10 @@
1616
use Symfony\Component\Validator\Exception\UnexpectedTypeException;
1717

1818
/**
19-
* Validates whether the value is a valid ISBN-10 or ISBN-13.
19+
* Validates whether the value is a valid ISBN-10 or ISBN-13
2020
*
2121
* @author The Whole Life To Learn <thewholelifetolearn@gmail.com>
22-
*
22+
* @author Manuel Reinhard <manu@sprain.ch>
2323
* @see https://en.wikipedia.org/wiki/Isbn
2424
*/
2525
class IsbnValidator extends ConstraintValidator
@@ -45,11 +45,43 @@ public function validate($value, Constraint $constraint)
4545
$value = str_replace('-', '', $value);
4646
}
4747

48-
$validation = 0;
49-
$value = strtoupper($value);
48+
if (null == $constraint->type) {
49+
if ($constraint->isbn10 && !$constraint->isbn13) {
50+
$constraint->type = 'isbn10';
51+
$value = strtoupper($value);
52+
} elseif ($constraint->isbn13 && !$constraint->isbn10) {
53+
$constraint->type = 'isbn13';
54+
$value = strtoupper($value);
55+
}
56+
}
57+
58+
if ('isbn10' === $constraint->type) {
59+
if (!$this->validateIsbn10($value)) {
60+
$this->context->addViolation($this->getMessage($constraint, 'isbn10'));
61+
62+
return;
63+
}
64+
} elseif ('isbn13' === $constraint->type) {
65+
if (!$this->validateIsbn13($value)) {
66+
$this->context->addViolation($this->getMessage($constraint, 'isbn13'));
67+
68+
return;
69+
}
70+
} else {
71+
if (!$this->validateIsbn10($value) && !$this->validateIsbn13($value)) {
72+
$this->context->addViolation($this->getMessage($constraint));
73+
74+
return;
75+
}
76+
}
77+
}
78+
79+
protected function validateIsbn10($value)
80+
{
81+
$validation = 0;
5082
$valueLength = strlen($value);
5183

52-
if (10 === $valueLength && null !== $constraint->isbn10) {
84+
if (10 === $valueLength) {
5385
for ($i = 0; $i < 10; $i++) {
5486
if ($value[$i] == 'X') {
5587
$validation += 10 * intval(10 - $i);
@@ -59,13 +91,21 @@ public function validate($value, Constraint $constraint)
5991
}
6092

6193
if ($validation % 11 != 0) {
62-
if (null !== $constraint->isbn13) {
63-
$this->context->addViolation($constraint->bothIsbnMessage);
64-
} else {
65-
$this->context->addViolation($constraint->isbn10Message);
66-
}
94+
return false;
95+
} else {
96+
return true;
6797
}
68-
} elseif (13 === $valueLength && null !== $constraint->isbn13) {
98+
}
99+
100+
return false;
101+
}
102+
103+
protected function validateIsbn13($value)
104+
{
105+
$validation = 0;
106+
$valueLength = strlen($value);
107+
108+
if (13 === $valueLength) {
69109
for ($i = 0; $i < 13; $i += 2) {
70110
$validation += intval($value[$i]);
71111
}
@@ -74,20 +114,25 @@ public function validate($value, Constraint $constraint)
74114
}
75115

76116
if ($validation % 10 != 0) {
77-
if (null !== $constraint->isbn10) {
78-
$this->context->addViolation($constraint->bothIsbnMessage);
79-
} else {
80-
$this->context->addViolation($constraint->isbn13Message);
81-
}
82-
}
83-
} else {
84-
if (null !== $constraint->isbn10 && null !== $constraint->isbn13) {
85-
$this->context->addViolation($constraint->bothIsbnMessage);
86-
} elseif (null !== $constraint->isbn10) {
87-
$this->context->addViolation($constraint->isbn10Message);
117+
return false;
88118
} else {
89-
$this->context->addViolation($constraint->isbn13Message);
119+
return true;
90120
}
91121
}
122+
123+
return false;
124+
}
125+
126+
protected function getMessage($constraint, $type=null)
127+
{
128+
if (null !== $constraint->message) {
129+
return $constraint->message;
130+
} elseif ($type == 'isbn10') {
131+
return $constraint->isbn10Message;
132+
} elseif ($type == 'isbn13') {
133+
return $constraint->isbn13Message;
134+
} else {
135+
return $constraint->bothIsbnMessage;
136+
}
92137
}
93138
}

‎src/Symfony/Component/Validator/Tests/Constraints/IsbnValidatorTest.php

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ public function getValidIsbn10()
4444
array('0321812700'),
4545
array('0-45122-5244'),
4646
array('0-4712-92311'),
47+
array('0-9752298-0-X')
4748
);
4849
}
4950

@@ -58,6 +59,7 @@ public function getInvalidIsbn10()
5859
array('0-4X19-92611'),
5960
array('0_45122_5244'),
6061
array('2870#971#648'),
62+
array('0-9752298-0-x')
6163
);
6264
}
6365

@@ -145,7 +147,7 @@ public function testExpectsStringCompatibleType()
145147
*/
146148
public function testValidIsbn10($isbn)
147149
{
148-
$constraint = new Isbn(array('isbn10' => true));
150+
$constraint = new Isbn(array('type' => 'isbn10'));
149151
$this->context
150152
->expects($this->never())
151153
->method('addViolation');
@@ -158,7 +160,7 @@ public function testValidIsbn10($isbn)
158160
*/
159161
public function testInvalidIsbn10($isbn)
160162
{
161-
$constraint = new Isbn(array('isbn10' => true));
163+
$constraint = new Isbn(array('type' => 'isbn10'));
162164
$this->context
163165
->expects($this->once())
164166
->method('addViolation')
@@ -172,7 +174,7 @@ public function testInvalidIsbn10($isbn)
172174
*/
173175
public function testValidIsbn13($isbn)
174176
{
175-
$constraint = new Isbn(array('isbn13' => true));
177+
$constraint = new Isbn(array('type' => 'isbn13'));
176178
$this->context
177179
->expects($this->never())
178180
->method('addViolation');
@@ -185,7 +187,7 @@ public function testValidIsbn13($isbn)
185187
*/
186188
public function testInvalidIsbn13($isbn)
187189
{
188-
$constraint = new Isbn(array('isbn13' => true));
190+
$constraint = new Isbn(array('type' => 'isbn13'));
189191
$this->context
190192
->expects($this->once())
191193
->method('addViolation')
@@ -199,7 +201,7 @@ public function testInvalidIsbn13($isbn)
199201
*/
200202
public function testValidIsbn($isbn)
201203
{
202-
$constraint = new Isbn(array('isbn10' => true, 'isbn13' => true));
204+
$constraint = new Isbn();
203205
$this->context
204206
->expects($this->never())
205207
->method('addViolation');
@@ -212,7 +214,7 @@ public function testValidIsbn($isbn)
212214
*/
213215
public function testInvalidIsbn($isbn)
214216
{
215-
$constraint = new Isbn(array('isbn10' => true, 'isbn13' => true));
217+
$constraint = new Isbn();
216218
$this->context
217219
->expects($this->once())
218220
->method('addViolation')

0 commit comments

Comments
 (0)
0