Sonata\AdminBundle\Form\Type\ModelListType with media items breaks on adding a new

quisse commented 4 years ago

Environment

Sonata packages

$ composer show --latest 'sonata-project/*'
sonata-project/admin-bundle              3.57.0 3.57.0 The missing Symfony Admin Generator
sonata-project/block-bundle              3.18.3 3.18.3 Symfony SonataBlockBundle
sonata-project/cache                     1.1.1  2.0.1  Cache library
sonata-project/cache-bundle              2.4.2  3.1.0  This bundle provides caching services
sonata-project/core-bundle               3.17.2 3.17.2 Symfony SonataCoreBundle (abandoned)
sonata-project/datagrid-bundle           2.5.0  2.5.0  Symfony SonataDatagridBundle
sonata-project/doctrine-extensions       1.5.1  1.5.1  Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 3.13.0 3.13.0 Symfony Sonata / Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/easy-extends-bundle       2.5.0  2.5.0  Symfony SonataEasyExtendsBundle
sonata-project/exporter                  1.11.1 1.11.1 Lightweight Exporter library
sonata-project/intl-bundle               2.7.0  2.7.0  Symfony SonataIntlBundle
sonata-project/media-bundle              3.23.0 3.23.0 Symfony SonataMediaBundle
sonata-project/seo-bundle                2.10.0 2.10.0 Symfony SonataSeoBundle
sonata-project/user-bundle               4.5.1  4.5.1  Symfony SonataUserBundle

Symfony packages

$ composer show --latest 'symfony/*'
symfony/assetic-bundle        v2.8.2  v2.8.2  Integrates Assetic into Symfony2
Package symfony/assetic-bundle is abandoned, you should avoid using it. Use symfony/webpack-encore-pack instead.
symfony/http-client           v4.4.2  v4.4.2  Symfony HttpClient component
symfony/http-client-contracts v1.1.8  v1.1.8  Generic abstractions related to HTTP clients
symfony/mime                  v4.4.2  v4.4.2  A library to manipulate MIME messages
symfony/monolog-bundle        v2.12.1 v3.5.0  Symfony MonologBundle
symfony/polyfill-apcu         v1.13.1 v1.13.1 Symfony polyfill backporting apcu_* functions to lower PHP versions
symfony/polyfill-ctype        v1.13.1 v1.13.1 Symfony polyfill for ctype functions
symfony/polyfill-intl-icu     v1.13.1 v1.13.1 Symfony polyfill for intl's ICU-related data and classes
symfony/polyfill-intl-idn     v1.13.1 v1.13.1 Symfony polyfill for intl's idn_to_ascii and idn_to_utf8 functions
symfony/polyfill-mbstring     v1.13.1 v1.13.1 Symfony polyfill for the Mbstring extension
symfony/polyfill-php56        v1.13.1 v1.13.1 Symfony polyfill backporting some PHP 5.6+ features to lower PHP versions
symfony/polyfill-php70        v1.13.1 v1.13.1 Symfony polyfill backporting some PHP 7.0+ features to lower PHP versions
symfony/polyfill-php72        v1.13.1 v1.13.1 Symfony polyfill backporting some PHP 7.2+ features to lower PHP versions
symfony/polyfill-php73        v1.13.1 v1.13.1 Symfony polyfill backporting some PHP 7.3+ features to lower PHP versions
symfony/polyfill-util         v1.13.1 v1.13.1 Symfony utilities for portability of PHP codes
symfony/security-acl          v3.0.4  v3.0.4  Symfony Security Component - ACL (Access Control List)
symfony/service-contracts     v1.1.8  v1.1.8  Generic abstractions related to writing services
symfony/swiftmailer-bundle    v2.6.7  v3.4.0  Symfony SwiftmailerBundle
symfony/symfony               v3.4.36 v4.4.2  The Symfony PHP framework

PHP version

$ php -v
PHP 7.1.29 (cli) (built: May  3 2019 07:55:37) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.1.0, Copyright (c) 1998-2018 Zend Technologies
    with Zend OPcache v7.1.29, Copyright (c) 1999-2018, by Zend Technologies
    with Xdebug v2.6.0, Copyright (c) 2002-2018, by Derick Rethans

Subject

When trying to create a media object from a \Sonata\Form\Type\ModelListType, it fails.

Steps to reproduce

// ItemAdmin
        $formMapper->add(
                'media',
                ModelListType::class,
                ['label' => 'Beeld', 'required' => true],
                ['link_parameters' => ['context' => 'guide', 'filter[name][value]' => $title],]
            )
// Item.orm.yml
    manyToOne:
        media:
            targetEntity: Application\Sonata\MediaBundle\Entity\Media
            cascade: {  }
            mappedBy: null
            inversedBy: null
            joinColumns:
                media_id:
                    referencedColumnName: id

Try to create a new media item by selecting the image provider.

Expected results

A newly created media item.

Actual results

