[RFC] [VOTE] Restarted vote on the negative arrays

php.internals

Pedro Magalhães

9 years ago
Hi internals, Based on the feedback on the original RFC targeting 7.2, I have now updated the RFC to target version 8.0. Additionally, I changed the existing PR to throw a E_DEPRECATED notice on the cases where this RFC will cause a BC break when the change is implemented to ease the detection of such cases. I have closed the current vote (YES 14 - 16 NO) and opened a new one for the revised RFC targeting the next major version. You can vote (again) at https://wiki.php.net/rfc/negative_array_index until 28/6/2017 19:00 UTC. Regards, Pedro

Nikita Popov

9 years ago
On Tue, Jun 13, 2017 at 8:44 PM, Pedro Magalhães <mail@pmmaga.net> wrote:
> Hi internals, > > Based on the feedback on the original RFC targeting 7.2, I have now updated > the RFC to target version 8.0. Additionally, I changed the existing PR to > throw a E_DEPRECATED notice on the cases where this RFC will cause a BC > break when the change is implemented to ease the detection of such cases. > > I have closed the current vote (YES 14 - 16 NO) and opened a new one for > the revised RFC targeting the next major version. > > You can vote (again) at https://wiki.php.net/rfc/negative_array_index > until 28/6/2017 19:00 UTC. > > Regards, > Pedro >
As this is a significant change to the RFC, I'd recommend moving it back to discussion first. Without some further evaluation, I am not sure whether throwing a deprecation from a low level hash API function like this is safe. The deprecation may be converted into an exception and it's not immediately clear that calling code will handle this correctly. In any case, even assuming this is safe, what is the expected behavior if the deprecation notice is converted into an exception? As the implementation stands right now, the element will still be inserted into the array. Is this intentional? ("We don't care" is also a valid answer -- it's an edge case.) It would also be nice to quickly check what the performance impact of this change is (a micro-benchmark on array appends should be enough). The patch adds a number of additional checks in a very hot code-path, which may or may not have a measurable impact. Regards, Nikita

Pieter Hordijk

9 years ago
----- Original Message -----
> From: "nikita ppv" <nikita.ppv@gmail.com> > To: "Pedro Magalhães" <mail@pmmaga.net> > Cc: "internals" <internals@lists.php.net> > Sent: Tuesday, June 13, 2017 9:15:33 PM > Subject: Re: [PHP-DEV] [RFC] [VOTE] Restarted vote on the negative arrays
> On Tue, Jun 13, 2017 at 8:44 PM, Pedro Magalhães <mail@pmmaga.net> wrote: > >> Hi internals, >> >> Based on the feedback on the original RFC targeting 7.2, I have now updated >> the RFC to target version 8.0. Additionally, I changed the existing PR to >> throw a E_DEPRECATED notice on the cases where this RFC will cause a BC >> break when the change is implemented to ease the detection of such cases. >> >> I have closed the current vote (YES 14 - 16 NO) and opened a new one for >> the revised RFC targeting the next major version. >> >> You can vote (again) at https://wiki.php.net/rfc/negative_array_index >> until 28/6/2017 19:00 UTC. >> >> Regards, >> Pedro >> > > As this is a significant change to the RFC, I'd recommend moving it back to > discussion first. > > Without some further evaluation, I am not sure whether throwing a > deprecation from a low level hash API function like this is safe. The > deprecation may be converted into an exception and it's not immediately > clear that calling code will handle this correctly. In any case, even > assuming this is safe, what is the expected behavior if the deprecation > notice is converted into an exception? As the implementation stands right > now, the element will still be inserted into the array. Is this intentional? > ("We don't care" is also a valid answer -- it's an edge case.) > > It would also be nice to quickly check what the performance impact of this > change is (a micro-benchmark on array appends should be enough). The patch > adds a number of additional checks in a very hot code-path, which may or > may not have a measurable impact. > > Regards, > Nikita
My concern is that valid code which is working fine and will continue working fine with the change described in the rfc will now start spitting out notices. How often and how many? I don't know. On top of that I agree with Nikita about the possibility of conversion to exceptions which will (/ might) actually break existing code. Pieter

Pedro Magalhães

9 years ago
> > > As this is a significant change to the RFC, I'd recommend moving it back > to > > discussion first. >
I have suspended the vote and will be resumed after we discuss the deprecation notice.
> > Without some further evaluation, I am not sure whether throwing a > > deprecation from a low level hash API function like this is safe. The > > deprecation may be converted into an exception and it's not immediately > > clear that calling code will handle this correctly. In any case, even > > assuming this is safe, what is the expected behavior if the deprecation > > notice is converted into an exception? As the implementation stands right > > now, the element will still be inserted into the array. Is this > intentional? > > ("We don't care" is also a valid answer -- it's an edge case.) >
It seems to be handled correctly. However, the current PR may still change if there are any issues with it.
> > It would also be nice to quickly check what the performance impact of > this > > change is (a micro-benchmark on array appends should be enough). The > patch > > adds a number of additional checks in a very hot code-path, which may or > > may not have a measurable impact. >
Running the relevant parts of Zend/bench.php on master and the PR branch indicates that it shouldn't have a serious on performance.
> My concern is that valid code which is working fine and will continue > working > fine with the change described in the rfc will now start spitting out > notices. > > How often and how many? I don't know. >
That's a very valid concern. I believe that the benefit outweighs the potential noise but, if most people disagree, there are other options like waiting until the last 7.x or not introducing it at all. In this case I would like to hear some more feedback on the usefulness of the deprecation notice. If necessary, I can open a second vote for the deprecation notice keeping the first one only about the main point of the RFC. Regards, Pedro