File size validator should only respect the explicitly configured maximum file size

+++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
@@ -302,12 +302,9 @@ public function getUploadValidators() {
     $max_filesize = Bytes::toInt(file_upload_max_size());
     if (!empty($settings['max_filesize'])) {
-      $max_filesize = min($max_filesize, Bytes::toInt($settings['max_filesize']));
+      $validators['file_validate_size'] = min($max_filesize, Bytes::toInt($settings['max_filesize']));
     }
 
-    // There is always a file size limit due to the PHP server limit.
-    $validators['file_validate_size'] = [$max_filesize];
-

this is also used for the actual upload validation.

I think we should instead just alter it in the constraint only, so we don't do this validation for existing files.

Also, this is IMHO a bug/regression because 7.x didn't do anything like this.

That said, I don't really understand why we do need that in there, as you wouldn't actually ever get that far if it's over the system limit?

But that's for another issue to change/discuss.

Looks good to me, I don't really know how we could test this nor how we need to convince to RTBC this. Possibly crosspost in the S3 CORS upload issue, as I'm sure people there will run into this problem.

Maybe we can improve the comments a bit and clearly state that files that are already uploaded and/or might even be stored remotly to not need to respect the max upload size. Needs work for that.

+++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
@@ -177,7 +177,7 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
       '#default_value' => $settings['max_filesize'],
-      '#description' => t('Enter a value like "512" (bytes), "80 KB" (kilobytes) or "50 MB" (megabytes) in order to restrict the allowed file size. If left empty the file sizes will be limited only by PHP\'s maximum post and file upload sizes (current limit <strong>%limit</strong>).', array('%limit' => format_size(file_upload_max_size()))),
+      '#description' => t('Enter a value like "512" (bytes), "80 KB" (kilobytes) or "50 MB" (megabytes) in order to restrict the allowed file size. If left empty the file sizes could be limited only by PHP\'s maximum post and file upload sizes (current limit <strong>%limit</strong>).', array('%limit' => format_size(file_upload_max_size()))),
       '#size' => 10,
       '#element_validate' => array(array(get_class($this), 'validateMaxFilesize')),

could reads strange, maybe we can change the test to specifically mention *Uploading files to the server will...* or so.

Idea how we could have *some* test coverage:

Move the logic for getting the validators into a protected helper method, write a unit test that uses mocking and a subclass to call that helper directly and get the validators, with different settings.

Reroll.

1. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -179,7 +179,7 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
   +      '#description' => t('Enter a value like "512" (bytes), "80 KB" (kilobytes) or "50 MB" (megabytes) in order to restrict the allowed file size. IUPloading files will only be limited only by PHP\'s maximum post and file upload sizes (current limit <strong>%limit</strong>).', ['%limit' => format_size(Environment::getUploadMaxSize())]),
   

   IUPloading files will only <- I think this got messed up in the reroll.

2. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileValidationConstraintValidator.php
   @@ -23,6 +24,16 @@ public function validate($value, Constraint $constraint) {
   +    // Always respect the configured maximum file size.
   +    if ($max_filesize = Bytes::toInt($value->getFieldDefinition()->getSetting('max_filesize'))) {
   +      $validators['file_validate_size'] = [$max_filesize];
   

   This makes it impossible to set max_filesize to 0.

   I think that's acceptable because one should disable the field or make it inaccessible. However, it'd make the code more pleasing if it precisely matched the comment. Something like:

   // Always respect the configured maximum file size.
   $field_definition = $value->getFieldDefinition();
   if (array_key_exists('max_filesize', $field_definition->getSettings())) {
     $validators['file_validate_size'] = [Bytes::toInt($field_definition->getSetting('max_filesize'))];
   }
   

That prevents us from avoiding max file size validation for some use-cases. A typical use case where this would be helpful is when files are not stored on the same server as Drupal (e.g. they are stored in a remote file system, S3).

Maybe I'm misunderstanding this because I'm not very familiar with file system stuff, but couldn't one argue that this is an upstream bug/missing feature in PHP? E.g. shouldn't there be stream wrapper specific versions of upload_max_filesize so that Drupal doesn't have to ignore its environment's configuration?

upload_max_filesize is a PHP ini setting that PHP automatically enforces, this is not related to stream wrappers. The file could absolutely live in the regular sites directory, maybe you've built a custom feature that imports files from a file share or whatever.

This is about validating a file that already exists on the server in regards to whether or not you would be able to upload it to that server. Why would you do that?

If you move to a new server that has a lower upload limit, does that make all your existing files suddenly invalid?

It makes sense to check the setting so that we can tell the user that this limit applies, but that's all it should be used for.

Thanks for helping me understand @Berdir. I misinterpreted the issue to be about uploading to the filesystem or to alternative storage like S3 and made the assumption that the problem was that upload_max_filesize shouldn't/doesn't apply when forward-streaming the upload to an S3 bucket.

Can we add a test for this?

I tried, but no, not easily. upload_max_filesize can't be changed/set at runtime. It doesn't just not use the value, it actively ignores an attempt to set it.

And without the ability to change it, I doubt that we want to have a test that creates a file that is bigger than upload_max_filesize, whatever that value might be.

If that would work, then doing something like \Drupal\Tests\file\Kernel\FileItemValidationTest::testFileValidationConstraint() and setting that instead of the explicit setting would work.

I suppose we could write a unit test for FileValidationConstraintValidator and mock getUploadValidators() and everything else going on, including the function call to file_validate(), but that seems like a lot work with a lot of hardcoded assumptions about the exact implementation of the code.

If your patch to make them separate constraints would move that logic out of \Drupal\Tests\file\Kernel\FileItemValidationTest::testFileValidationConstraint() and into whatever logic creates dynamic constraints with configuration then maybe we could check that?

reroll.

Bytes::toInt() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0

Also, please take into account the comment in #24 where we should check if the setting exists instead of checking if it is not 0.

One personal nitpick: I really dislike the code readability of if (a = b) {} instead of a = b; if (a) {}. There's coding standards for writing secure code that specifically forbid this kind of thing because of how easy it is to overlook what is happening here.

Needed another further fix.

FileFieldValidateTest already tests that the max filesize field setting is respected on both upload and other validation.

Per #29 there is no good way to test that the php environment setting is respected on upload but correctly ignored with other validation.

Random JS test fail, looks very unrelated?

1) Drupal\Tests\layout_builder\FunctionalJavascript\ContentPreviewToggleTest::testContentPreviewToggle
Behat\Mink\Exception\ElementHtmlException: Element exists on the page.

/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php:509
/var/www/html/core/modules/layout_builder/tests/src/FunctionalJavascript/ContentPreviewToggleTest.php:138
/var/www/html/core/modules/layout_builder/tests/src/FunctionalJavascript/ContentPreviewToggleTest.php:94
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

Reroll for 9.5+.

It's green so changing status to needs review.

rtbc per #38

This looks fine and I also can't think of way to test it. Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

Couldn't this be tested this with the plupload api module and the plupload widget module?