[Validator] Added ISBN validator #6718

Closed
wants to merge 4 commits into
from

Projects

None yet

6 participants

@thewholelifetolearn
Contributor

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
License of the code: MIT

For information about ISBN-10 and ISBN13: https://en.wikipedia.org/wiki/Isbn

This constraint permits to valid a ISBN-10 code, a ISBN-13 code or both on a value

@stloyd stloyd commented on an outdated diff Jan 12, 2013
src/Symfony/Component/Validator/Constraints/Isbn.php
+ *
+ * @author The Whole Life To Learn <thewholelifetolearn@gmail.com>
+ *
+ * @api
+ */
+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 function __construct($options = null)
+ {
+ if(null !== $options && !is_array($options)) {
@stloyd
stloyd Jan 12, 2013 Contributor

Missing space after if. Same below.

@stloyd stloyd commented on an outdated diff Jan 12, 2013
...ony/Component/Validator/Constraints/IsbnValidator.php
+{
+ /**
+ * {@inheritDoc}
+ */
+ public function validate($value, Constraint $constraint)
+ {
+ $validation = null;
+ $checkIsbn10 = false;
+ $checkIsbn13 = false;
+
+ if (null === $value || '' === $value) {
+ return;
+ }
+
+ if (!is_scalar($value)
+ && !(is_object($value) && method_exists($value, '__toString'))) {
@stloyd
stloyd Jan 12, 2013 Contributor

This should be one line.

@stloyd stloyd commented on an outdated diff Jan 12, 2013
...ony/Component/Validator/Constraints/IsbnValidator.php
+/**
+ * Validates wether the value is a valid ISBN-10 or ISBN-13
+ *
+ * @see https://en.wikipedia.org/wiki/Isbn
+ * @author The Whole Life To Learn <thewholelifetolearn@gmail.com>
+ *
+ * @api
+ */
+class IsbnValidator extends ConstraintValidator
+{
+ /**
+ * {@inheritDoc}
+ */
+ public function validate($value, Constraint $constraint)
+ {
+ $validation = null;
@stloyd
stloyd Jan 12, 2013 Contributor

This can be defined after both ifs.

@stloyd stloyd commented on an outdated diff Jan 12, 2013
...ony/Component/Validator/Constraints/IsbnValidator.php
+ return;
+ }
+
+ if (!is_scalar($value)
+ && !(is_object($value) && method_exists($value, '__toString'))) {
+ throw new UnexpectedTypeException($value, 'string');
+ }
+
+ $value = (string) $value;
+ $value = strtoupper($value);
+
+ if (!is_int($value)) {
+ $value = str_replace('-', '', $value);
+ }
+
+ if (10 == strlen($value)) {
@stloyd
stloyd Jan 12, 2013 Contributor

This should be with type comparsion: ===

@stloyd
stloyd Jan 12, 2013 Contributor

Also strlen() result should be set to variable to not call it twice.

@stloyd
stloyd Jan 12, 2013 Contributor

Also you should fail (push violation) when it's set in constraint to use ISBN13 not 10. Same below.

@stloyd stloyd commented on an outdated diff Jan 12, 2013
...ony/Component/Validator/Constraints/IsbnValidator.php
+
+ if (10 == strlen($value)) {
+ for ($i = 0; $i < 10; $i++) {
+ if ($value[$i] == 'X') {
+ $validation += 10 * intval(10 - $i);
+ } else {
+ $validation += intval($value[$i]) * intval(10 - $i);
+ }
+ }
+
+ if ($validation % 11 == 0) {
+ $checkIsbn10 = true;
+ }
+ }
+
+ if (13 == strlen($value)) {
@stloyd
stloyd Jan 12, 2013 Contributor

This should be elseif.

@stof stof commented on an outdated diff Jan 12, 2013
...ony/Component/Validator/Constraints/IsbnValidator.php
+ public function validate($value, Constraint $constraint)
+ {
+ $validation = null;
+ $checkIsbn10 = false;
+ $checkIsbn13 = false;
+
+ if (null === $value || '' === $value) {
+ return;
+ }
+
+ if (!is_scalar($value)
+ && !(is_object($value) && method_exists($value, '__toString'))) {
+ throw new UnexpectedTypeException($value, 'string');
+ }
+
+ $value = (string) $value;
@stof
stof Jan 12, 2013 Member

This line is useless as strtoupper already casts its argument as string.

@stof stof commented on an outdated diff Jan 12, 2013
...ony/Component/Validator/Constraints/IsbnValidator.php
+ $checkIsbn10 = false;
+ $checkIsbn13 = false;
+
+ if (null === $value || '' === $value) {
+ return;
+ }
+
+ if (!is_scalar($value)
+ && !(is_object($value) && method_exists($value, '__toString'))) {
+ throw new UnexpectedTypeException($value, 'string');
+ }
+
+ $value = (string) $value;
+ $value = strtoupper($value);
+
+ if (!is_int($value)) {
@stof
stof Jan 12, 2013 Member

is_int($value) will always be false here as $value will always be a string here.

@stloyd stloyd commented on an outdated diff Jan 12, 2013
...ony/Component/Validator/Constraints/IsbnValidator.php
+ }
+
+ if (13 == strlen($value)) {
+ for ($i = 0; $i < 13; $i += 2) {
+ $validation += intval($value[$i]);
+ }
+ for ($i = 1; $i < 12; $i += 2) {
+ $validation += intval($value[$i]) * 3;
+ }
+
+ if ($validation % 10 == 0) {
+ $checkIsbn13 = true;
+ }
+ }
+
+ if (null !== $constraint->isbn10 && null !== $constraint->isbn13
@stloyd
stloyd Jan 12, 2013 Contributor

No need for 3 checks that look almost same.

@stof stof commented on an outdated diff Jan 12, 2013
...ent/Validator/Tests/Constraints/IsbnValidatorTest.php
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Validator\Tests\Constraints;
+
+use Symfony\Component\Validator\Constraints\Isbn;
+use Symfony\Component\Validator\Constraints\IsbnValidator;
+
+/**
+ * @see https://en.wikipedia.org/wiki/Isbn
+ * @author The Whole Life To Learn <thewholelifetolearn@gmail.com>
@stof
stof Jan 12, 2013 Member

We don't put authorship annotations on test classes

@stloyd stloyd commented on an outdated diff Jan 12, 2013
...ony/Component/Validator/Constraints/IsbnValidator.php
+
+ if (!is_int($value)) {
+ $value = str_replace('-', '', $value);
+ }
+
+ if (10 == strlen($value)) {
+ for ($i = 0; $i < 10; $i++) {
+ if ($value[$i] == 'X') {
+ $validation += 10 * intval(10 - $i);
+ } else {
+ $validation += intval($value[$i]) * intval(10 - $i);
+ }
+ }
+
+ if ($validation % 11 == 0) {
+ $checkIsbn10 = true;
@stloyd
stloyd Jan 12, 2013 Contributor

You should push violation here and add return if null !== $constraint->isbn13. Same in next check.

@thewholelifetolearn
Contributor

@stloyd : Thank you for your remarks. I have taken them to account. But I don't like the new architecture of the validator. Too many nested if/elseif/else.

@stloyd stloyd commented on an outdated diff Jan 13, 2013
...ony/Component/Validator/Constraints/IsbnValidator.php
@@ -0,0 +1,89 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Validator\Constraints;
+use Symfony\Component\Validator\Constraint;
@stloyd
stloyd Jan 13, 2013 Contributor

Missing empty new line before this one.

@fabpot
Member
fabpot commented Mar 26, 2013

Does it belongs to Symfony core (I mean, that's not a validator that is really often needed)? So, for me, that's out of scope. What do you think @bschussek?

@thewholelifetolearn
Contributor

I do not know if it is often needed/used as I don't know all projects done with Symfony2 but it gives a choice more for developers. Plus, I don't see myself creating a new bundle just for this validator and it would be sad to have different validators for the same purpose.

@sprain
Contributor
sprain commented Apr 8, 2013

I refactored this validator and added it to SprainValidatorBundle. My idea is to make this a growing repo of validators which are not part of the Symfony2 core. Is this considered a good idea?

@thewholelifetolearn
Contributor

It can be a good idea to have an "unofficial" validator bundle but what are the characteristics for official validator?

@sprain
Contributor
sprain commented Apr 8, 2013

Good question … Any answers out there, @fabpot? What are the criterias whether something (like a new validator) should be included in the Symfony2 core?

@fabpot fabpot added a commit that referenced this pull request Apr 20, 2013
@fabpot fabpot merged branch thewholelifetolearn/master (PR #6718)
This PR was squashed before being merged into the master branch (closes #6718).

Discussion
----------

[Validator] Added ISBN validator

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
License of the code: MIT

For information about ISBN-10 and ISBN13: https://en.wikipedia.org/wiki/Isbn

This constraint permits to valid a ISBN-10 code, a ISBN-13 code or both on a value

Commits
-------

4582261 [Validator] Added ISBN validator
cf526ff
@fabpot fabpot added a commit that closed this pull request Apr 20, 2013
@fabpot fabpot merged branch thewholelifetolearn/master (PR #6718)
This PR was squashed before being merged into the master branch (closes #6718).

Discussion
----------

[Validator] Added ISBN validator

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
License of the code: MIT

For information about ISBN-10 and ISBN13: https://en.wikipedia.org/wiki/Isbn

This constraint permits to valid a ISBN-10 code, a ISBN-13 code or both on a value

Commits
-------

4582261 [Validator] Added ISBN validator
cf526ff
@fabpot fabpot closed this in cf526ff Apr 20, 2013
@fabpot
Member
fabpot commented Apr 20, 2013

@sprain: I've decided to merged both the ISBN and the IBAN validators. As you said, having to include a bundle just for some validators seems overkill, and bundling such validators does not add that much code to Symfony anyway.

@webmozart
Member

@fabpot 👍

@sprain
Contributor
sprain commented Apr 20, 2013

@fabpot Great, thanks! It still would be great to have some guidelines what kind of contributions are welcome and what should be handled in a third-party bundle.

@thewholelifetolearn
Contributor

@fabpot Thank you 👍. I would have written the documentation but I see it is already done. Thanks to you @Aitboudad :)
As @sprain said previously, it would be a good thing to know from which "level" the code can be merged into Symfony.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment