array_change_key_case(): Throw ValueError on invalid argument#15883
array_change_key_case(): Throw ValueError on invalid argument#15883Girgias merged 1 commit intophp:masterfrom
Conversation
cmb69
left a comment
cmb69
left a comment
There was a problem hiding this comment.
Thank you! Looks good. Also nice to get of the contrived test cases.
|
Do we need RFC for such changes? I have similar PR in #15647 |
|
I don't think such changes require an RFC (maybe in theory, but barely practical). Given #16641, we may consider to be more conservative, and deprecate passing anything but |
|
I think we should maybe put a general RFC how to deal with this. My preference is really to go through deprecations. I don't think we need to do RFC for each case but having some agreement (e.g. or possibly a single RFC) would be good. |
|
Btw. if we agree that deprecations are better for those case, I don't think we need RFC. |
|
Those should be warnings not deprecations if we are not turning them into ValueErrors. |
|
I would turn them into |
Girgias
force-pushed
the
array_key_change_error
branch
from
5636b64 to
8355060
Compare
| if (change_to_upper == PHP_CASE_UPPER) { | ||
| new_key = zend_string_toupper(string_key); | ||
| } else { | ||
| new_key = zend_string_tolower(string_key); | ||
| } |
There was a problem hiding this comment.
A switch() might potentially be clearer here.
|
Should this be added to NEWS/UPGRADING? |
Done, also fixed UPGRADING. Because once again some of the ValueErrors where in "changed function"... |
6 participants
This should be merged after 8.4 has been cut.