sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

Entity with several IDs (PK) get __toString and not getId from Objects #5620

Closed MAGlCUS closed 5 years ago

MAGlCUS commented 5 years ago

Environment

Sonata packages

$ composer show --latest 'sonata-project/*'
sonata-project/admin-bundle              3.51.0
sonata-project/block-bundle              3.15.0
sonata-project/cache                     2.0.1
sonata-project/core-bundle               3.17.0
sonata-project/datagrid-bundle           2.5.0
sonata-project/doctrine-extensions       1.3.0
sonata-project/doctrine-orm-admin-bundle 3.9.0
sonata-project/easy-extends-bundle       2.5.0
sonata-project/exporter                  2.0.1
sonata-project/user-bundle               4.4.0

Symfony packages

$ composer show --latest 'symfony/*'
symfony/acl-bundle                       v1.0.0
symfony/apache-pack                      v1.0.1
symfony/asset                            v4.3.2
symfony/browser-kit                      v4.3.2
symfony/cache                            v4.3.2
symfony/cache-contracts                  v1.1.5
symfony/config                           v4.3.2
symfony/console                          v4.3.2
symfony/css-selector                     v4.3.2
symfony/debug                            v4.3.2
symfony/debug-bundle                     v4.3.2
symfony/debug-pack                       v1.0.7
symfony/dependency-injection             v4.3.2
symfony/doctrine-bridge                  v4.3.2
symfony/dom-crawler                      v4.3.2
symfony/dotenv                           v4.3.2
symfony/event-dispatcher                 v4.3.2
symfony/event-dispatcher-contracts       v1.1.5
symfony/expression-language              v4.3.2
symfony/filesystem                       v4.3.2
symfony/finder                           v4.3.2
symfony/flex                             v1.4.1
symfony/form                             v4.3.2
symfony/framework-bundle                 v4.3.2
symfony/http-foundation                  v4.3.2
symfony/http-kernel                      v4.3.2
symfony/inflector                        v4.3.2
symfony/intl                             v4.3.2
symfony/maker-bundle                     v1.11.6
symfony/mime                             v4.3.2
symfony/monolog-bridge                   v4.3.2
symfony/monolog-bundle                   v3.4.0
symfony/options-resolver                 v4.3.2
symfony/orm-pack                         v1.0.6
symfony/phpunit-bridge                   v4.3.2
symfony/polyfill-intl-icu                v1.11.0
symfony/polyfill-intl-idn                v1.11.0
symfony/polyfill-mbstring                v1.11.0
symfony/polyfill-php72                   v1.11.0
symfony/polyfill-php73                   v1.11.0
symfony/process                          v4.3.2
symfony/profiler-pack                    v1.0.4
symfony/property-access                  v4.3.2
symfony/property-info                    v4.3.2
symfony/routing                          v4.3.2
symfony/security-acl                     v3.0.2
symfony/security-bundle                  v4.3.2
symfony/security-core                    v4.3.2
symfony/security-csrf                    v4.3.2
symfony/security-guard                   v4.3.2
symfony/security-http                    v4.3.2
symfony/serializer                       v4.3.2
symfony/serializer-pack                  v1.0.2
symfony/service-contracts                v1.1.5
symfony/stopwatch                        v4.3.2
symfony/swiftmailer-bundle               v3.2.8
symfony/templating                       v4.3.2
symfony/test-pack                        v1.0.6
symfony/translation                      v4.3.2
symfony/translation-contracts            v1.1.5
symfony/twig-bridge                      v4.3.2
symfony/twig-bundle                      v4.3.2
symfony/validator                        v4.3.2
symfony/var-dumper                       v4.3.2
symfony/var-exporter                     v4.3.2
symfony/web-link                         v4.3.2
symfony/web-profiler-bundle              v4.3.2
symfony/web-server-bundle                v4.3.2
symfony/yaml                             v4.3.2

PHP version

$ php -v
PHP 7.2.10

Subject

The documentation says that:

Note

For advanced usage, $id might be implemented as an object. The bundle will automatically resolve its string representation from the ID object using $entity->getId()->__toString() (if implemented) when needed (e.g., for generating url / rendering).

For example, in a use case where InnoDB-optimised binary UUIDs is implemented:

But if we have this use-case:

Entity class A {
  /**
     * @ORM\Id()
     * @ORM\GeneratedValue()
     * @ORM\Column(type="integer")
     */
  private $id;
  /**
     * @ORM\Column(type="string", length=100)
  */
  private $name;
  [.... OTHER PROPERTIES AND METHODS HERE....]
  public function __toString() {
    return $this->name;
  }
}

