The file is not updated if there are not other changes in the entity #8

Closed
chetzof opened this Issue Feb 2, 2012 · 28 comments

Comments

Projects
None yet
@chetzof

chetzof commented Feb 2, 2012

Say if i have a already existing record, and i'm trying to update it, in case if i ONLY upload file and no other entity properties get changed, the preUpdate event is not called, so the file is not uploaded.

Currently the workaround is to introduce some changes into the entity, before saving it.

@dustin10

This comment has been minimized.

Show comment
Hide comment
@dustin10

dustin10 Feb 2, 2012

Owner

I have come across this issue in my current project, but I have not had time to dive into it further. I have just implemented a little hack in my entities right now similar to the following:

public function setFile($file)
{
    $this->file = $file;

    if ($file instanceof UploadedFile) {
        $this->setUpdatedAt(new \DateTime());
    }
}

Hopefully I will have time to look into this soon.

Owner

dustin10 commented Feb 2, 2012

I have come across this issue in my current project, but I have not had time to dive into it further. I have just implemented a little hack in my entities right now similar to the following:

public function setFile($file)
{
    $this->file = $file;

    if ($file instanceof UploadedFile) {
        $this->setUpdatedAt(new \DateTime());
    }
}

Hopefully I will have time to look into this soon.

@chetzof

This comment has been minimized.

Show comment
Hide comment
@chetzof

chetzof Feb 2, 2012

Thanks :)

chetzof commented Feb 2, 2012

Thanks :)

@Blackskyliner

This comment has been minimized.

Show comment
Hide comment
@Blackskyliner

Blackskyliner Feb 15, 2012

Uhh Ohh dirty! :)
But thanks ;)

Uhh Ohh dirty! :)
But thanks ;)

@Problematic

This comment has been minimized.

Show comment
Hide comment
@Problematic

Problematic Feb 16, 2012

I believe this is an issue with the framework itself, and not this bundle in particular. I've run up against it as well, independently of this uploader, and I'm looking into it too.

I believe this is an issue with the framework itself, and not this bundle in particular. I've run up against it as well, independently of this uploader, and I'm looking into it too.

@bnlab

This comment has been minimized.

Show comment
Hide comment

bnlab commented Mar 30, 2012

+1 ;)

@inmarelibero

This comment has been minimized.

Show comment
Hide comment
@inmarelibero

inmarelibero Apr 2, 2012

Contributor

cool workaround, hoping it will be fixed soon

Contributor

inmarelibero commented Apr 2, 2012

cool workaround, hoping it will be fixed soon

@danielsan80

This comment has been minimized.

Show comment
Hide comment
@danielsan80

danielsan80 May 4, 2012

thanks guys! You have saved my day!

thanks guys! You have saved my day!

@fkrauthan

This comment has been minimized.

Show comment
Hide comment
@fkrauthan

fkrauthan Jul 13, 2012

I have this issue too maybe this could be fixed in this bundle.

I have this issue too maybe this could be fixed in this bundle.

@thomaskonrad

This comment has been minimized.

Show comment
Hide comment
@thomaskonrad

thomaskonrad Aug 14, 2012

Contributor

Same issue here. I guess this is because Doctrine doesn't detect any changes when only a file is uploaded as the "file" member variable is not mapped as a database column.

I'm just guessing, though.

Contributor

thomaskonrad commented Aug 14, 2012

Same issue here. I guess this is because Doctrine doesn't detect any changes when only a file is uploaded as the "file" member variable is not mapped as a database column.

I'm just guessing, though.

@MisterGlass

This comment has been minimized.

Show comment
Hide comment
@MisterGlass

MisterGlass Aug 16, 2012

This is an issue with doctrine, which wont persist the entity if it doesn't see any changes. I have an issue in the main Symfony project here: symfony/symfony#5150 (comment)

This is an issue with doctrine, which wont persist the entity if it doesn't see any changes. I have an issue in the main Symfony project here: symfony/symfony#5150 (comment)

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Aug 17, 2012

As I've explained to @aderuwe, this is kinda wrong.
This should not be pushed down to event listeners/subscribers attached to Doctrine ORM.
Since you're dealing with transitient fields here (not related with persistence) you are basically mixing persistence logic with the Bundle's logic. Don't do that.
Instead, I'd suggest to fix with something like

