A request to revert changes in removing empty file uploads #24546

szymach · 2017-10-13T11:34:18Z

Q	A
Bug report?	no
Feature request?	no
BC Break report?	possibly
RFC?	no
Symfony version	3.3.10

Hello,
The change introduced in #24198 broke our file upload bundle. Our flow looks like this:

We use a form to create an entity with a file, using a form field which is not actually persisted. From it, we create a copy of the upload locally and only the file path is persisted to the database.
On post load event, we recreate the file object from the file path in the database and then put it in the non-persisted entity field.
When editing the entity through a form, we do not initially pass the file to the field, but instead do it if none was sent. This prevents the current value being treated as a new one and overwriting itself in the process (current file being deleted and being replaced with the same file, with a suffixed name).

The problem we face now is that the form fields are being completely removed from the request and we have no hook for our functionality. Is there a possibility to revert this change? From what I can tell, it does not actually fix anything, but provides an easier way to deal with empty uploads.

nicolas-grekas · 2017-10-13T11:49:32Z

ping @enumag

xabbuh · 2017-10-13T12:02:05Z

@szymach I am not sure I understand your issue. Can you create a small example project that allows to reproduce it?

szymach · 2017-10-13T12:20:28Z

Long story short - we need those empty file fields in order to substitute the empty value with the one recreated from the database. For example, I have an entity with collection of entities that only contain a file field:

class Entity
{
   /**
    * @var FileEntity[]
    **/
    private $fileCollection;
}

class FileEntity
{
   /**
    * This contains a \SplFileInfo object created out of the uploaded file,
    * not actually persisted. Is used for the file form field.
    **/
   public $file;

  /**
   * Path to the copy of the uploaded file, persisted to database. 
   * Reads data from file and saves a recreated object during post load
   **/
   public $filePath;
}

Now, the only time a value for the file field is sent is when a user actually uploads a new file. If you edit the Entity class, we do not put the current value to the form field. So, when the form is submitted, that fileCollection initially contains an array of empty values (for each element of the collection an array with [file => null]). We then overwrite these empty array items with the values we recreate from the database.

After the change in aforementioned pull request, these empty form fields do not exist in the request at all and are completely removed from the form. We cannot substitute the empty values with the ones from the database, so each FileEntity is treated like an collection item that should be removed - since they are present in the entity collection, but not in the form field.

I hope this makes our predicament a bit more clear.

enumag · 2017-10-13T12:33:36Z

This is quite difficult to understand. Can we see the form?

The effect of that PR is to replace [null] with []. I have no idea how you managed to put [file => null] in there... Would this work for you? d545dcb

szymach · 2017-10-13T12:47:49Z

Here are example forms:

class GalleryType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder->add('photos', CollectionType::class, [
            'label' => false,
            'entry_type' => GalleryPhotoType::class,
            'allow_add' => true,
            'allow_delete' => true,
            'by_reference' => false
        ]);

    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults([
            'data_class' => Gallery::class
        ]);
    }
}

class GalleryPhotoType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder->add('image', ImageType::class, [
            'label' => 'admin.gallery.photo'
        ]);
    }

    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults([
            'data_class' => Photo::class
        ]);
    }
}

Edit an existing Gallery, with 1 element in the photos collection. Previously, after submitting the form, you would get a null for key gallery[photos][0][image]. Now, that key is non-existent, because no value was actually submitted for it (we replace that empty value later on). Since this was the only field for that collection item and it is empty, the whole item is treated as empty and is removed from the collection.

enumag · 2017-10-13T12:53:05Z

Did you try it with the commit I referenced above? If I understand the issue correctly it should help... Then we just need to add another test case and we're good.

szymach · 2017-10-13T13:04:17Z

Well .... it does :) But what happens if you upload multiple files for a single field? Could you end up with a case of $file = [null, null]? It would not affect us, since we handle multiple file upload differently, but you may want to take that into consideration.

yceruto · 2017-10-13T13:16:23Z

But what happens if you upload multiple files for a single field? Could you end up with a case of $file = [null, null]?

I think we have two options only: (1) select none file (2) select multiple files, so (at least on a normal behavior) with a single field you'll have [null] or [<File>, <File>] respectively.

enumag · 2017-10-13T13:17:24Z

Could you end up with a case of $file = [null, null]?

