VichUploaderBundle

VichUploaderBundle copied to clipboard

I've found interesting edge case: create form with vich file type (allow delete = true), then check remove file and submit invalid form. Post submit listener removes file from disk and clean file information from entity. But usually nobody calls flush for invalid forms. So we got unsynchronized state: file was removed but database not updated.

I see 2 different approaches to fixing this issue:

• a. change priority of our removing listener and check for isValid first (easy but not cover all cases)
• b. add dynamic preflush listener to entity manager wich will remove file

We just ran into this exact bug in our application. Has there already something been done, or ideas for a workaround?

This bug requires investigation. We should create something like queue - schedule image removing before flush and actually remove it from storage on post flush. Haven't time/money for fixing it right now. Maybe on version 2.0.

I guess we'll fork and implement your proposed solution a, as it seems to work. Which cases would this not cover?

Once time, feel free to provide a technical description of how to implement b and I might be able to have a look at it.

Thanks for fast reply!

Hi, is any chance to fix it?

I have fixed this locally, will make this public ready and create a PR soonish. I think this is a serious issue and should at least be mentioned in the docs.

I've encoutered the same problem and not finding this issue (I will look better next time, I promise 😄) I proposed a different solution, which is simpler. I quote it here for reference, I'm willing to open a PR as soon as possible if we decide to go my way as the issue is a serious one for me in one of my projects. I did not have a look at the implementation done by @kimwue, so I'm not aware of what he coded, but a queue to schedule file removal seems a lot of work when simpler solutions like mine seems to work fine as well, even though I do not really like to have to loop over all the form children

At first I though that the fix was as simple as checking that the form was valid before deleting the file from the storage in the POST_SUBMIT event listener added by the VichFileType type. However at that point the form has not still been processed completely and so other fields may not have been validated yet. I've managed to workaround the issue by creating a FormType form extension which adds (nearly) the same POST_SUBMIT listener added in the VichFileType class. The difference is that the listener iterates all form children starting from the root, checks if they are a VichFileType field and calls the $uploadHandler->remove() method only if the whole form is valid. I've done this because there is no way (at least easy) to add a listener to the root form from a child form when building it. I can prepare a PR if you're fine with this solution

@ste93cry I created a PR with my solution, its not 100% finished yet but I figured it might be enough to discuss the solution. Maybe do the same for yours? I tried a bit your direction in the beginning but couldn't get it to work.

Any updates on this? I discovered the same behavior when validation is fails. I have the entity with 3 files and when I had to update 1 and remove 2 other and if this 1 file is not valid 2 files are deleted but db doesn't updated.

Example:

    db_driver: orm
    storage: flysystem
    metadata:
        auto_detection: true
        cache: file
    mappings:
        # https://github.com/dustin10/VichUploaderBundle/issues/345
        platform_logo_shared:
            upload_destination: shared_filesystem
            namer: Vich\UploaderBundle\Naming\SmartUniqueNamer
            inject_on_load: false
            delete_on_update: true
            delete_on_remove: true

`
/**
* @Vich\Uploadable()
 */
class MyEntity {
    /**
     * @ORM\Id()
     * @ORM\GeneratedValue()
     */
    private $id;

    /**
     * @var string|null
     *
     * @ORM\Column(name="logo_img", type="string", nullable=true)
     */
    protected $logoImgName;

    /**
     * @var string|null
     *
     * @ORM\Column(name="logo_menu", type="string", nullable=true)
     */
    protected $logoMenuName;

    /**
     * @var string|null
     *
     * @ORM\Column(name="favicon", type="string", nullable=true)
     */
    protected $faviconName;

    /**
     * @var File|null
     * @Vich\UploadableField(mapping="platform_logo_shared", fileNameProperty="logoImgName")
     *
     * @Assert\File(
     *     maxSize="5M"
     * )
     */
    protected $logoFile;

    /**
     * @var File|null
     *
     * @Vich\UploadableField(mapping="platform_logo_menu", fileNameProperty="logoMenuName")
     *
     * @Assert\File(
     *     maxSize="5M"
     * )
     */
    protected $logoMenuFile;

    /**
     * @var File|null
     *
     * @Vich\UploadableField(mapping="platform_favicon", fileNameProperty="faviconName")
     *
     * @Assert\File(
     *     maxSize="5M"
     * )
     */
    protected $faviconFile;
...
} 
` 

up :)

There's a (sadly stale) PR about this. If anyone wants to pick it up from there and finish it...