public function updateFile(UploadedFile $newFile, YourOwnServiceStuffYouRequire $service)

and perform all the persistence operations related to the file itself there.
The fix by @dustin10 is valid just to avoid breaking BC, but please stop doing this, and instead set a persistent field's value before flushing (Not in preFlush or other nasty hacks: really, just do it BEFORE interacting with Doctrine's UoW lifecycle!)

As I've explained to @aderuwe, this is kinda wrong.
This should not be pushed down to event listeners/subscribers attached to Doctrine ORM.
Since you're dealing with transitient fields here (not related with persistence) you are basically mixing persistence logic with the Bundle's logic. Don't do that.
Instead, I'd suggest to fix with something like

public function updateFile(UploadedFile $newFile, YourOwnServiceStuffYouRequire $service)

and perform all the persistence operations related to the file itself there.
The fix by @dustin10 is valid just to avoid breaking BC, but please stop doing this, and instead set a persistent field's value before flushing (Not in preFlush or other nasty hacks: really, just do it BEFORE interacting with Doctrine's UoW lifecycle!)

@ftassi

This comment has been minimized.

Show comment
Hide comment
@ftassi

ftassi Sep 2, 2012

Collaborator

Hi @Ocramius, I would like to discuss this with you, will you be available for a quick chat on this topic ?

Collaborator

ftassi commented Sep 2, 2012

Hi @Ocramius, I would like to discuss this with you, will you be available for a quick chat on this topic ?

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Sep 2, 2012

