Problem/Motivation

#59750: Required flag on file forms breaks on validation introduced a change to how the default value is derived.

However this also introduced a bug with simple UploadedFile objects being casted into an array, rather than being wrapped.

https://stackoverflow.com/a/4345609

This breaks existing calls such as:

// Retrieve either the single UploadedFile object or an array of it.
$files = $form_state->getValue('file');

Steps to reproduce

Produce a test which confirms that the value returned by the form state is a valid UploadedFile object.

Proposed resolution

Do a check to see if the return value is not an array and then wrapping accordingly rather than casting.

e.g.

---    return (array) $uploaded_file;
+++    return is_array($uploaded_file) ? $uploaded_file : [$uploaded_file];

Remaining tasks

Provide issue fork/patch.

User interface changes

N/A

API changes

N/A

Release notes snippet

CommentFileSizeAuthor
#14 drupal-3352632-11-13.diff1018 bytesmingsong
#13 3352632-drupal-13.patch3.82 KBmingsong
#11 3352632-drupal-11.patch3.86 KBmingsong
#8 drupal-3352632-8.patch856 bytesmingsong
#4 3352632-file-form-state-value-4.diff908 bytescodebymikey

Issue fork drupal-3352632

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

  • 3352632-file-form-state Comparechanges, plain diff MR !3788
  • 9.5.x Comparecompare
  • 10.1.x Comparecompare
About issue forks

Comments

codebymikey created an issue. See original summary.

codebymikey’s picture

codebymikey commented
Related issues: +#59750: Required flag on file forms breaks on validation

codebymikey opened merge request !3788

codebymikey’s picture

codebymikey commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new 3352632-file-form-state-value-4.diff908 bytes

Status: Needs review » Needs work

The last submitted patch, 4: 3352632-file-form-state-value-4.diff, failed testing. View results

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Seems reasonable, just need to work through the failures

mingsong’s picture

mingsong
Location 🇦🇺
commented

Test patch #4 by following steps:

1. Login as a user who has permission to access Interface translation import page('/admin/config/regional/translate/import').
2. Upload a translation file (.po) via that form.
3. After submitting that form. An error said

Exception: Serialization of 'Symfony\Component\HttpFoundation\File\UploadedFile' is not allowed in serialize() (line 157 of core/lib/Drupal/Core/Batch/BatchStorage.php).

.

Test environment:

  • Drupal core version: 9.5.7
  • PHP version: 8.1.14

Note:
Without having the #4 patch, the translation import is working.

mingsong’s picture

mingsong
Location 🇦🇺
commented
StatusFileSize
new drupal-3352632-8.patch856 bytes

If we change line 97 at /core/lib/Drupal/Core/Render/Element/File.php

from 

return is_array($uploaded_file) ? $uploaded_file : [$uploaded_file];

to

return is_array($uploaded_file) ? $uploaded_file : [(array) $uploaded_file];

All tests on my local are passed.

Here is the new patch.

mingsong’s picture

mingsong
Location 🇦🇺
commented
Status: Needs work » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Render/Element/File.php
@@ -91,9 +91,9 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
+      return is_array($uploaded_file) ? $uploaded_file : [(array) $uploaded_file];

this is an array of arrays, is that correct?

Can we add to the existing test coverage for this case?

See \Drupal\Tests\file\Kernel\ManagedFileTest

mingsong’s picture

mingsong
Location 🇦🇺
commented
StatusFileSize
new 3352632-drupal-11.patch3.86 KB

No worries.

The problem with path #4 is that Symfony\Component\HttpFoundation\File\UploadedFile can not be serialized, according to the error message.

this is an array of arrays, is that correct?

Yes, that is right. A nested array instead of a single array.

Before having the patch #8.

The file element value could be a single array looks like this,

[
  [Symfony\Component\HttpFoundation\File\UploadedFiletest] => false, 
  [Symfony\Component\HttpFoundation\File\UploadedFileoriginalName] => vi.po, 
  [Symfony\Component\HttpFoundation\File\UploadedFilemimeType] => application/octet-stream, 
  [Symfony\Component\HttpFoundation\File\UploadedFileerror] => 0]
]

After patching, the file element value will be a nested array.

[
  [0] => 
    [
      [Symfony\Component\HttpFoundation\File\UploadedFiletest] => false, 
      [Symfony\Component\HttpFoundation\File\UploadedFileoriginalName] => vi.po, 
      [Symfony\Component\HttpFoundation\File\UploadedFilemimeType] => application/octet-stream, 
      [Symfony\Component\HttpFoundation\File\UploadedFileerror] => 0]
    ]
]
Can we add to the existing test coverage for this case?

Sure.
I create a new test /core/tests/Drupal/KernelTests/Core/Element/FileElementTest.php to cover this case.

Here is the new patch with test case.

mingsong’s picture

mingsong
Location 🇦🇺
commented
Status: Needs work » Needs review
mingsong’s picture

mingsong
Location 🇦🇺
commented
StatusFileSize
new 3352632-drupal-13.patch3.82 KB

There is a typo in patch #11.

New patch.

mingsong’s picture

mingsong
Location 🇦🇺
commented
StatusFileSize
new drupal-3352632-11-13.diff1018 bytes
codebymikey’s picture

codebymikey commented

Appreciate the tests @Mingsong and finding where the proposed patch breaks things during serializations, the issue is that calls like this:

// Should ideally return an array of UploadedFile instances.
$files = $form_state->getValue('file');

Should ideally still return an array of \Symfony\Component\HttpFoundation\File\UploadedFile instances, having it being casted to an array makes it harder to retrieve the original value from the form state.

Unless we document that uploaded files should never be read from the $form_state but instead directly from the request (as core seems to handle it in most of its code).

Or potentially introduce a new serializable class to decorate \Symfony\Component\HttpFoundation\File\UploadedFile and use that inside the form state (so that it can work in batch contexts).

edit: Usage of the current patch can lead to crashing of config entity forms which make use of file uploads when editting them (e.g. embed), since \Drupal\Core\Entity\EntityForm::copyFormValuesToEntity() will copy the UploadedFile into the entity without checks.
So if the entity form page has a Delete button, then during the form serialization for caching, it might throw a serialization exception due to the uploaded file being attached to the entity object.

mingsong’s picture

mingsong
Location 🇦🇺
commented
Status: Needs review » Needs work

Yes, I realized what @Mikey pointed too.

The problem is bigger than I thought. More works need to do.

mingsong’s picture

mingsong
Location 🇦🇺
commented
Issue tags: +Drupal 10

This also affects Drupal 10 too.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.