delete_on_update not working

[julien-lmnr]

Bug Report

Q	A
BC Break	no
Bundle version	dev-master
Symfony version	6.0.1
PHP version	8.0.14

Hello everyone,

It seems that the delete_on_update parameter is not working correctly, at least not in my case with the dev-master version. Strange because the delete_on_remove parameter works very well.

#[Vich\Uploadable]
class PageImageBlock
{
    #[Vich\UploadableField(mapping: 'page_image', fileNameProperty: 'imageName')]
    private ?File $imageFile = null;

    #[ORM\Column(type: 'string', length: 255)]
    private ?string $imageName = null;

    public function getImageFile(): ?File
    {
        return $this->imageFile;
    }

    public function setImageFile(?File $imageFile): self
    {
        $this->imageFile = $imageFile;

        if (null !== $imageFile) {
            $this->updatedAt = new DateTime();
        }

        return $this;
    }

    public function getImageName(): ?string
    {
        return $this->imageName;
    }

    public function setImageName(?string $imageName): self
    {
        $this->imageName = $imageName;

        return $this;
    }
}

vich_uploader:
    db_driver: orm

    metadata:
        type: attribute

    mappings:
        page_image:
            uri_prefix: '%app.path.page_image%'
            upload_destination: '%kernel.project_dir%/public%app.path.page_image%'
            namer: Vich\UploaderBundle\Naming\SmartUniqueNamer

Do you have an idea ?

[garak]

Sorry, I can't reproduce the problem. Using Symfony 6.0 and VichUploaderBundle 1.19, when I upload a new file I see that the old file is deleted.

[Jupi007]

Hi, it looks like I also have this problem. Using Sf 5.4 and VichUploaderBundle 1.19

delete_on_remove works but not delete_on_update.

[garak]

I was able to reproduce the problem with Symfony 5.4.1, but with Symfony 5.4.2 is working.

[Jupi007]

Just tried with Sf 5.4.2 (I was using the 5.4.1). I also removed var/cache, but it still doesn't work.

[garak]

@Jupi007 I guess it's related to other dependencies. Try to upgrade all your vendors. If you still experience this problem, please create a minimal application when it's possible to reproduce it systematically.

[julien-lmnr]

@garak Hi,

I just launched a new symfony 6.0.2 project in which I simply installed and configured the bundle and I still have the same problem. I created a repository with the code I'm talking about so you can maybe see if the cause is me or the bundle ^^.

https://github.com/julien-lmnr/vich-issue1252

[Jupi007]

@julien-lmnr Thanks, nice you did it :)

[julien-lmnr]

If it helps, while creating a listener to delete my cached images generated by liip, I notice that the vich_uploader.pre_remove event is never called when modifying an image.

[garak]

If it helps, while creating a listener to delete my cached images generated by liip, I notice that the vich_uploader.pre_remove event is never called when modifying an image.

Indeed, the event that should be called is vich_uploader.pre_upload

[garak]

By the way, the listeners used by this bundle to perform actions (like removing the old image when updated) are Doctrine listeners (tagged with doctrine.event_subscriber), not Symfony listeners. So, you won't find information about them in the Symfony profiler.

[Jupi007]

@Jupi007 I guess it's related to other dependencies. Try to upgrade all your vendors.

I tried to do a big upgrade of all vendors but the problem is still here. I also suspect that the problem is not related to this bundle itself. If I found the time, I'll try to also do a minimal application to investigate.

[yorshinnh]

Hi, it looks like i also have the same problem. Using Sf 5.3.13 and VichUploaderBundle 1.19.0

delete_on_remove works but not delete_on_update.

[cedricgeffroy]

Same with me SF 5.4.2 VichUploader 1.19.0

[olivier1980]

Same here, symfony 6.0.2, Easyadmin 4.0.3, VichUploader 1.19.0.

Did some debugging, in the CleanListener.php there is a continue at line 43 that shouldn't be triggered and is preventing the clean action.

[garak]

Same here, symfony 6.0.2, Easyadmin 4.0.3, VichUploader 1.19.0.

Did some debugging, in the CleanListener.php there is a continue at line 43 that shouldn't be triggered and is preventing the clean action.

The check is meant to avoid cleaning entities that are not involved in changeset

[sdespont]

Same here, Symfony 4.4.37, VichUploader 1.19.0 and PHP 7.4.26

[sdespont]

@garak Good news, I have downgraded to 1.18 and it works! So, the problem must be somewhere in https://github.com/dustin10/VichUploaderBundle/compare/1.18.0...1.19.0

I am checking the diffs, but could you have a look too?

[tsikora666]

Filename property have to be changed. I think, that the event is being called too soon (before property fileName changes).

Entity definition

/**
 * @Vich\Uploadable()
 */
