sonata-project / SonataMediaBundle

Symfony SonataMediaBundle
https://docs.sonata-project.org/projects/SonataMediaBundle
MIT License
451 stars 495 forks source link

Only reference media in S3 is removed; thumbnails remain #504

Closed coryjb closed 9 years ago

coryjb commented 10 years ago

Only the reference media is removed on S3 when removing a media entity. The source is in a Doctrine2 behavior when removing entities (http://www.doctrine-project.org/jira/browse/DDC-1680).

Essentially, the $id for the media entity is null in BaseMediaEventSubscriber::postRemove(). Sonata's FormatThumbnail::generatePrivateUrl() expects to find $id, but instead it generates something like default/01/0001/thumb__admin.jpg because the now removed entity's $id is null, so the thumbnail is never found.

I am working on a PR, but let me know if there's a better way to approach this.

coryjb commented 10 years ago

I extended ImageProvider to remove thumbnails in preRemove() rather than postRemove(). I did not submit a PR for the bundle, though, as the issue remains with a null media ID after Doctrine2 removes the entity. The side effect is a lingering entity without a file.

<?php

namespace Mobile\MediaBundle\Provider;

use Sonata\MediaBundle\Model\MediaInterface;

class ImageProvider extends \Sonata\MediaBundle\Provider\ImageProvider
{
    /**
     * Remove thumbnails before removing from database; bug in SonataMediaBundle that
     * prevents media thumbnails from being removed because $id is null after
         * Doctrine's remove() so the thumbnail file path is never contains the ID.
     */
    public function preRemove(MediaInterface $media)
    {
        $path = $this->getReferenceImage($media);

        if ($this->getFilesystem()->has($path)) {
            $this->getFilesystem()->delete($path);
        }

        if ($this->requireThumbnails()) {
            $this->thumbnail->delete($this, $media);
        }
    }

    /**
     * {@inheritdoc}
     */
    public function postRemove(MediaInterface $media)
    {

    }
}
adc-scastro commented 10 years ago

I encountered the same problem, and that solution fixed it. Can you make a pull request with it ?

rande commented 10 years ago

@coryjb can you link the PR to this issue ?

coryjb commented 10 years ago

Thank you, @rpg600!

rande commented 10 years ago

Can you confirm the solution ? if so can you create a PR to move the code from the postUpdate to the preUpdate

rande commented 10 years ago

related solution https://github.com/sonata-project/SonataMediaBundle/pull/558

Filoz commented 10 years ago

Hi, I have the same problem, any news about that?

Thank you!

xaben commented 9 years ago

After some more testing, noticed reference media is removed only from the first folder, the real path is:

string 'default/0001/21/ce919284ff28bdacc549def78b7427ca48e10e8d.png' (length=60)

while the provider tries to delete:

string 'default/0001/01/ce919284ff28bdacc549def78b7427ca48e10e8d.png' (length=60)

So neither the media references (except from the first folder), nor any of the thumbnails are deleted from the filesystem. And it's not limited only to S3 as stated in this issue.

Haven't given much priority to this issue, but now after 21k medias inserted that weight about 9Gb this becomes a much bigger problem. Wonder how to clean it up now...

rande commented 9 years ago

@srascar Can you look to this issue ? @xaben Can you try to provide more information about the path issue ? not sure it is related to the current issue.

xaben commented 9 years ago

As @coryjb mentioned the postRemove event gets a media that no longer has an Id.

References are deleted for the first 1..999 Medias from the 0001/01 folder because of DefaultGenerator.php#40, on Media with ID 1000 and further the path should be 0001/02, but as the Id is not set it defaults back to 0001/01 and fails to delete the reference.

As for the thumbnails, they are not deleted for any media, as the Media Id is used in their naming.

The PR i proposed doesn't modify generatePrivateUrl() as @rpg600 in #558 , it just caches the list of reference/thumbnail files in preRemove when there is an Id, and on postRemove when the Id is no longer set inside the Media, it loops through the files and removes them.

This issue is not isolated to the S3 filesystem only. I tested against the Local Filesystem and the same happens.

rande commented 9 years ago

@xaben thanks for reporting this issue. We will fix that ASAP.

rande commented 9 years ago

Closing as fixed by #696