Promote warnings to errors in array_flip() #4582
Conversation
| $trans = array_flip($trans); | ||
| } catch (\TypeError $e) { | ||
| echo $e->getMessage() . "\n"; | ||
| } | ||
| var_dump($trans); |
There was a problem hiding this comment.
this var_dump can be removed - as well as the EXPECT output. When an exception occurs the array is not modified - this is known behavior and doesn't need testing as part of this change
|
Here similar to the array_count_values() case, I'm not totally sure if this is appropriate as it currently doesn't return an error value: It just skips values that can't be flipped. |
Should the warning message be amended in such a way to inform that theses values are skipped? |
|
@Girgias Changing the error message to make it clear that values are skipped sounds like a good idea. Apart from that I don't think it's a big loss to keep this as a warning, because it doesn't impact the return type. |
| @@ -4444,7 +4444,8 @@ PHP_FUNCTION(array_flip) | |||
| } | |||
| zend_symtable_update(Z_ARRVAL_P(return_value), Z_STR_P(entry), &data); | |||
| } else { | |||
| php_error_docref(NULL, E_WARNING, "Can only flip STRING and INTEGER values!"); | |||
| zend_type_error("Can only flip STRING and INTEGER values!"); | |||
There was a problem hiding this comment.
We should also drop the ALLCAPS and the exclamation mark ... this is a very aggressive warning ;)
There was a problem hiding this comment.
@nikic I actually noticed that too and there are more exclamation marks which can be removed. I think a separate PR is better
There was a problem hiding this comment.
Should I do the same for array_count_values and group both of them in a single PR ?
|
Closing in favour of #4661 |
Split from #4566