Symfony\Component\Debug\Exception\FatalThrowableError Type error: strtolower() expects parameter 1 to be string, null given 
    /var/www/myproject/vendor/sonata-project/media-bundle/src/Extra/Pixlr.php:189 strtolower
    /var/www/myproject/vendor/sonata-project/media-bundle/src/Extra/Pixlr.php:189 Sonata\MediaBundle\Extra\Pixlr::isEditable
    /var/www/myproject/vendor/twig/twig/src/Extension/CoreExtension.php:1495 twig_get_attribute
    /var/www/myproject/var/cache/prod/twig/80/805c74e8b29ad9fbe08e40b4535094e6461f0fa904c92ceff25bff6734d62a48.php:97 __TwigTemplate_f2e726aeef28659d31a29900d7113deb9bef4934e431afd8aa053140dd8b2394::block_form
    /var/www/myproject/vendor/twig/twig/src/Template.php:184 Twig\Template::displayBlock
    /var/www/myproject/var/cache/prod/twig/78/7876c855193d0901c25af72a5e4106d0f2313ac94481b05fda25ff1fb96eec36.php:67 __TwigTemplate_6c87fae9f60af765f56a6ff12184e65b0b0545a86edbc94e818ec5451a1a207f::block_content
    /var/www/myproject/vendor/twig/twig/src/Template.php:184 Twig\Template::displayBlock
    /var/www/myproject/var/cache/prod/twig/78/7876c855193d0901c25af72a5e4106d0f2313ac94481b05fda25ff1fb96eec36.php:45 __TwigTemplate_6c87fae9f60af765f56a6ff12184e65b0b0545a86edbc94e818ec5451a1a207f::doDisplay
    /var/www/myproject/vendor/twig/twig/src/Template.php:407 Twig\Template::displayWithErrorHandling
    /var/www/myproject/vendor/twig/twig/src/Template.php:380 Twig\Template::display
    /var/www/myproject/var/cache/prod/twig/84/84fce01592adc8ec0a6e71bd519819ea8de9a9d5212fef0fe32d39ccbf85e1d6.php:63 __TwigTemplate_16541f8c2d04c97a9ac2725683e900cf7c7d4ba165e4f1646e58e6f45c85af6c::doDisplay
    /var/www/myproject/vendor/twig/twig/src/Template.php:407 Twig\Template::displayWithErrorHandling
    /var/www/myproject/vendor/twig/twig/src/Template.php:380 Twig\Template::display
    /var/www/myproject/var/cache/prod/twig/80/805c74e8b29ad9fbe08e40b4535094e6461f0fa904c92ceff25bff6734d62a48.php:47 __TwigTemplate_f2e726aeef28659d31a29900d7113deb9bef4934e431afd8aa053140dd8b2394::doDisplay
    /var/www/myproject/vendor/twig/twig/src/Template.php:407 Twig\Template::displayWithErrorHandling
    /var/www/myproject/vendor/twig/twig/src/Template.php:380 Twig\Template::display
    /var/www/myproject/vendor/twig/twig/src/Template.php:392 Twig\Template::render
    /var/www/myproject/vendor/symfony/symfony/src/Symfony/Bridge/Twig/TwigEngine.php:50 Symfony\Bridge\Twig\TwigEngine::render
    /var/www/myproject/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php:284 Sonata\AdminBundle\Controller\CRUDController::originalRender
    /var/www/myproject/vendor/sonata-project/admin-bundle/src/Controller/CRUDController.php:129 Sonata\AdminBundle\Controller\CRUDController::renderWithExtraParams
    /var/www/myproject/vendor/sonata-project/admin-bundle/src/Controller/CRUDController.php:662 Sonata\AdminBundle\Controller\CRUDController::createAction
    /var/www/myproject/vendor/sonata-project/media-bundle/src/Controller/MediaAdminController.php:45 Sonata\MediaBundle\Controller\MediaAdminController::createAction
    /var/www/myproject/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpKernel.php:151 Symfony\Component\HttpKernel\HttpKernel::handleRaw
    /var/www/myproject/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpKernel.php:68 Symfony\Component\HttpKernel\HttpKernel::handle
    /var/www/myproject/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php:200 Symfony\Component\HttpKernel\Kernel::handle
    /var/www/myproject/web/app.php:21 [main]

(temporary) solution

Adding following lines to the extended media entity.

    public function getExtension()
    {
        return $this->getProviderReference()? parent::getExtension():'';
    }

greg0ire commented 4 years ago

Either this is wrong:

https://github.com/sonata-project/SonataMediaBundle/blob/0b793970580fc3e4dfbde9efeacac384e112609b/src/Model/MediaInterface.php#L324-L327 and we need to document that properly and adapt this piece of code: https://github.com/sonata-project/SonataMediaBundle/blob/0b793970580fc3e4dfbde9efeacac384e112609b/src/Extra/Pixlr.php#L183-L190 or there is a bug in implementation of MediaInterface. Does it make sense not to have an extension for a media?

quisse commented 4 years ago

AFAIK it doesn't make sense to have an extension when there is no media (= new empty object). I'm also in doubt if the Pixlr should do anything when there's no media.

github-actions[bot] commented 4 years ago

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

quisse commented 4 years ago

Not sure how to take this forward? For me this is currently out of scope.

VincentLanglet commented 4 years ago

public function getExtension()
    {
        $providerReference = $this->getProviderReference();
        if (!$providerReference) {
            return null;
        }

        // strips off query strings or hashes, which are common in URIs remote references
        return preg_replace('{(\?|#).*}', '', pathinfo($providerReference, PATHINFO_EXTENSION));
    }

Is not compatible with

/**
  * @return string
  */
public function getExtension();

I don't know the context of this bundle:

If you consider that null is not a valid value, you should create a PR to change it.
If you consider that null is a valida value, you should update the signature of the Interface. Then you should modify the implementation of Pixir::isEditable (and maybe others class/methods) to handle the null case.

sonata-project / SonataMediaBundle