Converting some warnings to errors in file.c #5007
Conversation
Girgias
force-pushed
the
file-func-warnings-to-errors
branch
3 times, most recently
from
7cfb206 to
3eb1bc7
Compare
| php_error_docref(NULL, E_WARNING, "delimiter must be a character"); | ||
| RETURN_FALSE; | ||
| zend_value_error("delimiter must be a character"); | ||
| return; | ||
| } else if (delimiter_str_len > 1) { | ||
| php_error_docref(NULL, E_NOTICE, "delimiter must be a single character"); |
There was a problem hiding this comment.
I think it's important that we keep these pairs of warnings symmetrical. Either both should throw or neither.
There was a problem hiding this comment.
Symmetrical with the SPL ones right? I'll do the changes at the same time borrowing parts from #4991
There was a problem hiding this comment.
Symmetrical with the error above. I now realize though that in this case we throw a warning but still continue running the function, ugh. I think @cmb69's suggestion above to make this a single check (with BC break) may make the most sense.
There was a problem hiding this comment.
Ah ok, want me to make that part of this PR or seperate one?
| @@ -1831,8 +1831,8 @@ PHP_FUNCTION(fputcsv) | |||
| if (delimiter_str != NULL) { | |||
| /* Make sure that there is at least one character in string */ | |||
| if (delimiter_str_len < 1) { | |||
| php_error_docref(NULL, E_WARNING, "delimiter must be a character"); | |||
| RETURN_FALSE; | |||
| zend_value_error("delimiter must be a character"); | |||
There was a problem hiding this comment.
(Drive-by) This message is somewhat confusing, maybe "delimiter cannot be empty"?
There was a problem hiding this comment.
That message would better reflect the check, but actually only the first char is ever used as delimiter; maybe it's worth introducing a pontetial BC break here, and check for delimiter_str_len == 1)? Same for enclosure a few lines below.
There was a problem hiding this comment.
Another possibility would be to actually add support for multiple chars (effectively: Unicode support).
There was a problem hiding this comment.
Another possibility would be to actually add support for multiple chars (effectively: Unicode support).
Tried to have a look, it seems that the whole fgetcsv and fputcsv sub-routines need to be rewritten. I don't mind either way to merge the checks or have them adapted to have multibyte support. But I feel this should rather be in a separate PR.
Girgias
force-pushed
the
file-func-warnings-to-errors
branch
from
3eb1bc7 to
9c9b4d9
Compare
|
Azure pipeline failures are unrelated (sapi/fpm/tests/reload-uses-sigkill-as-last-measure.phpt) |
Girgias
force-pushed
the
file-func-warnings-to-errors
branch
from
9c9b4d9 to
18f25a3
Compare
| @@ -3,8 +3,11 @@ Bug #71882 (Negative ftruncate() on php://memory exhausts memory) | |||
| --FILE-- | |||
| <?php | |||
| $fd = fopen("php://memory", "w+"); | |||
| var_dump(ftruncate($fd, -1)); | |||
| try { | |||
| var_dump(ftruncate($fd, -1)); | |||
There was a problem hiding this comment.
All the var_dump()s in try-catch blocks could be removed as it's sure that an exception is thrown before executing them. At least this is how I prefer to do it, so feel free to ignore. Otherwise, the PR looks good for me :)
There was a problem hiding this comment.
The reason I don't like removing them is if we remove the Error/Exception you need to go back up and add it to make sure that the behaviour is expected.
Also everything is just "standard". Moreover, this can be removed after the fact.
|
As it seems the only thing holding this up is the debate on how to handle the non symmetric warnings/errors in So if there are no further objections I'll merge this in a week. |
|
Yes, this is fine to merge. fputcsv() etc changes are not really related. |
Small intermediate step after #4991
Also removed some test which were basically testing erroneous values in fgetcsv, maybe I should do the same for fputcsv?