Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A new media is created after update an old one from Admin Form #1273

Open
anacona16 opened this issue Aug 2, 2017 · 33 comments
Open

A new media is created after update an old one from Admin Form #1273

anacona16 opened this issue Aug 2, 2017 · 33 comments
Labels

Comments

@anacona16
Copy link

anacona16 commented Aug 2, 2017

Environment

Sonata packages

$ composer show sonata-project/*
sonata-project/admin-bundle              3.20.1 The missing Symfony Admin Generator
sonata-project/block-bundle              3.3.2  Symfony SonataBlockBundle
sonata-project/cache                     1.0.7  Cache library
sonata-project/core-bundle               3.4.0  Symfony SonataCoreBundle
sonata-project/datagrid-bundle           2.2.1  Symfony SonataDatagridBundle
sonata-project/doctrine-extensions       1.0.2  Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 3.1.6  Symfony Sonata / Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/easy-extends-bundle       2.2.0  Symfony SonataEasyExtendsBundle
sonata-project/exporter                  1.7.1  Lightweight Exporter library
sonata-project/media-bundle              3.6.0  Symfony SonataMediaBundle
sonata-project/notification-bundle       3.1.0  Symfony SonataNotificationBundle

Symfony packages

$ composer show symfony/*
symfony/assetic-bundle     v2.8.2  Integrates Assetic into Symfony2
symfony/monolog-bundle     v2.12.1 Symfony MonologBundle
symfony/phpunit-bridge     v3.3.6  Symfony PHPUnit Bridge
symfony/polyfill-intl-icu  v1.4.0  Symfony polyfill for intl's ICU-related data and classes
symfony/polyfill-mbstring  v1.4.0  Symfony polyfill for the Mbstring extension
symfony/polyfill-php56     v1.4.0  Symfony polyfill backporting some PHP 5.6+ features to lower PHP versions
symfony/polyfill-php70     v1.4.0  Symfony polyfill backporting some PHP 7.0+ features to lower PHP versions
symfony/polyfill-util      v1.4.0  Symfony utilities for portability of PHP codes
symfony/security-acl       v3.0.0  Symfony Security Component - ACL (Access Control List)
symfony/swiftmailer-bundle v2.6.3  Symfony SwiftmailerBundle
symfony/symfony            v3.3.6  The Symfony PHP framework

PHP version

$ php -v
PHP 5.6.30-12~ubuntu16.04.1+deb.sury.org+1 (cli) 
Copyright (c) 1997-2016 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2016 Zend Technologies
    with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2016, by Zend Technologies

Subject

A new media is created after update an old one. There is not a way to set new_on_update to false

Steps to reproduce

Create a new media (image, file, YouTube, Vimeo, etc) then update the same media uploading a new file or image or url.

Expected results

The same record should be updated with new metadata information.

Actual results

A new media is created, and the metadata for the old one is updated too including the file or image or url.

Request log

####################################################################################################
#######################################  02/Aug/2017 08:05:47  #####################################
####################################################################################################
___ REQUEST ________________________________________________________________________________________
Matched route "admin_sonata_media_media_edit".
--> route: admin_sonata_media_media_edit
--> route_parameters:
      _controller: 'Sonata\MediaBundle\Controller\MediaAdminController::editAction'
      _sonata_admin: sonata.media.admin.media
      _sonata_name: admin_sonata_media_media_edit
      id: '28'
      _route: admin_sonata_media_media_edit
--> request_uri: 'http://symfony_test.dev/app_dev.php/admin/sonata/media/media/28/edit?context=default&hide_context=0&uniqid=s5981cd655cd95'
--> method: POST
___ (!) SECURITY ___________________________________________________________________________________
Populated the TokenStorage with an anonymous Token.
___ DOCTRINE _______________________________________________________________________________________
SELECT t0.name AS name_1, t0.description AS description_2, t0.enabled AS enabled_3,
  t0.provider_name AS provider_name_4, t0.provider_status AS provider_status_5,
  t0.provider_reference AS provider_reference_6, t0.provider_metadata AS provider_metadata_7,
  t0.width AS width_8, t0.height AS height_9, t0.length AS length_10, t0.content_type AS
  content_type_11, t0.content_size AS content_size_12, t0.copyright AS copyright_13, t0.author_name
  AS author_name_14, t0.context AS context_15, t0.cdn_is_flushable AS cdn_is_flushable_16,
  t0.cdn_flush_identifier AS cdn_flush_identifier_17, t0.cdn_flush_at AS cdn_flush_at_18,
  t0.cdn_status AS cdn_status_19, t0.updated_at AS updated_at_20, t0.created_at AS created_at_21,
  t0.id AS id_22 FROM media__media t0 WHERE t0.id = ?
--> 'query params': ['28']
___ DOCTRINE _______________________________________________________________________________________
"START TRANSACTION"
___ DOCTRINE _______________________________________________________________________________________
INSERT INTO media__media (name, description, enabled, provider_name, provider_status,
  provider_reference, provider_metadata, width, height, length, content_type, content_size,
  copyright, author_name, context, cdn_is_flushable, cdn_flush_identifier, cdn_flush_at, cdn_status,
  updated_at, created_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--> 'query params': { 1: 'Conoce el framework PHP Symfony', 2: null, 3: false, 4: sonata.media.provider.youtube, 5: 1, 6: 2XFiTXbOPLk, 7: { thumbnail_height: 360, thumbnail_url: 'https://i.ytimg.com/vi/2XF [...]', provider_url: 'https://www.youtube.com/', width: 480, type: video, author_name: DesarrolloWeb.com, height: 270, version: '1.0', thumbnail_width: 480, title: 'Conoce el framework PHP Symfony', author_url: 'https://www.youtube.com/us [...]', provider_name: YouTube, html: '<iframe width="480" height [...]' }, 8: 480, 9: 270, 10: null, 11: video/x-flv, 12: null, 13: null, 14: DesarrolloWeb.com, 15: default, 16: null, 17: null, 18: null, 19: null, 20: '2017-08-02T08:05:48-05:00', 21: '2017-08-02T08:05:48-05:00' }
___ DOCTRINE _______________________________________________________________________________________
UPDATE media__media SET provider_reference = ?, cdn_is_flushable = ?, name = ?, provider_metadata
  = ?, author_name = ?, cdn_status = ?, updated_at = ? WHERE id = ?
--> 'query params': [2XFiTXbOPLk, true, 'Conoce el framework PHP Symfony', { author_url: 'https://www.youtube.com/us [...]', thumbnail_width: 480, html: '<iframe width="480" height [...]', author_name: DesarrolloWeb.com, title: 'Conoce el framework PHP Symfony', version: '1.0', thumbnail_url: 'https://i.ytimg.com/vi/2XF [...]', width: 480, provider_name: YouTube, height: 270, provider_url: 'https://www.youtube.com/', type: video, thumbnail_height: 360 }, DesarrolloWeb.com, 3, '2017-08-02T08:05:49-05:00', 28]
___ DOCTRINE _______________________________________________________________________________________
"COMMIT"


####################################################################################################
#######################################  02/Aug/2017 08:05:49  #####################################
####################################################################################################
___ REQUEST ________________________________________________________________________________________
Matched route "admin_sonata_media_media_edit".
--> route: admin_sonata_media_media_edit
--> route_parameters:
      _controller: 'Sonata\MediaBundle\Controller\MediaAdminController::editAction'
      _sonata_admin: sonata.media.admin.media
      _sonata_name: admin_sonata_media_media_edit
      id: '29'
      _route: admin_sonata_media_media_edit
--> request_uri: 'http://symfony_test.dev/app_dev.php/admin/sonata/media/media/29/edit?context=default&hide_context=0'
--> method: GET
___ (!) SECURITY ___________________________________________________________________________________
Populated the TokenStorage with an anonymous Token.
___ DOCTRINE _______________________________________________________________________________________
SELECT t0.name AS name_1, t0.description AS description_2, t0.enabled AS enabled_3,
  t0.provider_name AS provider_name_4, t0.provider_status AS provider_status_5,
  t0.provider_reference AS provider_reference_6, t0.provider_metadata AS provider_metadata_7,
  t0.width AS width_8, t0.height AS height_9, t0.length AS length_10, t0.content_type AS
  content_type_11, t0.content_size AS content_size_12, t0.copyright AS copyright_13, t0.author_name
  AS author_name_14, t0.context AS context_15, t0.cdn_is_flushable AS cdn_is_flushable_16,
  t0.cdn_flush_identifier AS cdn_flush_identifier_17, t0.cdn_flush_at AS cdn_flush_at_18,
  t0.cdn_status AS cdn_status_19, t0.updated_at AS updated_at_20, t0.created_at AS created_at_21,
  t0.id AS id_22 FROM media__media t0 WHERE t0.id = ?
--> 'query params': ['29']

As you can see an INSERT INTO media__media statement is executed, also an UPDATE media__media statement is executed later.

Problems found

BaseMediaAdmin class is using the ProviderDataTransformer class but is not setting value for new_on_update flag and this value is always true.

I don't see a way to set that value to false.

Proposed solution

In this method check if $media->getId() === null if is null then set 'new_on_update' => true if not then set 'new_on_update' => false, and pass a new array argument this class ProviderDataTransformer

This solution won't affect to sonata_media_type form type.

Issues related

#974 but using form type: sonata_media_type

@anacona16
Copy link
Author

anacona16 commented Aug 2, 2017

Proposed solution

If I'm updating a media from an admin form that's what I want to do: update it, right? So, I think the best option is pass 'new_on_update' => false always

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2017

new_on_update defaults to true, but it looks like you should be able to set it: https://github.com/sonata-project/SonataMediaBundle/blob/fabd14b939267ab9ef622963f14aed1cf26caa4c/Resources/doc/reference/form.rst .

@anacona16
Copy link
Author

anacona16 commented Aug 2, 2017

@greg0ire yes it's possible but only if you use the form sonata_media_type, but is not possible to set it from admin form.

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2017

Have you checked the contents of this variable at runtime?

@anacona16
Copy link
Author

It's always true because none option is passed from here https://github.com/sonata-project/SonataMediaBundle/blob/3.x/Admin/BaseMediaAdmin.php#L197

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2017

Yeah but options are not passed on construct, they are passed when calling getOptions. You can throw an exception in that method, that will give you a stack trace, helpful to see where $options comes from.

@anacona16
Copy link
Author

anacona16 commented Aug 2, 2017

This is the stack trace:

Exception:

  at vendor/sonata-project/media-bundle/Form/DataTransformer/ProviderDataTransformer.php:151
  at Sonata\MediaBundle\Form\DataTransformer\ProviderDataTransformer->getOptions(array())
     (vendor/sonata-project/media-bundle/Form/DataTransformer/ProviderDataTransformer.php:48)
  at Sonata\MediaBundle\Form\DataTransformer\ProviderDataTransformer->__construct(object(Pool), 'Application\\Sonata\\MediaBundle\\Entity\\Media')
     (vendor/sonata-project/media-bundle/Admin/BaseMediaAdmin.php:197)
  at Sonata\MediaBundle\Admin\BaseMediaAdmin->configureFormFields(object(FormMapper))
     (app/cache/dev/classes.php:3228)
  at Sonata\AdminBundle\Admin\AbstractAdmin->defineFormBuilder(object(FormBuilder))
     (app/cache/dev/classes.php:3222)
  at Sonata\AdminBundle\Admin\AbstractAdmin->getFormBuilder()
     (app/cache/dev/classes.php:4227)
  at Sonata\AdminBundle\Admin\AbstractAdmin->buildForm()
     (app/cache/dev/classes.php:3261)
  at Sonata\AdminBundle\Admin\AbstractAdmin->getForm()
     (vendor/sonata-project/admin-bundle/Controller/CRUDController.php:255)
  at Sonata\AdminBundle\Controller\CRUDController->editAction('67')
  at call_user_func_array(array(object(MediaAdminController), 'editAction'), array('67'))
     (app/cache/dev/classes.php:18855)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Request), 1)
     (app/cache/dev/classes.php:18810)
  at Symfony\Component\HttpKernel\HttpKernel->handle(object(Request), 1, true)
     (vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php:171)
  at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
     (web/app_dev.php:12)

As you can see, options is empty.

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2017

Indeed. Please have a look at the calling function to know why it is empty there, and so on.

@anacona16
Copy link
Author

anacona16 commented Aug 2, 2017

The option in the __construct method for ProviderDataTransformer is by default empty: https://github.com/sonata-project/SonataMediaBundle/blob/3.x/Form/DataTransformer/ProviderDataTransformer.php#L45

From here: the transformers is being calling without options: https://github.com/sonata-project/SonataMediaBundle/blob/3.x/Admin/BaseMediaAdmin.php#L197

I know you said that options are not passed on construct, but here https://github.com/sonata-project/SonataMediaBundle/blob/3.x/Form/Type/MediaType.php#L73 in the sonata_media_type form they are passing the options on construct.

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2017

You're right, reading your stack trace, it looks like getOptions is called in the constructor.

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2017

I think the right fix would be to have a property on the admin to configure that, and expose it in the configuration.

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2017

Or maybe it worked as you expected before? Can you check previous versions of the media bundle?

@anacona16
Copy link
Author

Maybe you're right, but my question is, if you have access to the admin form (sonata admin interface) and you choose to update a media why a new media is created?

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2017

I don't know or use this bundle, but the answer seems to be in the docs

then the media will be overwritten and related entities can be affected by this change.

@anacona16
Copy link
Author

anacona16 commented Aug 2, 2017

Of course, by relationships with other entities. But, I think they're talking about the asset not about the entity.

Here: http://demo.sonata-project.org/ it's working as expected, can you confirm which version are installed for that demo?

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2017

I don't know, but I think it is based on the sandbox, so you can have a look.

I think they're talking about the asset no the entity.

I don't get it. It's clearly written

related entities

not assets...

@anacona16
Copy link
Author

anacona16 commented Aug 2, 2017

You're right. I meant, I have a Product entity with:

id -> integer
name -> string
image -> ManyToOne with `Sonata\MediaBundle\Entity\Media`

--- Rows table_products ---
Product # 123 has attached an image # 321.

Media Entity # 321 have and Image (asset) with a content X.

If I updated the registry and I upload a new image (asset) with a content Y, of course the related entity will be affected, but the change will be only the asset content.

I'm installing the sandbox project on my local.

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2017

If it works as you expect in the sandbox, then it must be a bug. Please use git bisect to find the commit that introduced it. The author will be more able than me to help you with this.

@anacona16
Copy link
Author

I tried to test different versions but was almost impossible, there are a lot of dependencies and incompatibilities between bundles. I think I'll override that method in the Application/SonataBundle.

@anacona16
Copy link
Author

anacona16 commented Aug 3, 2017

I found the problem, is not an issue from media-bundle. A recent commit on admin-bundle is causing this issue: see here sonata-project/SonataAdminBundle@9655c1e#commitcomment-22646949

They know about the issue but I don't see any solution at the moment.

@greg0ire
Copy link
Contributor

greg0ire commented Aug 3, 2017

Oh right... any idea how to fix this?

@anacona16
Copy link
Author

anacona16 commented Aug 3, 2017

The quick solution maybe is this: #1273 (comment)

But I'm not sure if is the best solution.

@greg0ire
Copy link
Contributor

greg0ire commented Aug 3, 2017

Sorry I meant "fixing sonata-project/SonataAdminBundle@9655c1e#commitcomment-22646949"

@featuriz
Copy link

featuriz commented Nov 16, 2017

I got temporary solution for this problem.
https://github.com/sonata-project/SonataMediaBundle/blob/3.x/src/Form/DataTransformer/ProviderDataTransformer.php#L103
the comment is "// create a new media to avoid erasing other media or not ..."

To solve this problem (Stop creating new one, and delete old file/thumbnail and update) , I modified ProviderDataTransformer.php as

// create a new media to avoid erasing other media or not ...
//        $newMedia = $this->options['new_on_update'] ? new $this->class() : $media;

//        $newMedia->setProviderName($media->getProviderName());
//        $newMedia->setContext($media->getContext());
//        $newMedia->setBinaryContent($binaryContent);

//        if (!$newMedia->getProviderName() && $this->options['provider']) {
//            $newMedia->setProviderName($this->options['provider']);
//        }
//
//        if (!$newMedia->getContext() && $this->options['context']) {
//            $newMedia->setContext($this->options['context']);
//        }

        $provider = $this->pool->getProvider($media->getProviderName());

        try {
            $provider->transform($media);
        } catch (\Exception $e) { // NEXT_MAJOR: When switching to PHP 7+, change this to \Throwable
            // #1107 We must never throw an exception here.
            // An exception here would prevent us to provide meaningful errors through the Form
            // Error message inspired from Monolog\ErrorHandler
            $this->logger->error(
                sprintf('Caught Exception %s: "%s" at %s line %s', get_class($e), $e->getMessage(), $e->getFile(), $e->getLine()),
                ['exception' => $e]
            );
        }

        return $media;

This solves my problem now. At the same time the thumbnails are not deleted (This is a new problem).
New Problem
To solve this I override the method postUpdate() from the class Sonata\MediaBundle\Provider\ImageProvider with the code.

public function preRemove(MediaInterface $media) {
        if ($this->requireThumbnails()) {
            $this->thumbnail->delete($this, $media);
        }
    }

    public function postUpdate(MediaInterface $media) {
        if (!$media->getBinaryContent() instanceof \SplFileInfo) {
            return;
        }

        // Delete the current file from the FS
        $oldMedia = clone $media;
        // if no previous reference is provided, it prevents
        // Filesystem from trying to remove a directory
        if ($media->getPreviousProviderReference() !== null) {
            $oldMedia->setProviderReference($media->getPreviousProviderReference());

            $path = $this->getReferenceImage($oldMedia);

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

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

        $this->fixBinaryContent($media);

        $this->setFileContents($media);

        $this->generateThumbnails($media);

        $media->resetBinaryContent();
    }

There is already a problem regarding removing thumbnail in 2.x. But 3.x missing this line.
Issue link : #504 (comment)

@FireLizard
Copy link

A possible solution: removing the ProviderDataTransformer and add it with new options (e.g. new_on_update) to the FormBuilder.

Working example:

<?php
namespace ExtendedBundles\MediaBundle\Admin;

use Sonata\AdminBundle\Form\FormMapper;
use Sonata\MediaBundle\Admin\BaseMediaAdmin;
use Sonata\MediaBundle\Form\DataTransformer\ProviderDataTransformer;
use Symfony\Component\Form\DataTransformerInterface;
use Symfony\Component\Form\FormBuilderInterface;

/**
 * Class MediaAdmin
 *
 * @package ExtendedBundles\MediaBundle\Admin
 */
