Re: cvs: php-src /ext/mcrypt mcrypt.c /ext/mcrypt/tests mcrypt_enc_self_test.phpt
| From: | Hannes Magnusson | Date: | Sun, 18 May 2008 19:27:07 +0000 |
| Subject: | Re: cvs: php-src /ext/mcrypt mcrypt.c /ext/mcrypt/tests mcrypt_enc_self_test.phpt | ||
| References: | 1 2 3 | Groups: | php.cvs php.internals |
| Request: | Send a blank email to internals+get-37707@lists.php.net to get a copy of this message | ||
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.
-Hannes