Problem/Motivation
core/modules/file/src/Plugin/rest/resource/FileUploadResource.php will call fclose(FALSE);
if it fails to open a temp file for storing the in-progress upload:
$temp_file_path = $this->fileSystem->tempnam('temporary://', 'file');
$temp_file = fopen($temp_file_path, 'wb');
if ($temp_file) {
...
}
else {
// Close the file streams.
fclose($temp_file); // Not a good idea.
...
}
I first noticed this at #2940383-59: Unify file upload logic of REST and JSON:API.6. Then that issue got postponed, and JSON:API is moving forward with its own nearly identical copy of the "upload service". I'm trying to fix the bug again at #3035866: Update \Drupal\jsonapi\ForwardCompatibility\FileFieldUploader to be in sync with \Drupal\file\Plugin\rest\resource\FileUploadResource but @Wim Leers "absolutely, completely, vehemently disagree[s] with this." ;)
I agree we need to fix it in REST, too, hence this issue. :)
Proposed resolution
If fopen()
returned FALSE, don't call fclose()
on that "file pointer".
Remaining tasks
Upload the patch.Decide if/how we can write a test for this. *sigh* I don't really want to jump through all the hoops to mock the file system for this. Can't we plainly see that calling fclose(FALSE) is a bad idea, remove that line, and sleep easy if all the rest of the REST tests pass? ;)Reviews.RTBC.Commit.- Re-fix this in JSON:API: #3036251: JSON:API TemporaryJsonapiFileFieldUploader::streamUploadData() can call fclose(FALSE)
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
N/A.
Comment | File | Size | Author |
---|---|---|---|
#2 | 3036197-2.patch | 861 bytes | dww |
Comments
Comment #2
dwwComment #3
dwwComment #4
gabesulliceThis seems entirely sensible to me.
Comment #5
dwwOpened #3036251: JSON:API TemporaryJsonapiFileFieldUploader::streamUploadData() can call fclose(FALSE) as a child issue to fix this in JSON:API.
If this lands before #2843147: Add JSON:API to core as a stable module that can be fixed in JSON:API contrib. If #2843147 lands first, we can move that issue to the core queue and fix it once this lands (or possibly merge that in and fix both in the same commit).
Ahh, the joys of hoop jumping! ;)
-Derek
Comment #6
alexpottCommitted and pushed 04343d7d91 to 8.7.x and 3528551bb1 to 8.6.x. Thanks!
Comment #9
dwwExcellent, thanks! I just unblocked the JSON:API issue.
Cheers,
-Derek