class MediaAdmin extends BaseMediaAdmin
{
    /**
     * @inheritDoc
     */
    protected function configureFormFields(FormMapper $formMapper)
    {
        parent::configureFormFields($formMapper);

        $builder = $formMapper->getFormBuilder();
        $transformers = $builder->getModelTransformers();

        // Remove ProviderDataTransformer from array.
        $transformers = array_filter(
            $transformers,
            function (DataTransformerInterface $transformer){
                return ! $transformer instanceof ProviderDataTransformer;
            }
        );

        // Remove all ModelTransformers and add filtered array of transformers.
        $builder->resetModelTransformers();
        array_walk(
            $transformers,
            function (DataTransformerInterface $transformer, $key, FormBuilderInterface $builder){
                $builder->addModelTransformer($transformer, true);
            },
            $builder
        );

        // Add a new ProviderDataTransformer with customized options.
        $builder->addModelTransformer(
            new ProviderDataTransformer(
                $this->pool,
                $this->getClass(),
                [ 'new_on_update' => false ]
            ),
            true
        );
    }
}

What do you think?

Greetings
FireLizard

@fafiebig
Copy link

fafiebig commented Feb 8, 2018

you can reset your transformers in the media related admin method like so:

public function configureFormFields(FormMapper $formMapper)
{
    parent::configureFormFields($formMapper);

    $formMapper->getFormBuilder()->resetModelTransformers();
    $formMapper->getFormBuilder()->addModelTransformer(
        new ProviderDataTransformer(
            $this->pool,
            $this->getClass(),
            array(
                'new_on_update' => false
            )
        ),
        true 
    );
}

