Fix GH-20189: Tempnam notice message incorrect #20191
Conversation
The notice was outputted before the creation of the file even took place. Therefore, if the creation of the file fails, the notice is outputted while it shouldn't have been. This changes one test which indeed suffered from this issue due to the open_basedir restriction.
The function returned false, this should've never been outputted in the first place.
| if (fd == -1) { | ||
| /* Use default temporary directory. */ | ||
| if (!(flags & PHP_TMP_FILE_SILENT)) { | ||
| php_error_docref(NULL, E_NOTICE, "file created in the system's temporary directory"); |
There was a problem hiding this comment.
Alternatively perhaps just:
file will be created […]
Looking at ext/standard/tests/file/bug52624.phpt folks might otherwise be confused why the final error message mentions /tmp.
There was a problem hiding this comment.
I see what you mean. I'm undecided yet, hence also the TODO I put
There was a problem hiding this comment.
I don't think we should change the text completely (even for successful fallback creation) because some people can ignore error message by text in error handler (I saw that in some legacy code in past) so we don't want to break that (or more minimise chance of breaking it) if the target is 8.3. But different message in that TODO bit would make sense.
bukka
left a comment
There was a problem hiding this comment.
The fix makes sense except maybe that TODO bit...
| if (UNEXPECTED(fd == -1)) { | ||
| /* TODO: decide whether we should notice that even the fallback failed? */ | ||
| } else { |
There was a problem hiding this comment.
I just checked and all callers trigger their own warning if the function fails so I think you should remove this TODO. Also if we really wanted such warning / notice, it should happen for non fallback as well...
There was a problem hiding this comment.
Ah I see what you meant of keep that message after reading Tim's comment. I guess maybe that makes sense to add a noticed so people don't get confused actually.
The notice was outputted before the creation of the file even took place. Therefore, if the creation of the file fails, the notice is outputted while it shouldn't have been.
This changes one test which indeed suffered from this issue due to the open_basedir restriction.