Entity class B {
  /**
     * @ORM\Id()
     * @ORM\GeneratedValue()
     * @ORM\Column(type="integer")
     */
  private $id;
  /**
     * @ORM\Column(type="string", length=100)
  */
  private $name;
  [.... OTHER PROPERTIES AND METHODS HERE....]
  public function __toString() {
    return $this->name;
  }
}

Entity class C {
  /**
     * @ORM\Id
     * @ORM\ManyToOne(targetEntity="App\Entity\classA", inversedBy="classC")
     * @ORM\JoinColumn(name="classA_id",referencedColumnName="id", nullable=false)
     */
    private $classA;

    /**
     * @ORM\Id
     * @ORM\ManyToOne(targetEntity="App\Entity\classB", inversedBy="classC")
     * @ORM\JoinColumn(name="classB_id",referencedColumnName="id", nullable=false)
     */
    private $classB;

    [.... OTHER PROPERTIES AND METHODS HERE....]
}

Then you make a sonata admin with this configuration in service.yaml:

service.yaml

admin.classC:
        class: App\Admin\classCAdmin
        arguments: [~, App\Entity\classC, ~]
        tags:
            - { name: sonata.admin, manager_type: orm, group: admin, label: classC}
        public: true

If you access to list Action, all the routes are generated with __toString() function of classA and classB. This is correct but not desirable behavior. It is correct if we have a use case like the one in the note (UUID Object to manage the ID), but it is incorrect if we have objects that are not exclusively used to maintain the classC ID.

In the use case exposed, sonata admin must generate URL with classA and classB getId.

When sonata admin use "admin.id(entity)" check if child id entities has __toString() function exposed, but first must verify if getId() is exposed.

In the worst case, sonata admin should check if the entity (classC) has its own getId, and thus could be more versatile in most cases (slugs, id, __toString, etc.).

Steps to reproduce

Create the classes and the admin of the previous paragraph and look at the URL in delete button or edit button.

Expected results

When sonata admin use "admin.id(entity)" check if child id entities has __toString() function exposed, but first must verify if getId() is exposed and generate URL with those values.

Actual results

The URLs are generated with __toString function of child classes.

MAGlCUS commented 5 years ago

The function with the "anormal" function is getIdentifierValues($entity) in sonata-project/doctrine-orm-admin-bundle/Model/ModelManager.php

public function getIdentifierValues($entity)
    {
        // Fix code has an impact on performance, so disable it ...
        //$entityManager = $this->getEntityManager($entity);
        //if (!$entityManager->getUnitOfWork()->isInIdentityMap($entity)) {
        //    throw new \RuntimeException('Entities passed to the choice field must be managed');
        //}

        $class = ClassUtils::getClass($entity);
        $metadata = $this->getMetadata($class);
        $platform = $this->getEntityManager($class)->getConnection()->getDatabasePlatform();

        $identifiers = [];

        foreach ($metadata->getIdentifierValues($entity) as $name => $value) {
            if (!\is_object($value)) {
                $identifiers[] = $value;

                continue;
            }

            if (method_exists($value, '__toString')) {
                $identifiers[] = (string) $value;

                continue;
            }

            $fieldType = $metadata->getTypeOfField($name);
            $type = $fieldType && Type::hasType($fieldType) ? Type::getType($fieldType) : null;
            if ($type) {
                $identifiers[] = $type->convertToDatabaseValue($value, $platform);

                continue;
            }

            $metadata = $this->getMetadata(ClassUtils::getClass($value));

            foreach ($metadata->getIdentifierValues($value) as $value) {
                $identifiers[] = $value;
            }
        }

        return $identifiers;
    }

if we modified the class adding:

foreach ($metadata->getIdentifierValues($entity) as $name => $value) {
            if (!\is_object($value)) {
                $identifiers[] = $value;

                continue;
            }

            if (method_exists($value, 'getId')) {
                $identifiers[] = (string) $value->getId();

                continue;
            }

            if (method_exists($value, '__toString')) {
                $identifiers[] = (string) $value;

                continue;
            }

All works fine.

MAGlCUS commented 5 years ago

I find a issue in SonataDoctrineORMAdminBundle (#923). Perhaps it's better follow that issue.

core23 commented 5 years ago

Can you try to write a PR with a bugfix @MAGlCUS ?

MAGlCUS commented 5 years ago

Can you try to write a PR with a bugfix @MAGlCUS ?

@core23, Yes, but it must be in SonataDoctrineORMAdminBundle project ;-)

MAGlCUS commented 5 years ago

Can you try to write a PR with a bugfix @MAGlCUS ?

@core23, here you have the PR.

phansys commented 5 years ago

Closed in favor of sonata-project/SonataDoctrineORMAdminBundle#928.