EDIT: Sure, you need to add your own transformers again.

@FireLizard
Copy link

But that would reset/remove all other ModelTransformers, wouldn't it?

@eshishko
Copy link

eshishko commented May 23, 2018

Have same problem. After adding new ProviderDataTransformer with new_on_update false option, there is not deleting old files on update, like it was mentioned in #1273 (comment). Can we add some check before clearing previousProviderReference #1434?

public function reverseTransform($media)
{
    \\...
    if ($this->options['new_on_update']) {
        $newMedia->setProviderName($media->getProviderName());
        $newMedia->setContext($media->getContext());
        $newMedia->setBinaryContent($binaryContent);
    }
    \\...
}

Then previousProviderReference will not be null, and will be removed. However, thumbnail is not removing.

@lukepass
Copy link

I experienced the same problem today after updating a OneToOne and replacing it in the Sonata Media form.

@skydiablo
Copy link

skydiablo commented Jul 22, 2019

same here, any new suggestions? in a ManyToOne relation...

@edocabhi
Copy link

edocabhi commented Nov 8, 2019

I have the following three entities

  1. property
  2. property_has_media (many to one relation with property and media)
  3. media

I was trying to edit the media from the property admin itself and facing the same issue.
Removing 'by_reference' => false from the property_has_media admin configureFormFields method made it work like expected for me.

This also creates new thumbnails when the media image is replaced by a new on in the embedded admin form and all db entries are updated correctly.

I also have the options new_on_update and empty_on_new set as false for both property and property_has_media admin.

symfony 4.3

@stale
Copy link

stale bot commented Jan 30, 2020

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@jordisala1991
Copy link
Member

I am reopening this one. I can confirm a really strange behavior when you edit a media in the media admin (not in a relation through SonataMediaType). A new media is created, the old one is also updated...

The fix adding the new_on_update seems to be working, but I have not checked all the cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests