Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
#2869855: Race condition in file_save_upload causes data loss added a call to do
// Ensure that the file object is validated.
$file->setValidationRequired(TRUE);
This has resulted in a regression that when you call ->save() on the File objects returned by file_save_upload() you now need to ensure they are validated. This is causing errors in contributed project testing - see https://www.drupal.org/pift-ci-job/1446339
Proposed resolution
Once the File object is saved call $file->setValidationRequired(FALSE);
to reset this.
OR
Remove the call to $file->setValidationRequired(FALSE);
as I'm not sure it is necessary.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#2 | 3089697-2.patch | 646 bytes | alexpott |
Comments
Comment #2
alexpottActually I think we can fixed this by removing code. The call to
$file->setValidationRequired(TRUE);
is over cautious. There's no way in the current code of calling $file->save() without calling $file->validate() in _file_save_upload_single().Comment #3
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedAh yes, this is the code in my controller that is probably bitten by this. I'll update my local development branch to see if that's the cause.
Comment #4
alexpottComment #5
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedFunny enough, adding that call in 8.7 makes it fail later in the test, so it still looks something else. Actually going to switch now (lazy sometimes, sorry)
Comment #6
alexpott@swentel yeah - but if you do the removal as done in #2 the test passes (at least locally for me).
Comment #7
alexpottI;ve run indieweb's test locally against 8.9.x with this patch applied...
So in my opinion if this patch is green then we're solving a regression. Thanks for reporting @swentel!
@swentel fyi your deprecation notices are :
Comment #8
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commented@alexpott doesn't fix it here - running on a just upgraded version of my site. Definitely weird: before the upgrade, no test errors. After: always getting the same error just like on the test bot, with or without patch. Mysterious :)
- edit - does MicropubTest fail for you before the removal?
Comment #9
alexpott@swentel yep it totally does fail for exactly the reason reported before I apply #2
Comment #10
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedOk, user fail. My composer update failed, but didn't notice that. Fixed. Fail without, success with patch now.
So, RTBC on my part. Since everything is still green, I guess that's fine (at least in my book).
- edit - Although it kind of smells fishy :) (but _file_save_upload_single() is not the nicest function to look at anyway)
Comment #11
mcdruidI haven't had a chance to dig into this properly yet, but I'm curious... if the upload process is calling
file_save_upload()
which then calls_file_save_upload_single()
where this happens:...how do we end up with a file object which apparently has not been validated? In core's tests at least the validation happens later on in the same function.
If we need to remove the lines above that's fine, but I don't 100% understand why we'd need to yet.
Have we had any other reports of contrib test failures because of this?
Comment #12
alexpott@mcdruid well because previously you could call save again on the file entities returned from file_save_upload() and now you can't. That type of behaviour change is likely to break a number of custom and contrib projects.
Looking at the code in _file_save_upload() it is not possible for it to return an unvalidated file so I really don't think the original call to $file->setValidationRequired(TRUE); is necessary - and the green tests show that this was not necessary to fix #2869855: Race condition in file_save_upload causes data loss as that issue added test coverage.
Comment #15
catchCommitted/pushed to 9.0.x/8.9.x/8.8.x, thanks!
Comment #17
mcdruidThanks for fixing this!