Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 0.8.5

Bug #10167 ISSN/ISBN validation is broken by _checkControlNumber
Submitted: 2007-02-23 11:11 UTC Modified: 2007-04-18 18:16 UTC
From: ash at hexmen dot com Assigned: dufuz
Status: Closed Package: Validate (version CVS)
PHP Version: Irrelevant OS:
Roadmaps: 0.8.0    
Subscription  


 [2007-02-23 11:11 UTC] ash at hexmen dot com (Ash Searle)
Description: ------------ The Validate_ISPN package relies on _checkControlNumber from Validate - which contains a bug... The 'allow_high' parameter passed from _checkControlNumber to _getControlNumber is 'true' if the last character of the passed in $number is 'X' and false otherwise - this is bad logic. Instead, the 'allow_high' parameter should be 'true' if the modulo is greater than 10 - regardless of what $number is passed in. Note: with correct logic, the 'allow_high' parameter is redundant, as it can be determined within the function itself. A proper-patch with test-cases is available at: http://hexmen.com/code/patches/pear-2879.diff (this bug is probably related to bug 2879, hence the numbering) A one-line patch is below: Index: ../Validate.php =================================================================== RCS file: /repository/pear/Validate/Validate.php,v retrieving revision 1.107 diff -u -r1.107 Validate.php --- ../Validate.php 9 Jan 2007 20:26:00 -0000 1.107 +++ ../Validate.php 23 Feb 2007 16:04:21 -0000 @@ -715,7 +715,7 @@ return false; } $target_digit = substr($number, count($weights), 1); - $control_digit = Validate::_getControlNumber($number, $weights, $modulo, $subtract, $target_digit === 'X'); + $control_digit = Validate::_getControlNumber($number, $weights, $modulo, $subtract, $modulo > 10); if ($control_digit == -1) { return false; Test script: --------------- echo Validate_ISPN::issn('1044-789X') ? "YES\n" : "NO\n"; echo Validate_ISPN::issn('1044-7890') ? "YES\n" : "NO\n"; echo Validate_ISPN::isbn('073571410X') ? "YES\n" : "NO\n"; echo Validate_ISPN::isbn('0735714100') ? "YES\n" : "NO\n"; Expected result: ---------------- YES NO YES NO Actual result: -------------- YES YES YES YES

Comments

 [2007-03-27 16:43 UTC] dufuz (Helgi Þormar)
Those isbn numbers you give in your test script, did you obtain it from a book or some place on the internet you could link me to ?
 [2007-03-30 01:54 UTC] ash at hexmen dot com
The valid ISSN number 1044-789X is for Dr Dobbs Journal. As the last-digit is a check-digit, it's unique. So, 1044-7890 is an invalid ISSN number. Similarly, the valid ISBN number is 073571410X, and the invalid one is 0735714100. The book is 'Defensive Design for the Web...' http://www.amazon.com/o/ASIN/073571410X
 [2007-04-18 17:55 UTC] dufuz (Helgi Þormar)
I just tried your patch and just about every single test started to fail for me, did you try running the whole test suite ? pear run-tests -p Validate_ISPN after you did the change Let me know how that goes for you.
 [2007-04-18 18:05 UTC] dufuz (Helgi Þormar)
Sorry about that, things seem to run perfectly, what I saw was just my imagination playing tricks with me (with a combination of mixing up the global pear dir and my local test dir ;-))
 [2007-04-18 18:16 UTC] dufuz (Helgi Þormar)
This bug has been fixed in CVS. If this was a documentation problem, the fix will appear on pear.php.net by the end of next Sunday (CET). If this was a problem with the pear.php.net website, the change should be live shortly. Otherwise, the fix will appear in the package's next release. Thank you for the report and for helping us make PEAR better.