Posted
1 year ago
by [email protected]
— view source
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
Posted
1 year ago
by Nikita Popov
— view source
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 aE_DEPRECATEDnotice 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
Posted
1 year ago
by Pieter Hordijk
— view source
----- Original Message -----
From: "nikita ppv" [email protected]
To: "Pedro Magalhães" [email protected]
Cc: "internals" [email protected]
Sent: Tuesday, June 13, 2017 9:15:33 PM
Subject: Re: [PHP-DEV] [RFC] [VOTE] Restarted vote on the negative arrays
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 aE_DEPRECATEDnotice 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,
PedroAs 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
Posted
1 year ago
by [email protected]
— view source
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