Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-28 Thread Tim Düsterhus
Hi On 10/28/22 18:25, Jeffrey Dafoe wrote: Informally, doesn't this mean that voters wanted a and c in PHP9? Not necessarily. It might be that a voter does not like the Exception wrapping at all (thus declining the proposal), but *if* it happens to be accepted then also increasing

Re: RE: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-28 Thread Christoph M. Becker
On 28.10.2022 at 18:25, Jeffrey Dafoe wrote: >> a) "Add the \UnserializationFailedException and wrap any Throwables in PHP >> 8.x?" >> >> This proposal was declined with 20 (Yes) to 12 (No) Votes (62.5%). >> >> b) "Increase the severity of emitted E_NOTICE to E_WARNING in PHP 8.x?" >> >> This

RE: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-28 Thread Jeffrey Dafoe
> a) "Add the \UnserializationFailedException and wrap any Throwables in PHP > 8.x?" > > This proposal was declined with 20 (Yes) to 12 (No) Votes (62.5%). > > b) "Increase the severity of emitted E_NOTICE to E_WARNING in PHP 8.x?" > > This proposal was accepted with 33 (Yes) to 2 (No) Votes

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-28 Thread Tim Düsterhus
Hi On 10/14/22 15:44, Tim Düsterhus wrote: Voting will run 2 weeks until: 2022-10-28 at 14:00 UTC Voting just closed. The RFC was partly accepted: a) "Add the \UnserializationFailedException and wrap any Throwables in PHP 8.x?" This proposal was declined with 20 (Yes) to 12 (No) Votes

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-27 Thread Tim Düsterhus
Hi On 10/14/22 15:44, Tim Düsterhus wrote: Voting will run 2 weeks until: 2022-10-28 at 14:00 UTC As a heads up: Voting closing tomorrow, in about 30.5 hours. Best regards Tim Düsterhus -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit:

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-20 Thread Dan Ackroyd
On Wed, 19 Oct 2022 at 19:11, Tim Düsterhus wrote: > > While the behavior would in fact change, it would not introduce a > "security issue" if this what you were hinting at. No, just that it would break in production, and then have to be fixed after a live site was already affecting end-users.

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-19 Thread Tim Düsterhus
Hi On 10/19/22 06:14, Tim Starling wrote: The example in the RFC was written to be correct for the general case, without imposing any constraints in the input data. As I said, if throwing was optional, like in json_decode(), I would be all for it. I'm just doing a cost/benefit analysis. The

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-19 Thread Tim Düsterhus
Hi On 10/18/22 14:04, Dan Ackroyd wrote: function getUserPreferences($data) { $user_preferences = @unserialize($data); if ($user_preferences === false) { $user_preferences = get_default_preferences(); } return $user_preferences; } […] I agree that this implementation

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-18 Thread Tim Starling
On 18/10/22 22:32, Tim Düsterhus wrote: false is a valid value that might've been serialized. The manual specifically notes:  false is returned both in the case of an error and if unserializing the serialized false value. It is possible to catch this special case by comparing data with

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-18 Thread Nicolas Grekas
> > The other failing tests involve "unserialize_callback_func" to gracefully > > detect class-not-found, and they are all plain broken by the RFC. > > > > I created this patch/PR to show the changes that would be required on > > Symfony to work around the BC break: > >

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-18 Thread Dan Ackroyd
Hi Tim, On Fri, 14 Oct 2022 at 15:44, Tim Düsterhus wrote: > > Hi > > as announced on Wednesday [1] I've now opened the vote for: > > "Improve unserialize() error handling" [2] I'm going to have to vote no for changing to throwing an exception. The BC break is too big imo, and too hard for

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-18 Thread Tim Düsterhus
Hi On 10/18/22 03:26, Tim Starling wrote: If I'm reading the implementation correctly, the original exception is thrown away, there's no way to get the original message and backtrace. That will make debugging more difficult. See Derick's reply (and my reply to Derick's reply). I reviewed

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-18 Thread Tim Düsterhus
Hi On 10/18/22 09:32, Derick Rethans wrote: If I'm reading the implementation correctly, the original exception is thrown away, there's no way to get the original message and backtrace. That will make debugging more difficult. I believe that is not true. It should be wrapped and accessible

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-18 Thread Derick Rethans
On 18 October 2022 02:26:59 BST, Tim Starling wrote: > If I'm reading the implementation correctly, the original exception > is thrown away, there's no way to get the > original message and backtrace. > That will make debugging more difficult. I believe that is not true. It should be wrapped

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-17 Thread Mike Schinkel
Larry, > On Oct 17, 2022, at 6:01 PM, Larry Garfield wrote: > > On Mon, Oct 17, 2022, at 12:33 PM, Tim Düsterhus wrote: > Okay, now the Exception message changed. Personally I do not consider this a BC break: I believe Exception messages are meant for human consumption, not for

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-17 Thread Tim Starling
I'm going to vote against exception wrapping. If I'm reading the implementation correctly, the original exception is thrown away, there's no way to get the original message and backtrace. That will make debugging more difficult. I reviewed about 150 callers of unserialize() in the MediaWiki

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-17 Thread Larry Garfield
On Mon, Oct 17, 2022, at 12:33 PM, Tim Düsterhus wrote: >>> Okay, now the Exception message changed. Personally I do not consider >>> this a BC break: I believe Exception messages are meant for human >>> consumption, not for programs. Otherwise fixing a typo in the message >>> would be a BC

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-17 Thread Jordan LeDoux
Sorry for double send Nicolas, I hit reply instead of reply-all on my first message. :) On Mon, Oct 17, 2022 at 1:57 AM Nicolas Grekas wrote: > > Yes, the specific error message should be part of the BC promise. This > allows building test suites that can assert the message in a stable way. >

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-17 Thread Tim Düsterhus
Hi On 10/17/22 10:57, Nicolas Grekas wrote: Thanks for taking the time to have a closer look. Unfortunately, you picked the one failing test where there could be a discussion about whether the original code needs improvement or not. I disagree. The other locations are equally broken with

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-17 Thread Peter Bowyer
On Mon, 17 Oct 2022 at 09:57, Nicolas Grekas wrote: > I created this patch/PR to show the changes that would be required on > Symfony to work around the BC break: > https://github.com/symfony/symfony/pull/47882 > > Note to readers: in this whole discussion, Symfony is just an example of >

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-17 Thread Nicolas Grekas
Hi Tim, Thanks for taking the time to have a closer look. Unfortunately, you picked the one failing test where there could be a discussion about whether the original code needs improvement or not. The other failing tests involve "unserialize_callback_func" to gracefully detect class-not-found,

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-15 Thread Tim Düsterhus
Hi Resending the message without attachments, because the mailing list rejected the original due to exceeding 3 bytes: ezmlm-reject: fatal: Sorry, I don't accept messages larger than 3 bytes (#5.2.3) You can find the attachments at:

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-15 Thread juan carlos morales
El sáb., 15 de octubre de 2022 11:02, juan carlos morales < dev.juan.mora...@gmail.com> escribió: > As far as I read... Everyone is right. All agree is BC break change, but > this is being considered to be part of PHP 9.0, so if what We get is a > better PHP then We must move forward, and

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-15 Thread juan carlos morales
As far as I read... Everyone is right. All agree is BC break change, but this is being considered to be part of PHP 9.0, so if what We get is a better PHP then We must move forward, and such steps should be done in a major version change.

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-15 Thread Christoph M. Becker
On 15.10.2022 at 11:06, Nicolas Grekas wrote: >>> I'm therefore voting NO on the proposal. >> >> I'm not surprised by the “no” on the first vote based on the previous >> discussion. I am surprised however that you also disagree with raising >> the E_NOTICE to E_WARNING for consistency. > > Since

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-15 Thread Marco Pivetta
Tbh, this affects a new minor and a new major, and only in an unhappy path scenario, where the current PHP API is bad. PHP 9 will introduce a hard crash: good. It's a BC break: yes, native de-serialization is one of the most unsafe parts of the language, and it requires hardening. The fact that

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-15 Thread Nicolas Grekas
> > Not sure why I didn't think about it before but I just ran the test suite > > of Symfony after applying the patch proposed in the RFC to change the way > > exceptions are handled by unserialize. > > > > This change breaks the test suite of 5 separate components. I created > this > > gist to

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-14 Thread Tim Düsterhus
Hi On 10/14/22 22:42, Nicolas Grekas wrote: Not sure why I didn't think about it before but I just ran the test suite of Symfony after applying the patch proposed in the RFC to change the way exceptions are handled by unserialize. This change breaks the test suite of 5 separate components. I

Re: [PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-14 Thread Nicolas Grekas
Hi Tim, as announced on Wednesday [1] I've now opened the vote for: > > "Improve unserialize() error handling" [2] > > The RFC contains three votes, each of which requires a 2/3 majority. Two > of the votes are for 8.x (8.3), one for 9.0. > > Voting will run 2 weeks until: > > 2022-10-28 at 14:00

[PHP-DEV] [VOTE] Improve unserialize() error handling

2022-10-14 Thread Tim Düsterhus
Hi as announced on Wednesday [1] I've now opened the vote for: "Improve unserialize() error handling" [2] The RFC contains three votes, each of which requires a 2/3 majority. Two of the votes are for 8.x (8.3), one for 9.0. Voting will run 2 weeks until: 2022-10-28 at 14:00 UTC