As far as I know, that is not possible.

szymach · 2017-10-13T13:22:02Z

In that case I consider your fix to do the job. Thank you :)

enumag · 2017-10-13T13:59:52Z

After some experiments [null, null] is in fact possible for cases like this:

<form method="post" enctype="multipart/form-data">
<input type="file" name="form[images][]"><br>
<input type="file" name="form[images][]"><br>
</form>

In this case I believe we want to remove the nulls. So the correct fix in my opinion is to detect whether the array is associative and use array_filter only if it is not. What do you guys think?

EDIT: This is what I have in mind: enumag@79a7f8a

szymach · 2017-10-13T14:20:32Z

Hey. I will get back to you later this evening with a response, since I will need to do some additional checks on the solution you provided and I do not have the time ATM.

szymach · 2017-10-13T18:51:20Z

OK, so I have checked whether your proposal will affect our multiple files upload and it does, unfortunately. We really need these empty values for our bundle to work, otherwise the form fields are removed before we can do anything about it. We may find a workaround for it, but it could also mean rewriting the bundle from scratch or abandoning it altogether, which I would rather avoid, since we have already put a lot of work into it.

I understand that the previous behavior did not explicitly state null will be returned for each empty file upload, but it has been substantially altered. From what I have read on the #24198 discussion, the issue was that the empty value has been transformed from null into an empty string? Should you not simply explicitly state empty_value => null to prevent this?

enumag · 2017-10-13T19:03:47Z

The problem is that when you have <input type="file" multiple name="upload[]"> then http-foundation gives this results (before #24198):

// 2 files submitted:
$request->files['upload'] = [ UploadedFile, UploadedFile ];
// 1 file submitted:
$request->files['upload'] = [ UploadedFile ];
// no file submitted:
$request->files['upload'] = [ null ];

Which is wtf, the expected result for the last case is of course an empty array.

This was fixed in symfony/form UploadType by removing the null. However this fix was problematic because it:

didn't work for me because while http-foundation returned [null], UploadType somehow recieved [''] instead and
UploadType isn't the correct place to fix it. The array should be empty even when using http-foundation directly.

We might have to revert it for the sake of BC obviously but I'd much rather find some better solution.

Can you explain how my last proposal breaks your bundle?

szymach · 2017-10-13T19:11:24Z

Well it does what it currently does - it yields an empty array instead of array with nulls for each empty uploaded item. It removes the array keys we expect them to be.

enumag · 2017-10-13T19:17:00Z

Replacing ['upload' => [null]] with ['upload' => []] does not remove the upload key. Yes it removed the 0 key since [null] is actually [0 => null] but that's just for sequential arrays (integer keys) and from the example above you're using associative arrays (string keys) so this should not affect you.

szymach · 2017-10-17T20:13:35Z

Hello, sorry for the delay, busy times.

It turned out we were simply referring to index 0 on the files array explicitly, using reset() fixed the issue. Your fix does the trick.

enumag · 2017-10-17T21:04:21Z

Good. I'll send a PR later this week.

… (enumag) This PR was squashed before being merged into the 2.7 branch (closes #24606). Discussion ---------- [HttpFoundation] Fix FileBag issue with associative arrays | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24546 | License | MIT | Doc PR | Commits ------- 8ea2860 [HttpFoundation] Fix FileBag issue with associative arrays

xabbuh added BC Break Bug Form labels Oct 13, 2017

carsonbot added the Status: Needs Review label Oct 13, 2017

xabbuh added this to the 2.7 milestone Oct 13, 2017

xabbuh added the Status: Waiting feedback label Oct 13, 2017

enumag mentioned this issue Oct 18, 2017

[HttpFoundation] Fix FileBag issue with associative arrays #24606

Closed

fabpot closed this as completed Oct 26, 2017

yceruto mentioned this issue Nov 2, 2017

File Type is now empty when updating (in 3.3) #24792

Closed

BoShurik mentioned this issue Dec 24, 2019

[Form][HttpFoundation] Filtering FileBag removes items from CollectionType with FileType #35100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

A request to revert changes in removing empty file uploads #24546

A request to revert changes in removing empty file uploads #24546

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

A request to revert changes in removing empty file uploads #24546

A request to revert changes in removing empty file uploads #24546

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!