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

CommentFileSizeAuthor
#2 3089697-2.patch646 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
646 bytes

Actually 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().

swentel’s picture

Ah 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.

  if ($file = file_save_upload($file_key, $validators, $destination, $delta)) {
    $uploaded_files[] = $file;
  }    
alexpott’s picture

Issue summary: View changes
swentel’s picture

Funny 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)

alexpott’s picture

@swentel yeah - but if you do the removal as done in #2 the test passes (at least locally for me).

alexpott’s picture

I;ve run indieweb's test locally against 8.9.x with this patch applied...

sudo -u _www ./vendor/bin/phpunit -v -c ./core modules/indieweb/tests/                                                                                                                          2.3m  Wed 23 Oct 16:55:46 2019
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.2.22
Configuration: /Users/alex/dev/sites/drupal8alt.dev/core/phpunit.xml

Testing modules/indieweb/tests/
........................                                          24 / 24 (100%)

Time: 9.74 minutes, Memory: 12.00MB

OK (24 tests, 1404 assertions)

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 :

  7x: Entity type "indieweb_feed" is using config schema as a fallback for a missing `config_export` definition is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. See https://www.drupal.org/node/2949023.
    3x in FeedTest::testFeeds from Drupal\Tests\indieweb\Functional
    2x in MicrosubTest::testMicrosubBasic from Drupal\Tests\indieweb\Functional
    2x in MicrosubTest::testMicrosubCleanup from Drupal\Tests\indieweb\Functional

  6x: file_prepare_directory() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\File\FileSystemInterface::prepareDirectory(). See https://www.drupal.org/node/3006851.
    6x in MicropubTest::testUpload from Drupal\Tests\indieweb\Functional

  4x: Global variable $pager_total is deprecated in drupal:8.8.0 and is removed in drupal:9.0.0. Use \Drupal\Core\Pager\PagerManagerInterface instead. See https://www.drupal.org/node/2779457
    4x in MicrosubTest::testMicrosubBasic from Drupal\Tests\indieweb\Functional

  1x: Entity type "behavior_settings" is using config schema as a fallback for a missing `config_export` definition is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. See https://www.drupal.org/node/2949023.
    1x in GoneTest::testGone from Drupal\Tests\indieweb\Functional
swentel’s picture

@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?

alexpott’s picture

@swentel yep it totally does fail for exactly the reason reported before I apply #2

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Ok, 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)

mcdruid’s picture

I 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:

// Ensure that the file object is validated.
$file->setValidationRequired(TRUE);

...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?

alexpott’s picture

@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.

  • catch committed 323aeb6 on 9.0.x
    Issue #3089697 by alexpott, swentel: Regression due to File::...

  • catch committed e91fad0 on 8.9.x
    Issue #3089697 by alexpott, swentel: Regression due to File::...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.0.x/8.9.x/8.8.x, thanks!

  • catch committed 8b9d80a on 8.8.x
    Issue #3089697 by alexpott, swentel: Regression due to File::...
mcdruid’s picture

Thanks for fixing this!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.