Validate uploaded filename only if no upload error occured. #6423
Conversation
Most file upload errors (in particular UPLOAD_ERR_NO_FILE) yield an empty filename. The UploadFile validator does not report the original error in that case, but FILE_NOT_FOUND instead. This fix validates the filename only in case of UPLOAD_ERR_OK. Any other error code will be reported correctly.
|
There are changes in the tests that I didn't notice before. Marking as BC Break and downgrading set milestone to 2.4 |
|
I disagree on the BC break. The PR fixes a bug introduced in 606f28a. Before that commit, any PHP upload errors (like UPLOAD_ERR_NO_FILE) got checked and reported with their own validation message. These errors are typically accompanied by an empty string as filename because there is no file. The faulty commit added an extra check on the filename which reports UploadFile::FILE_NOT_FOUND if the file does not exist. The problem is that this check is done before the PHP error code is evaluated. Since we have an empty string as filename in case of an upload error, we now get a meaningless UploadFile::FILE_NOT_FOUND message instead of the correct message like UploadFile::NO_FILE. The code that reports these messages is still there, but it never gets called because the filename check incorrectly takes precedence. There is no point in evaluating the filename if no file got uploaded, for reasons reported by the PHP error code. Along with the commit came a new test testEmptyFileArrayShouldReturnFalseRatherThanTriggerError(), which later got renamed to testEmptyFileShouldReturnFalseAndDisplayNotFoundMessage(). This test is broken too. It sets up a test case with UPLOAD_ERR_NO_FILE and an empty filename, and then asserts that UploadFile::FILE_NOT_FOUND is reported. The test passes, but only because it tests broken behavior. The correct message should be UploadFile::NO_FILE. I changed the test to assert UploadFile::FILE_NOT_FOUND only in case of no error. The other test case with UPLOAD_ERR_NO_FILE got put into a new test which checks for UploadFile::NO_FILE. To summarize: it's not a BC break, but a fix for recently broken behavior and broken tests. |
|
Still changes behavior on validation errors being produced, and therefore can't land in |
|
It's a fix for a regression introduced in PR #5555, first released in ZF 2.2.6. That PR addresses an issue in all file validators, and except for UploadFile, that change makes sense. So this unintentional change in behavior is an oversight that should never have made it into a release. Yes, my PR changes behavior (that's the nature of every bugfix) - but so does the mentioned faulty commit, because it never yields anything other than FILE_NOT_FOUND. If the breakage was introduced in a minor release, couldn't it be fixed in one? |
|
@Ocramius I'm inclined to agree with @hschletz on this one. The point here is that the validation message was actually incorrect previously, as another file upload error may have been the reason for the file not being present or accessible. In addition, we should not have attempted to resolve the file until we knew the file upload status was ok. I'm marking for 2.3.2 -- the original fix was done during a patch release and introduced a change in validation behavior as well -- to correct an issue. This is the same situation. I'll make sure to point it out in the release notes. |
…ptyFileCheck Validate uploaded filename only if no upload error occured.
…oadErrorCodeShouldPrecedeEmptyFileCheck Validate uploaded filename only if no upload error occured.
Most file upload errors (in particular UPLOAD_ERR_NO_FILE) yield an empty filename. The UploadFile validator does not report the original error in that case, but FILE_NOT_FOUND instead.
This fix validates the filename only in case of UPLOAD_ERR_OK. Any other error code will be reported correctly.