@ftassi ping me on IRC tomorrow eventually ( #doctrine )

Ocramius commented Sep 2, 2012

@ftassi ping me on IRC tomorrow eventually ( #doctrine )

@davidromani

This comment has been minimized.

Show comment
Hide comment
@davidromani

davidromani Nov 2, 2012

Same problem, still waiting...

Thanks.

Same problem, still waiting...

Thanks.

@stfalcon

This comment has been minimized.

Show comment
Hide comment
@stfalcon

stfalcon Nov 9, 2012

Contributor

@dustin10, maybe you have a normal solution? much time has passed.

Contributor

stfalcon commented Nov 9, 2012

@dustin10, maybe you have a normal solution? much time has passed.

@Sydney-o9

This comment has been minimized.

Show comment
Hide comment
@Sydney-o9

Sydney-o9 Nov 12, 2012

Doctrine uses identical operator (===) to compare changes between old and new values. The operator used on the same object with different data always return true. The best way is to clone the object.

Doctrine uses identical operator (===) to compare changes between old and new values. The operator used on the same object with different data always return true. The best way is to clone the object.

@stfalcon

This comment has been minimized.

Show comment
Hide comment
@stfalcon

stfalcon Nov 13, 2012

Contributor

@stof, maybe you can help resolve this problem?

Contributor

stfalcon commented Nov 13, 2012

@stof, maybe you can help resolve this problem?

@stfalcon

This comment has been minimized.

Show comment
Hide comment
@stfalcon

stfalcon Nov 21, 2012

Contributor

Guys, I researched this issue and think it's unsolvable problem for current architecture :(

This hack should be added to the documentation

Contributor

stfalcon commented Nov 21, 2012

Guys, I researched this issue and think it's unsolvable problem for current architecture :(

This hack should be added to the documentation

@Koc

This comment has been minimized.

Show comment
Hide comment
@Koc

Koc Nov 22, 2012

Contributor

maybe preFlush event can helps?

Contributor

Koc commented Nov 22, 2012

maybe preFlush event can helps?

@stfalcon

This comment has been minimized.

Show comment
Hide comment
@stfalcon

stfalcon Nov 22, 2012

Contributor

@Koc I tried. I couldn't get entity in preFlush event

Contributor

stfalcon commented Nov 22, 2012

@Koc I tried. I couldn't get entity in preFlush event

@Baachi

This comment has been minimized.

Show comment
Hide comment
@Baachi

Baachi Nov 22, 2012

Contributor

The only way to go is, to use the vich_uploader.storage service manually.

$form = $this->createForm(new AcmeType(), $entity);
$form->bind($request);

if ($form->isValid()) {
    // Upload the new file
    $storage = $this->container->get('vich_uploader.storage');
    $storage->upload($entity);

   // Save the new filename
   $om->persist($entity);
   $om->flush();

    return new RidirectRespone('/');
}

@Ocramius Right?

Contributor

Baachi commented Nov 22, 2012

The only way to go is, to use the vich_uploader.storage service manually.

$form = $this->createForm(new AcmeType(), $entity);
$form->bind($request);

if ($form->isValid()) {
    // Upload the new file
    $storage = $this->container->get('vich_uploader.storage');
    $storage->upload($entity);

   // Save the new filename
   $om->persist($entity);
   $om->flush();

    return new RidirectRespone('/');
}

@Ocramius Right?

@stfalcon

This comment has been minimized.

Show comment
Hide comment
@stfalcon

stfalcon Nov 22, 2012

Contributor

@Baachi maybe. but still doesn't work in SonataAdminBundle (from the box) :(

Contributor

stfalcon commented Nov 22, 2012

@Baachi maybe. but still doesn't work in SonataAdminBundle (from the box) :(

@Baachi

This comment has been minimized.

Show comment
Hide comment
@Baachi

Baachi Nov 22, 2012

Contributor

@stfalcon Hm i doesn't work with SonataAdminBundle.
But i use custom events to upload my images/files, so SonataAdminBundle should call his own events, too.

Contributor

Baachi commented Nov 22, 2012

@stfalcon Hm i doesn't work with SonataAdminBundle.
But i use custom events to upload my images/files, so SonataAdminBundle should call his own events, too.

@MisterGlass

This comment has been minimized.

Show comment
Hide comment
@MisterGlass

MisterGlass Nov 22, 2012

Maybe you can bake the hack into the sonata code?

Maybe you can bake the hack into the sonata code?

@stfalcon

This comment has been minimized.

Show comment
Hide comment
@stfalcon

stfalcon Nov 22, 2012

Contributor

@MisterGlass I thought about it. need to try

Contributor

stfalcon commented Nov 22, 2012

@MisterGlass I thought about it. need to try

@ftassi

This comment has been minimized.

Show comment
Hide comment
@ftassi

ftassi Nov 24, 2012

Collaborator

Right now the bundle is coupled to doctrine's lifecycle callback, which means that with no changes to the entity, no callback are called and no upload is triggered.

With this coupling there are two options to solve this problem:

  1. Add an updated field as suggested here
  2. Manually upload the file as suggested here

The second one is my favorite and in the future I would like to loose coupling with persistence layer, but I've not implemented this yet and I'm afraid that I'll not do it soonish.

Collaborator

ftassi commented Nov 24, 2012

Right now the bundle is coupled to doctrine's lifecycle callback, which means that with no changes to the entity, no callback are called and no upload is triggered.

With this coupling there are two options to solve this problem:

  1. Add an updated field as suggested here
  2. Manually upload the file as suggested here

The second one is my favorite and in the future I would like to loose coupling with persistence layer, but I've not implemented this yet and I'm afraid that I'll not do it soonish.

@ftassi ftassi closed this Nov 24, 2012

@thomaskonrad

This comment has been minimized.

Show comment
Hide comment
@thomaskonrad

thomaskonrad Nov 24, 2012

Contributor

I think that if there is no generic fix for the moment then the workaround(s) should be added to the documentation.

Contributor

thomaskonrad commented Nov 24, 2012

I think that if there is no generic fix for the moment then the workaround(s) should be added to the documentation.

@ftassi

This comment has been minimized.

Show comment
Hide comment
@ftassi

ftassi Nov 24, 2012

Collaborator

👍
We could add a "known issues" (or known limitations) section where we can explain this and why we can't use the id of the object for file name (ref: #14)

Any chance to work on a PR ?

Collaborator

ftassi commented Nov 24, 2012

👍
We could add a "known issues" (or known limitations) section where we can explain this and why we can't use the id of the object for file name (ref: #14)

Any chance to work on a PR ?

Mondane added a commit to Mondane/VichUploaderBundle that referenced this issue Jun 30, 2015

Update known_issues.md
Add 'instanceof'  check as written here: dustin10#8 (comment) to avoid this issue dustin10#297 (comment)

Koc added a commit to Koc/VichUploaderBundle that referenced this issue Sep 2, 2016

Koc added a commit to Koc/VichUploaderBundle that referenced this issue Sep 2, 2016

Koc added a commit to Koc/VichUploaderBundle that referenced this issue Sep 3, 2016

Koc added a commit to Koc/VichUploaderBundle that referenced this issue Sep 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment