Re: cvs: php-src /ext/mcrypt mcrypt.c /ext/mcrypt/tests mcrypt_enc_self_test.phpt

Hannes Magnusson wrote: > On Sun, May 18, 2008 at 8:14 PM, David Soria Parra <dsoria@gmx.net> wrote: >> Hannes Magnusson wrote: >>> On Sun, May 18, 2008 at 7:15 PM, David Soria Parra <dsp@php.net> wrote: >>>> dsp Sun May 18 17:15:08 2008 UTC >>>> >>>> Modified files: >>>> /php-src/ext/mcrypt mcrypt.c >>>> /php-src/ext/mcrypt/tests mcrypt_enc_self_test.phpt >>>> Log: >>>> MFB: Make mcrypt_enc_self_test() return value compatible with documentation and >>>> mcrypt_module_self_test() >>>> >>>> >>>> >>>> http://cvs.php.net/viewvc.cgi/php-src/ext/mcrypt/mcrypt.c?r1=1.109&r2=1.110&diff_format=u >>>> Index: php-src/ext/mcrypt/mcrypt.c >>>> diff -u php-src/ext/mcrypt/mcrypt.c:1.109 php-src/ext/mcrypt/mcrypt.c:1.110 >>>> --- php-src/ext/mcrypt/mcrypt.c:1.109 Mon Dec 31 07:12:11 2007 >>>> +++ php-src/ext/mcrypt/mcrypt.c Sun May 18 17:15:08 2008 >>>> @@ -16,7 +16,7 @@ >>>> | Derick Rethans <derick@derickrethans.nl> | >>>> +----------------------------------------------------------------------+ >>>> */ >>>> -/* $Id: mcrypt.c,v 1.109 2007/12/31 07:12:11 sebastian Exp $ */ >>>> +/* $Id: mcrypt.c,v 1.110 2008/05/18 17:15:08 dsp Exp $ */ >>>> >>>> #ifdef HAVE_CONFIG_H >>>> #include "config.h" >>>> @@ -476,7 +476,12 @@ >>>> PHP_FUNCTION(mcrypt_enc_self_test) >>>> { >>>> MCRYPT_GET_TD_ARG >>>> - RETURN_LONG(mcrypt_enc_self_test(pm->td)); >>>> + >>>> + if (mcrypt_enc_self_test(pm->td) == 0) { >>>> + RETURN_TRUE; >>>> + } else { >>>> + RETURN_FALSE; >>>> + } >>>> } >>>> /* }}} */ >>>> >>>> >>>> http://cvs.php.net/viewvc.cgi/php-src/ext/mcrypt/tests/mcrypt_enc_self_test.phpt?r1=1.1&r2=1.2&diff_format=u >>>> Index: php-src/ext/mcrypt/tests/mcrypt_enc_self_test.phpt >>>> diff -u php-src/ext/mcrypt/tests/mcrypt_enc_self_test.phpt:1.1 >>>> php-src/ext/mcrypt/tests/mcrypt_enc_self_test.phpt:1.2 >>>> --- php-src/ext/mcrypt/tests/mcrypt_enc_self_test.phpt:1.1 Sat May 17 23:27:42 >>>> 2008 >>>> +++ php-src/ext/mcrypt/tests/mcrypt_enc_self_test.phpt Sun May 18 17:15:08 2008 >>>> @@ -7,4 +7,4 @@ >>>> $td = mcrypt_module_open(MCRYPT_RIJNDAEL_256, '', MCRYPT_MODE_CBC, >>>> ''); >>>> var_dump(mcrypt_enc_self_test($td)); >>>> --EXPECT-- >>>> -int(0) >>>> \ No newline at end of file >>>> +bool(true) >>> This is a massive BC break... >>> Imagine people who did if (mcrypt_enc_self_test($td) == 0) { ... } >>> that has now changed from dealing with success to failure. >>> >>> I'd say rather fix the docs >>> >>> -Hannes >> Yes I noticed that, that's why its not in 5.2. I think it's kind of >> "cleaning up" as >> mcrypt_module_self_test return a boolean too and the docs say it too. Maybe it's just >> in my opinion >> that the function itself is not that often used as it would be real issue, but if people >> have >> objections for sure we can revert that. > > The point is this is a very subtle change and not something people > will suspect when their applications stop working out of the blue, > without so much as a single warning. > A function rename would be obvious as it would give a fatal error, but > this is a return value change from false to true... > > IMO this should be reverted and the docs fixed. > reverted. > -Hannes