Re: [PHP-DEV] Re: [PHP-CVS] com php-src: ext/sodium: throw exceptions instead of errors: ext/sodium/libsodium.c

2017-11-28 Thread Remi Collet 
To summarize current situation

libsodium v2.0.9 pecl extension use E_WARNING

sodium extension in php-7.2.0 (tag) use E_WARNING
(as in previous RCs)

sodium extension in PHP-7.2 (branch) use E_WARNING
(all changes have been reverted)

Seems consistent.

For 7.2.0, I don't think we have to change this, especially as there is
no real consensus about the right thing to do.



Remi


P.S. delay before 7.2.1RC1 is very short (1 or 2 weeks)


-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] Re: [PHP-CVS] com php-src: ext/sodium: throw exceptions instead of errors: ext/sodium/libsodium.c

2017-11-28 Thread Kalle Sommer Nielsen 
2017-11-29 0:03 GMT+01:00 Nikita Popov :
> In summary, the first two commits combined were not a BC break -- they only
> removed a warning and made an error message nicer. After the third commit
> we now *do* have a BC break, because what was previously a SodiumException
> is now an E_ERROR.
>
> I hope we can restore some sanity to this world by reverting that revert...

All in all, E_ERROR should *never* be used by an extension, we should
revert back to using an Exception and re-tag the 7.2.0 release asap


-- 
regards,

Kalle Sommer Nielsen
ka...@php.net

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] Re: [PHP-CVS] com php-src: ext/sodium: throw exceptions instead of errors: ext/sodium/libsodium.c

2017-11-28 Thread Nikita Popov 
So
https://github.com/php/php-src/commit/31d221f9c72f0d0322c84907c5d89a4464667244
landed just now to revert this change, per the request here. This leaves us
with the worst variant... so, let's step back a bit:

The main commit that landed (for 7.2.1) is
https://github.com/php/php-src/commit/c219991c77e4c68f7d62963e18a1da778de0bbe0#diff-563df88ede8d2a03e291599c46952c13.
This commit *removes* checks that certain variables are below an
interactive (read: recommended) level, in which case a warning was thrown.
This resolves bug #75572. These checks were replaced with checks against
minimum values, in which case an E_ERROR is thrown. Note that these
minimums are *not* recommendations. They are hard limits imposed by the
used algorithms. The libsodium functions will *already* fail if you chose
parameters below the limits (see e.g.
https://github.com/jedisct1/libsodium/blob/d49d7e8d4f4dd8df593beb9e715e7bc87bc74108/src/libsodium/crypto_pwhash/argon2/pwhash_argon2i.c#L160).
The only thing that happened here is that these errors now get a nice,
explicit error message, instead of a generic error.

The second commit is
https://github.com/php/php-src/commit/c05cbd1e775fa69ed9939796a908390f2bfb4459,
which is what started this thread. This commit changes the E_ERRORs
introduced in the previous commit (not part of any release) to be
SodiumExceptions, because that's what ext/sodium does everywhere else.

Now
https://github.com/php/php-src/commit/31d221f9c72f0d0322c84907c5d89a4464667244
landed, which reverts that change again, based on the request here.

To summarize:

Previously:  wrote:

> On Tue, Nov 28, 2017 at 1:17 PM, Sebastian Bergmann 
> wrote:
>
> > Am 28.11.2017 um 13:56 schrieb Frank Denis:
> > > Commit:c05cbd1e775fa69ed9939796a908390f2bfb4459
> > > Author:Frank Denis  Tue, 28 Nov 2017
> > 13:56:11 +0100
> > > Parents:   c219991c77e4c68f7d62963e18a1da778de0bbe0
> > > Branches:  PHP-7.2
> > >
> > > Link:   http://git.php.net/?p=php-src.git;a=commitdiff;h=
> > c05cbd1e775fa69ed9939796a908390f2bfb4459
> > >
> > > Log:
> > > ext/sodium: throw exceptions instead of errors
> >
> > I am very much in favor of this change. However, making such a change in
> > PHP 7.2.1 is a BC break. Any chance to get that into PHP 7.2.0?
> >
> >
>  I agree that this is a BC break in bug fixing release and should be
> reverted from PHP-7.2 IMO and kept in master only (whatever the change is -
> error or exception...)
>
> CC'd Frank...
>
> Cheers
>
> Jakub
>

Re: [PHP-DEV] Re: [PHP-CVS] com php-src: ext/sodium: throw exceptions instead of errors: ext/sodium/libsodium.c

2017-11-28 Thread Jakub Zelenka 
On Tue, Nov 28, 2017 at 1:17 PM, Sebastian Bergmann 
wrote:

> Am 28.11.2017 um 13:56 schrieb Frank Denis:
> > Commit:c05cbd1e775fa69ed9939796a908390f2bfb4459
> > Author:Frank Denis  Tue, 28 Nov 2017
> 13:56:11 +0100
> > Parents:   c219991c77e4c68f7d62963e18a1da778de0bbe0
> > Branches:  PHP-7.2
> >
> > Link:   http://git.php.net/?p=php-src.git;a=commitdiff;h=
> c05cbd1e775fa69ed9939796a908390f2bfb4459
> >
> > Log:
> > ext/sodium: throw exceptions instead of errors
>
> I am very much in favor of this change. However, making such a change in
> PHP 7.2.1 is a BC break. Any chance to get that into PHP 7.2.0?
>
>
 I agree that this is a BC break in bug fixing release and should be
reverted from PHP-7.2 IMO and kept in master only (whatever the change is -
error or exception...)

CC'd Frank...

Cheers

Jakub

[PHP-DEV] Re: [PHP-CVS] com php-src: ext/sodium: throw exceptions instead of errors: ext/sodium/libsodium.c

2017-11-28 Thread Sebastian Bergmann 
Am 28.11.2017 um 13:56 schrieb Frank Denis:
> Commit:c05cbd1e775fa69ed9939796a908390f2bfb4459
> Author:Frank Denis  Tue, 28 Nov 2017 
> 13:56:11 +0100
> Parents:   c219991c77e4c68f7d62963e18a1da778de0bbe0
> Branches:  PHP-7.2
> 
> Link:   
> http://git.php.net/?p=php-src.git;a=commitdiff;h=c05cbd1e775fa69ed9939796a908390f2bfb4459
> 
> Log:
> ext/sodium: throw exceptions instead of errors

I am very much in favor of this change. However, making such a change in
PHP 7.2.1 is a BC break. Any chance to get that into PHP 7.2.0?

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php