From: Kevin Israel <pleasestand@live.com>
Date: Tue, 24 Jun 2014 07:14:24 +0000 (-0400)
Subject: Special:BookSources: Correct validation of ISBNs containing X
X-Git-Tag: 1.31.0-rc.0~13733
X-Git-Url: http://git.cyclocoop.org/%24image?a=commitdiff_plain;h=19473fa625e173505ec0244710e0d30d0bcb0bdc;p=lhc%2Fweb%2Fwiklou.git

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 e5a6cd1a1b..e322007b3f 100644
--- 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 e6750e125e..f6b9d3312c 100644
--- 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
index 0000000000..d341ccf128
--- /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 ) );
+	}
+}