Selecting too many files with JS off causes WSOD with data loss

Critical bugs:

"Critical bugs include those that:

• Render the system unusable and have no workaround.
• Cause loss of data.
• Expose security vulnerabilities.
• Cause tests to fail in HEAD on the automated testing platform, since this blocks all other work.

"

Data loss is bad.

What is the fatal error?

Still very much needs tests.

#8 is working fine as I hace tested the patch manually at local. and didn't see ant WSOD or "PHP Fatal error: Call to undefined method Drupal\\field\\Entity\\FieldStorageConfig::getFieldName() in /data/website/tcdrupal8/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php on line 353"

Below are the screenshots of verification:
Uploaded files when JS is OFF

Result of Saving the Node page when JS is OFF.

Wow, nice find!

It is not straightforward to write a test coverage for that.
I tried it with

      // Try to upload more files than allowed.
      $test_file = $this->getTestFile('text');
      $this->drupalGet("node/add/$type_name");
      $delta = 0;
      $edit['files[' . $field_name . '_' . $delta . '][]'] = drupal_realpath($test_file->getFileUri());
      $edit['files[' . $field_name . '_' . $delta . '][] '] = drupal_realpath($test_file->getFileUri());
      $edit['files[' . $field_name . '_' . $delta . '][]  '] = drupal_realpath($test_file->getFileUri());
      $edit['files[' . $field_name . '_' . $delta . '][]   '] = drupal_realpath($test_file->getFileUri());
      $this->drupalPostForm(NULL, $edit, t('Upload'));

but that did not produced the expected result.

In general this issue summary shows that having some more static analysis of our code is not bad idea.

Working on a test.

Ok. I don't see any other way except of building the whole POST data by hand, I don't see how you can upload multiple files with the same name through either our or curl's API.

Took me quite some time to get that right, that stuff is picky :p

Anyway, this is a failing tests, also noticed that we're displaying the wrong number of uploaded files.. but I'm not sure which one to use since we could have existing files. Either we mention how many files were uploaded (but then it could say max 4, uploaded 2) or we use the total count but then we might need to reword the message to make sense.

The test looks great to me!

Nice one!

I think we need test coverage for the issue described on #19 since #16 was passing without fixing that

$keep + 1 makes no sense, there could be any amount of images, not just one more than allowed? I guess what you meant is to use $count.

And yes, as I said, the message could be confusing that way. But it would be equally confusing if it would say you uploaded 4 files when you only uploaded two? We should probably reword the error a bit, not sure which number to use either way.

Not sure which issue @21 refers to exactly, just the message?

#23: The message and the wrong count.

We should explicitly test for the correct expected message is I think what laurii means.

Testing for the correct message is easy.

The harder part is deciding what the correct message is.

As I said, the patch in #19 is IMHO worse, not better.

If we want something better than #16 then we need to rewrite the message. And either say something like N *additional* files were uploaded (and use $uploaded, like my patch) Or we say something like "N existing and uploaded files" and use $count.

And I guess a few variations of the test. I can update the test accordingly, not sure about what to write. But it might be easier when we have the test already.

This was assigned to @mbaynton, but I think it shouldn't overlap too much since he was trying to figure out a different solution apparently. I just added some more tests. First the same as before, then creating a node with 1 file, then adding two more, then trying to add another two.

Found another weird bug, the count of the current files didn't work, this seems to work in the test, still a bit weird.

Tests are not fully working yet but done for now.

No interdiff, since the test code moved around quite a bit, so would be pretty useless.

#29: Tests look great to me. Thanks so much for your contribution! :)

@mbaynton, you are correct, it should be passed against php 5.5. in one of the recent thread https://groups.drupal.org/node/473473

@mbayton: We have just started testing on PHP 5.5 because we agreed dropping the PHP 5.4 only a very short period of time ago and there was also technical stuff that needed to get addressed before we could start testing on PHP 5.5. That comment is 14 days old. All the new patches will be tested against PHP 5.5.

Likely I have committed some Drupal coding style sins, for which I apologize, but its a hard thing to never screw up while simultaneously learning a large new codebase. I will be grateful to anyone who points them out and/or fixes them.

