Uh oh!
There was an error while loading. Please reload this page.
Promote warnings to errors in array_flip() - #4582
Conversation
| } 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
nikic
commented
Aug 21, 2019
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. |
Girgias
commented
Aug 21, 2019
Should the warning message be amended in such a way to inform that theses values are skipped? |
nikic
commented
Aug 26, 2019
@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. |
| 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 ?
Girgias
commented
Aug 31, 2019
Closing in favour of #4661 |
Split from #4566