Special:BookSources: Correct validation of ISBNs containing X

PHP's "equal" (==) operator considers the integer 0 to be equal to
the string 'X', and when 'X' is converted to a number, it becomes 0.
Neither is desired here.

* Fail when an X is encountered while calculating the check digit.
  (X can only occur as the check digit of an ISBN-10.)
* Fixed the check digit comparisons by adding an explicit string cast.
* Used the "identical" operator to make it more obvious that no type
  juggling should take place during the comparisons.
* Added some test cases.
* Removed an outdated TODO.

Bug: 67021
Change-Id: I85f53c41f403a60340e9441757fe66b9764e623c

diff --git a/RELEASE-NOTES-1.25 b/RELEASE-NOTES-1.25
index e5a6cd1..e322007 100644 (file)
--- a/RELEASE-NOTES-1.25
+++ b/RELEASE-NOTES-1.25
@@ -22,6 +22,8 @@ production.
 === Bug fixes in 1.25 ===
 * (bug 71003) No additional code will be generated to try to load CSS-embedded
   SVG images in Internet Explorer 6 and 7, as they don't support them anyway.
+* (bug 67021) On Special:BookSources, corrected validation of ISBNs (both
+  10- and 13-digit forms) containing "X".
 
 === Action API changes in 1.25 ===
 * (bug 65403) XML tag highlighting is now only performed for formats

diff --git a/includes/specials/SpecialBooksources.php b/includes/specials/SpecialBooksources.php
index e6750e1..f6b9d33 100644 (file)
--- a/includes/specials/SpecialBooksources.php
+++ b/includes/specials/SpecialBooksources.php
@@ -26,7 +26,6 @@
  * The parser creates links to this page when dealing with ISBNs in wikitext
  *
  * @author Rob Church <robchur@gmail.com>
- * @todo Validate ISBNs using the standard check-digit method
  * @ingroup SpecialPage
  */
 class SpecialBookSources extends SpecialPage {
@@ -73,7 +72,9 @@ class SpecialBookSources extends SpecialPage {
                $sum = 0;
                if ( strlen( $isbn ) == 13 ) {
                        for ( $i = 0; $i < 12; $i++ ) {
-                               if ( $i % 2 == 0 ) {
+                               if ( $isbn[$i] === 'X' ) {
+                                       return false;
+                               } elseif ( $i % 2 == 0 ) {
                                        $sum += $isbn[$i];
                                } else {
                                        $sum += 3 * $isbn[$i];
@@ -81,11 +82,14 @@ class SpecialBookSources extends SpecialPage {
                        }
 
                        $check = ( 10 - ( $sum % 10 ) ) % 10;
-                       if ( $check == $isbn[12] ) {
+                       if ( (string)$check === $isbn[12] ) {
                                return true;
                        }
                } elseif ( strlen( $isbn ) == 10 ) {
                        for ( $i = 0; $i < 9; $i++ ) {
+                               if ( $isbn[$i] === 'X' ) {
+                                       return false;
+                               }
                                $sum += $isbn[$i] * ( $i + 1 );
                        }
 
@@ -93,7 +97,7 @@ class SpecialBookSources extends SpecialPage {
                        if ( $check == 10 ) {
                                $check = "X";
                        }
-                       if ( $check == $isbn[9] ) {
+                       if ( (string)$check === $isbn[9] ) {
                                return true;
                        }
                }

diff --git a/tests/phpunit/includes/specials/SpecialBooksourcesTest.php b/tests/phpunit/includes/specials/SpecialBooksourcesTest.php
new file mode 100644 (file)
index 0000000..d341ccf
--- /dev/null
+++ b/tests/phpunit/includes/specials/SpecialBooksourcesTest.php
@@ -0,0 +1,36 @@
+<?php
+class SpecialBooksourcesTest extends MediaWikiTestCase {
+       public static function provideISBNs() {
+               return array(
+                       array( '978-0-300-14424-6', true ),
+                       array( '0-14-020652-3', true ),
+                       array( '020652-3', false ),
+                       array( '9781234567897', true ),
+                       array( '1-4133-0454-0', true ),
+                       array( '978-1413304541', true ),
+                       array( '0136091814', true ),
+                       array( '0136091812', false ),
+                       array( '9780136091813', true ),
+                       array( '9780136091817', false ),
+                       array( '123456789X', true ),
+
+                       // Bug 67021
+                       array( '1413304541', false ),
+                       array( '141330454X', false ),
+                       array( '1413304540', true ),
+                       array( '14133X4540', false ),
+                       array( '97814133X4541', false ),
+                       array( '978035642615X', false ),
+                       array( '9781413304541', true ),
+                       array( '9780356426150', true ),
+               );
+       }
+
+       /**
+        * @covers SpecialBooksources::isValidISBN
+        * @dataProvider provideISBNs
+        */
+       public function testIsValidISBN( $isbn, $isValid ) {
+               $this->assertSame( $isValid, SpecialBooksources::isValidISBN( $isbn ) );
+       }
+}