[WIP] Refactor event system

[Baachi]

Hey,

I refactored the event system a little bit. Now you can configure which listeners should be triggerd and which not.
So if you have some issues with upload a file/image, like #8. You can disable the event listener and upload the file by hand.

Feedback, please!

Todo

• Tests
• Test it in a real application
• Make it workable with MongoDBODM

[Baachi]

@ftassi Any feedback?

[ioleo]

@Baachi nice one 👍

[nifr]

I recently came across the error where the upload handler was not triggered if only the image had been changed.
I was able to fix it in my case with the solution suggested in #96.

Now im back with the same situation in a new project... having this issue solved in the bundle would be nice.

@dustin10 Any comments/proposals on this PR ? I would be willing to help out if something's missing.

[ftassi]

Hi @Baachi thanks for the PR and sorry for late responding. Here are my thoughts about it.
I like the idea of being able to define which listener should be triggered, but I think your PR needs to be tweaked a bit before it can be merged.

We have 4 events that should trigger 4 different actions:

• prePersist and preUpdate is used for uploading the file
• preUpdate is also used for deleting the previous file if configured to
• postUpload is used for injecting the File instance in the Entity
• postRemove is used for deleting/unlinking the uploaded file

The user should be able to configure those 4 behaviours independently and I also would like to let the user free to define them globally or for at mapping level (specific mapping config should override globals).

Sadly we're not able to tag or untag listeners at runtime(are we?), I think we need to always call the listeners, pass the configuration data to the PropertyMapping class and use it to decide what to do for each uploadable field. This is an example of what I'm talking about (as the bundle already do something like this): https://github.com/dustin10/VichUploaderBundle/blob/master/Storage/AbstractStorage.php#L94

Does this make sense for you?

[ftassi]

should be delete_on_remove for consistence with mapping node
also delete_on_update is missing here

[Baachi]

@ftassi Attach the listeners at runtime is not easy, because we can not use the DIC. But your suggestions make sense and i will follow your suggestions. I hope i will find some time at this weekend to finish this PR.

[ftassi]

👌

[leopro]

a possible alternative solution could be listen preFlush event instead of preUpdate, something like this:

/**
 * I listen this event instead of preUpdate because 
 * if change only the not mapped property doctrine does not dispatch the preUpdate event
 */
public function preFlush(PreFlushEventArgs $args)
{
    $uow = $args->getEntityManager()->getUnitOfWork();
    $identities = $uow->getIdentityMap();

    foreach ($identities as $key => $identity) {
        foreach ($identity as $id => $entity) {
            if ($entity instanceof UploadableInterface) {
                //...
            }
        }
    }
}

I have taken this piece of code from a project in which I used an interface, but it's easy implement it via annotations

[sirian]

any news?

[Baachi]

@sirian Currently i'm very busy, but if you want send PR's to my branch and i will merge it, so it will be vissible here.
Maybe i will find some time at the weekend.

[K-Phoen]

What's the status of this PR?

[Baachi]

I'm currently haven't any project which use the VichUploaderBundle and currently i'm busy. There are many things todo, @ftassi want some more features before he will merge it.

[Baachi]

@K-Phoen can i close this PR?

[K-Phoen]

Yep :)

Do you think there are still things in your PR that we should backport into the actual codebase?

[Baachi]

I don't think so, you do a great job with this PR #183 .