#[ORM\Entity(repositoryClass: AttachmentRepository::class)]
#[ORM\HasLifecycleCallbacks]
class Attachment
{
//....
#[ORM\Column(type: 'string', length: 255, nullable: true)]
    private ?string $fileName = '';
    /**
     * @Vich\UploadableField(mapping="month_company_attachments", fileNameProperty="fileName", mimeType="mimeType")
     */
    private ?File $File = null;

    /**
     * @return File|null
     */
    public function getFile():?File
    {
        return $this->File;
    }

    /**
     * @param File $File
     */
    public function setFile(?File $File):void
    {
        $this->File = $File;
        $this->updatedAt = new \DateTimeImmutable();
    }

        public function getFileName():?string
    {
        return $this->fileName;
    }

    public function setFileName(?string $fileName):self
    {
        $this->fileName = $fileName;
        return $this;
    }

}

\Vich\UploaderBundle\EventListener\Doctrine\CleanListener::preUpdate

        $changeSet = $event->getEntityChangeSet();
        foreach ($this->getUploadableFilenameFields($object) as $field => $fileName) {
            f (!isset($changeSet[$fileName])) {
                continue;
            }

            $this->handler->clean($object, $field, $changeSet[$fileName][0]);

The problem is, that $changeSet contains only 'updatedAt' key, because $fileName didn't change yet. That's the place where code blows up, because handler->clear will never be called :(

[tsikora666]

I've checked... Downgrading from version 1.19 to 1.18 solved problem - events are being processed in different order now.

[garak]

I've checked... Downgrading from version 1.19 to 1.18 solved problem - events are being processed in different order now.

So, just to confirm: if you try to call setFilename inside setFile method, does it work?

[tsikora666]

Yes, it works then. Listener \Vich\UploaderBundle\EventListener\Doctrine\CleanListener::preUpdate contains changed $changeSet['fileName'] key with old file's name value. I assure you, that I haven't changed my code at all, just downgraded bundle to 1.18

My env context:

• symfony 5.4.4
• php 8.1.2 64 bit
• vich/uploader-bundle v1.18.0

[garak]

@tsikora666 could you check if the solution proposed in #1267 is fixing this issue too? Thanks

[mathieu-ducrot]

I can also confirm that downgrading from 1.19 to 1.18 fixed the behavior of delete_on_update with this stack :

• symfony 4.4.36
• php 7.4.22
• vich/uploader-bundle v1.19.0 -> v1.18.0

[pimoux]

I confirm that the event is not called on version 1.19 and called on version 1.18. My stack was:

• php >= 8.0.2
• symfony 6.0.4
• vich/uploader-bundle v1.19.0 (event delete_on_update not called)

I created a project with older versions and it works when the version of the bundle is 1.18. This is the stack of this testing project:

• php >= 7.2.5
• symfony 5.4.5
• vich/uploader-bundle v1.18.0 (event called)

[garak]

@pimoux could you check if the solution proposed in #1267 is fixing this issue too? Thanks

[Jupi007]

Hi, I can confirm that the problem is gone using the 1.18 version on the same project. I also tried #1267, but it doesn't work sadly.

[pimoux]

@garak same situation as Jupi007, the previous file is not removed sadly. I replaced the content of my RemoveListener.php (vendor/vich/src/EventListener) with your changes in your RemoveListener.php in the issue. (I prefer say what I did because it's the first time I've done something like this and I'm not 100% sure if it works).

[garak]

Can you please check if related PR is fixing?

[Jupi007]

Just tested this PR: it still not working, sadly.

[garak]

I performed some tries on the sandbox provided by @julien-lmnr and I ended up with this solution: you can make it working just by dropping the DateTime field and updating the fileNameProperty instead. For the provided sandbox, the change is the following:

--- a/src/Entity/Page.php
+++ b/src/Entity/Page.php
@@ -7,10 +7,10 @@ use DateTimeImmutable;
 use DateTimeInterface;
 use Doctrine\ORM\Mapping as ORM;
 use Symfony\Component\HttpFoundation\File\File;
+use Symfony\Component\HttpFoundation\File\UploadedFile;

@@ -43,10 +51,8 @@ class Page
     {
         $this->imageFile = $imageFile;

-        if (null !== $imageFile) {
-            // It is required that at least one field changes if you are using doctrine
-            // otherwise the event listeners won't be called and the file is lost
-            $this->updatedAt = new DateTimeImmutable();
+        if ($imageFile instanceof UploadedFile) {
+            $this->imageName = $imageFile->getClientOriginalName();
         }
     }

If it's OK for you, I can propose a change in documentation, document the change in the CHANGELOG and close this issue

[Jupi007]

@garak I just tried your proposal solution:

• the event is now called ;
• but removing the old file is impossible because the event is called after we updated the filename. I'm using the messenger component for file removal, and it try to remove a file named like the new file original name.

[garak]

@garak I just tried your proposal solution:

* the event is now called ;

* but removing the old file is impossible because the event is called after we updated the filename. I'm using the messenger component for file removal, and it try to remove a file named like the new file original name.

I don't get the last sentence: how are you using the messenger?

[Jupi007]

I don't get the last sentence: how are you using the messenger?

I followed the instruction here: https://github.com/dustin10/VichUploaderBundle/blob/master/docs/events/howto/remove_files_asynchronously.md

The message handler receive a wrong filename so it can't do the deletion.

[3PSY0N]

Same issue for me, Vich: 1.19 Symfony: 6.0.6 PHP: 8.1.3

delete_on_update: not working anymore delete_on_remove: working as expected

[broiniac]

Vich: 1.19 / Symfony: 5.4 / PHP: 8.1 Same issue, but with Embedded version as well. Can I help somehow?

[garak]

@Jupi007 can you please try applying the following patch? I'm trying to pass the old filename to the event, so it should be used also by the messenger.

diff --git a/src/Event/Event.php b/src/Event/Event.php
index ac812ba..35c67d9 100644
--- a/src/Event/Event.php
+++ b/src/Event/Event.php
@@ -21,10 +21,14 @@ class Event extends ContractEvent
     /** @var bool */
     protected $cancel = false;

-    public function __construct($object, PropertyMapping $mapping)
+    /** @var string|null */
+    protected $oldFilename;
+
+    public function __construct($object, PropertyMapping $mapping, ?string $oldFilename = null)
     {
         $this->object = $object;
         $this->mapping = $mapping;
+        $this->oldFilename = $oldFilename;
     }

     /**
@@ -61,4 +65,9 @@ class Event extends ContractEvent
     {
         return $this->cancel;
     }
+
+    public function getOldFilename(): ?string
+    {
+        return $this->oldFilename;
+    }
 }
diff --git a/src/Handler/UploadHandler.php b/src/Handler/UploadHandler.php
index 08672ad..0571642 100644
--- a/src/Handler/UploadHandler.php
+++ b/src/Handler/UploadHandler.php
@@ -101,7 +101,7 @@ class UploadHandler extends AbstractHandler

         $preEvent = new Event($obj, $mapping);

-        $this->dispatch(Events::PRE_REMOVE, $preEvent);
+        $this->dispatch(Events::PRE_REMOVE, $preEvent, $oldFilename);

         if ($preEvent->isCanceled()) {
             return;

[olivier1980]

Any future patch should preferably also include a unit test so it doesn't get broken unnoticed again.

[Jupi007]

@garak I just tried your proposal solution. Sadly, it doesn't work.

The event is only fired if the filename is updated in the entity (as you proposed here: https://github.com/dustin10/VichUploaderBundle/issues/1252#issuecomment-1060654885), but so $oldFilename contains the original name of the new uploaded file, and this is really not that I expect.

Also, but not important, this is not working:

- $this->dispatch(Events::PRE_REMOVE, $preEvent);
+ $this->dispatch(Events::PRE_REMOVE, $preEvent, $oldFilename);

This is should be done instead:

- $preEvent = new Event($obj, $mapping);
+ $preEvent = new Event($obj, $mapping, $oldFilename);

But anyway, it doesn't work.

[julien-lmnr]

@garak how @Jupi007 said, this solution (comment) indeed works. On the other hand, a problem appears when we have an entity validation that fails, it replaces all the links to the old file with the new one which is not correct.

[1ed]

Hello, I think the main problem is the order of the listeners. The CleanListener has higher priority, so it runs before the UploadListener which changes the filename field. I think we should change the listener priority to -50 instead of 50.

But after that we will have another problems. First this check https://github.com/dustin10/VichUploaderBundle/blob/7593b603f960512a3b5f443f41865b7ef3ab8da6/src/Handler/UploadHandler.php#L121-L126 in https://github.com/dustin10/VichUploaderBundle/blob/7593b603f960512a3b5f443f41865b7ef3ab8da6/src/Handler/UploadHandler.php#L80-L90 because at this time the UploadedFile already changed to a File istance.

But after fixing that the https://github.com/dustin10/VichUploaderBundle/blob/7593b603f960512a3b5f443f41865b7ef3ab8da6/src/Handler/UploadHandler.php#L92-L114 $mapping->erase call removes the mapped properties regardless of the $forcedFilename argument, so at the and the filename will be persisted as null.

[garak]

Can anyone try if the fix proposed in #1285 is solving this issue? I hope so, since I just reverted the behaviour to the old one.

[Jupi007]

@garak Your patch seems to work as it, without any change in my project 🥳 The message handler is called with the correct filename.

It could be nice if some other people test this to confirm it is fixed.

[garak]

@julien-lmnr can you confirm, please?

[julien-lmnr]

@garak Yes, I confirm ! It works perfectly for me now 👍

[sdespont]

@garak the branch has been merged to master, but no release has been done. Is there any reason? In my point of view, this fix is important because today files are not automatically deleted and most of this library users are not aware of that.

[garak]

@sdespont it was released in 1.19.1 indeed

[sdespont]

@garak Oh... sorry