No need to apologize really, it's always good to see people new to core development in the critical queue. Welcome :) !

So } else { this one, we use a line break:

}
else {

In $args = array( it's good to break into multiple lines but you only need to indent by two spaces

  $args = array(
    '%field' => $field_name,
    '@count' => $cardinality
  );

I also would move drupal_realpath($file->getFileUri()); into a variable to avoid writing it twice.

Otherwise, I believe this is great work, please keep it up!

+++ b/core/modules/file/src/Tests/FileFieldTestBase.php
@@ -149,16 +149,25 @@ function updateFileField($name, $type_name, $field_settings = array(), $widget_s
+   * Allows uploading more than one file to a single field on a node.
+   */
+  function uploadNodeFiles(array $files, $field_name, $nid_or_type, $new_revision = TRUE, $extras = array()) {
     $edit = array(

Ensure that we document all parameters

Except @dawehner, everything looks good to me. I have tested manually the issue and found its working in both state

• JS-ON
• JS-OFF

Attached screenshot in JS-OFF state

+   *
+   * @param \Drupal\file\Entity\File $file
+   *   The File to be uploaded
+   * @param string $field_name
+   *   The name of the field on which the files should be saved
+   * @param $nid_or_type
+   *   A numeric node id to upload files to an existing node, or a string
+   *   indicating the desired bundle for a new node.
+   * @param bool $new_revision
+   *   The revision number
+   * @param array $extras
+   *   Optional settings for entity_create() when a new node is created.
    */

Looks good.

Once small thing needs clarification here. can't be use $nid instead of $nid_or_type because whenever we require bundle type we can use $extra['type'] ?

First of all, great finding and thanks for filing this as bug and fixing @mbaynton. Here is my questions/comments:

+++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
@@ -326,39 +323,6 @@ public static function value($element, $input = FALSE, FormStateInterface $form_
-  public static function validateMultipleCount($element, FormStateInterface $form_state, $form) {

1. This is a public method, so treated as an API and we are removing it. Not sure how that would affect contrib/sites that are using it.

2. This method provided validation + user friendly message. However the patch doesn't seem to provide this message anywhere?

Update: Just read the comment #29 and found my answer for #2 in this comment. So may be we can ignore that.

1. This is a public method, so treated as an API and we kinda removing it. Not sure how that would affect contrib/sites that are using it.

I'm not entirely convinced that every public method should be treated as API function. It was public, because it was used as FAPI callback.

In terms of the method removal, we could add a 'just in case' change record, but I'm not concerned about the API change as such especially given it's necessary to fix the critical issue. If we'd settled on marking things @internal, this is one that definitely should have been.

1. +++ b/core/modules/file/src/Tests/FileFieldTestBase.php
   @@ -147,18 +147,51 @@ function updateFileField($name, $type_name, $field_settings = array(), $widget_s
   +   * @param array $extras
   +   *   Optional settings for entity_create() when a new node is created.
   ...
   +   * @param array $extras
   +   *   Optional settings for entity_create() when a new node is created.
   

   I'd just say something like Additional values when a new node is created. entity_create() is deprecated so we should avoid adding more references to it and it's values, not settings.

2. +++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
   @@ -215,6 +216,42 @@ function testMultiValuedWidget() {
   +
   +    // Try to upload more files than allowed, new node.
   +    $upload_files[] = $test_file;
   +    $this->uploadNodeFiles($upload_files, $field_name, $type_name);
   +    $this->assertRaw(t('%field: this field cannot hold more than @count values.', $args));
   

   Can you post a screenshot of the UI in case more than the allowed number of files have been uploaded?

   Best compared with how the current UI would look like with the old fix.

3. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1683,16 +1683,19 @@ protected function drupalPostForm($path, $edit, $submit, array $options = array(
   +              if (is_array($file) && count($file)) {
   +                // There seems to be no way via php's API to cURL to upload
   +                // several files with the same post field name. However, Drupal
   +                // still sees array-index syntax in a similar way
   +                for ($i = 0; $i < count($file); $i++) {
   +                  $postfield = str_replace('[]', '', $key) . '[' . $i . ']';
   +                  $file_path = drupal_realpath($file[$i]);
   +                  $post[$postfield] = curl_file_create($file_path);
                    }
   

   Nice. Cleaning up non 5.5 code is fine here I think, although you should in general be careful about expanding the scope of an issue.

   Would be good to confirm (attached a raw post request maybe?) that this really results in same behavior but I'm pretty sure this works. Otherwise Drupal wouldn't accept it :)

New patch.

So #47.2 and #47.4 is still open ...

@dawehner, @Berdir

I think the screenshot from the simpletest (FileFieldWidgetTest) is enough. Creating a POST it's not straight.

What you think? Let me know.

Is that really what we want, validation on each individual delta?
Is this maybe just broken inline error messages? Just trying to be curious.

I don't think we need to fix the validation problem here, pretty sure that's yet another of those inline error message problems.

Not being able to submit on validation forms however is a problem. The delete button probably needs .. #limit_validation_errors.

I don't think we need to fix the validation problem here, pretty sure that's yet another of those inline error message problems.

Not being able to submit on validation forms however is a problem. The delete button probably needs .. #limit_validation_errors.

Yeah sorry. Let's refocus!

I think at this point, the *only* code change we should make is the one in #8, s/getFieldName/getName, and the new tests.

We've uncovered several inconsistencies and major bugs, but none of them are directly related to the part of this issue that is critical.

@mbaynton, can you open a follow-up issue for the other problems you've found?

I think at this point, the *only* code change we should make is the one in #8, s/getFieldName/getName, and the new tests.

Yeah, we want to fix the critical asap and ensure we have the needed follow ups.

I am working on it

This patch is a combination #48and #8 that's why no interdiff. I took #8change and test coverage from #48.

The message is probably different now:

Field %field can only hold @max values but there were @count uploaded. The following files have been omitted as a result

, I think we have to adapt the test.

No, the error message does not make sense. In this case I uploaded like 8 files onto a 3-file-allowed field.

Yes, it's the wrong number. And multiple changes have been discussed.

#16 had my fix for that, which can still be confusing, #19 hat a different fix which is still wrong (it would then say 4) and #23 and #25 has some more thoughts from me about the error message. #25 has two suggestions on how we could slightly change the text so it is correct, not sure which is better. Doesn't make a huge difference I think.

Again no interdiff since this is straight upload of #16. This was discussed on IRC. Tim Plunkett and dawehner suggested to put #16patch and it was RTBC in #17.

Agreed, we now have two child issues to hash out all of the other changes that could be made here, but #16 was more than enough for this critical. Thanks!

Nice constructive work!

yeah we should learn to stay even more focused.

I'm not sure about this, I think the test approach from @mbaynton was much cleaner than what I did. I think we just need to fix the assertion message there?

And I'm not sure if #68 was considered, I don't think that making a small change to the error message isn't that much out of scope for this issue, since we're already changing the line anyway.

This is the patch I hoped would result in #64, but there were too many additional changes from iterations between #16 and #48.

This is also what I think @Berdir was asking for in #72.

RTBC +1.

Wowza. Major points for thoroughness, mbaynton! :D

RTBC + 1, that is the fix I want to see.

Committed 341f0e4 and pushed to 8.0.x. Thanks!

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -1683,16 +1683,20 @@ protected function drupalPostForm($path, $edit, $submit, array $options = array(
-                else {
-                  // @todo: Drop support for this when PHP 5.5 is required.
-                  $post[$key] = '@' . $file;

Yay! But we need to do more (in a followup). PHP5.5 defaults to still allowing @ to signify file uploads whereas PHP5.6 defaults to not. I think we should change to not supporting this as the switch of behaviour in PHP versions makes for an unstable test environment. Created #2538260: Set CURLOPT_SAFE_UPLOAD to TRUE in Simpletest's CURL options

#2537930: Clean up FileWidget's behavior when too many files uploaded
#2537940: Expose already-coded better UX when too many files are uploaded