search

sonata-project / SonataAdminBundle

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

Incorrect Generic Typing for ModelManagerInterface #8158

Closed amacrobert-meq closed 3 months ago

amacrobert-meq commented 5 months ago

Environment

Sonata packages

show

``` $ composer show --latest 'sonata-project/*' Direct dependencies required in composer.json: sonata-project/admin-bundle 4.29.2 4.29.3 The missing Symfony Admin Generator sonata-project/doctrine-orm-admin-bundle 4.9.1 4.15.0 Integrate Doctrine ORM into the SonataAdminBundle sonata-project/entity-audit-bundle 1.11.0 1.16.1 Audit for Doctrine Entities Transitive dependencies not required in composer.json: sonata-project/block-bundle 5.1.0 5.1.0 Symfony SonataBlockBundle sonata-project/doctrine-extensions 1.18.1 2.3.0 Doctrine2 behavioral extensions sonata-project/exporter 3.3.0 3.3.0 Lightweight Exporter library sonata-project/form-extensions 1.20.0 2.3.0 Symfony form extensions sonata-project/twig-extensions 2.4.0 2.4.0 Sonata twig extensions ```

Symfony packages

show

``` $ composer show --latest 'symfony/*' Direct dependencies required in composer.json: symfony/browser-kit v5.4.31 v7.0.0 Simulates the behavior of a web browser, allowing you to make requ... symfony/console v5.4.34 v7.0.2 Eases the creation of beautiful and testable command line interfaces symfony/css-selector v5.4.26 v7.0.0 Converts CSS selectors to XPath expressions symfony/debug-bundle v4.4.37 v7.0.0 Provides a tight integration of the Symfony VarDumper component an... symfony/dotenv v5.4.34 v7.0.2 Registers environment variables from a .env file symfony/flex v1.21.4 v2.4.3 Composer plugin for Symfony symfony/framework-bundle v5.4.34 v7.0.2 Provides a tight integration between Symfony components and the Sy... symfony/mailer v5.4.34 v7.0.2 Helps sending emails symfony/maker-bundle v1.50.0 v1.52.0 Symfony Maker helps you create empty commands, controllers, form c... symfony/monolog-bundle v3.10.0 v3.10.0 Symfony MonologBundle symfony/phpunit-bridge v7.0.2 v7.0.2 Provides utilities for PHPUnit, especially user deprecation notice... symfony/process v5.4.34 v7.0.2 Executes commands in sub-processes symfony/runtime v5.4.26 v7.0.0 Enables decoupling PHP applications from global state symfony/security-bundle v5.4.34 v7.0.2 Provides a tight integration of the Security component into the Sy... symfony/serializer v5.4.34 v7.0.2 Handles serializing and deserializing data structures, including o... symfony/stopwatch v5.4.21 v7.0.0 Provides a way to profile code symfony/validator v5.4.34 v7.0.2 Provides tools to validate values symfony/var-dumper v5.4.29 v7.0.2 Provides mechanisms for walking through any arbitrary PHP variable symfony/web-profiler-bundle v5.4.34 v7.0.2 Provides a development tool that gives detailed information about ... symfony/yaml v5.4.31 v7.0.0 Loads and dumps YAML files Transitive dependencies not required in composer.json: symfony/asset v5.4.31 v7.0.0 Manages URL generation and versioning of web assets such as CSS st... symfony/cache v5.4.34 v7.0.2 Provides extended PSR-6, PSR-16 (and tags) implementations symfony/cache-contracts v2.5.2 v3.4.0 Generic abstractions related to caching symfony/config v5.4.31 v7.0.0 Helps you find, load, combine, autofill and validate configuration... symfony/dependency-injection v5.4.34 v7.0.2 Allows you to standardize and centralize the way objects are const... symfony/deprecation-contracts v3.4.0 v3.4.0 A generic function and convention to trigger deprecation notices symfony/doctrine-bridge v5.4.34 v7.0.2 Provides integration for Doctrine with various Symfony components symfony/dom-crawler v5.4.32 v7.0.0 Eases DOM navigation for HTML and XML documents symfony/error-handler v5.4.29 v7.0.0 Provides tools to manage errors and ease debugging PHP code symfony/event-dispatcher v5.4.34 v7.0.2 Provides tools that allow your application components to communica... symfony/event-dispatcher-contracts v3.4.0 v3.4.0 Generic abstractions related to dispatching event symfony/expression-language v5.4.21 v7.0.2 Provides an engine that can compile and evaluate expressions symfony/filesystem v5.4.25 v7.0.0 Provides basic utilities for the filesystem symfony/finder v5.4.27 v7.0.0 Finds files and directories via an intuitive fluent interface symfony/form v5.4.33 v7.0.1 Allows to easily create, process and reuse HTML forms symfony/http-client v5.4.34 v7.0.2 Provides powerful methods to fetch HTTP resources synchronously or... symfony/http-client-contracts v2.5.2 v3.4.0 Generic abstractions related to HTTP clients symfony/http-foundation v5.4.34 v7.0.0 Defines an object-oriented layer for the HTTP specification symfony/http-kernel v5.4.34 v7.0.2 Provides a structured process for converting a Request into a Resp... symfony/mime v5.4.26 v7.0.0 Allows manipulating MIME messages symfony/monolog-bridge v5.4.31 v7.0.0 Provides integration for Monolog with various Symfony components symfony/options-resolver v5.4.21 v7.0.0 Provides an improved replacement for the array_replace PHP function symfony/password-hasher v5.4.31 v7.0.0 Provides password hashing utilities symfony/polyfill-ctype v1.28.0 v1.28.0 Symfony polyfill for ctype functions symfony/polyfill-intl-grapheme v1.28.0 v1.28.0 Symfony polyfill for intl's grapheme_* functions symfony/polyfill-intl-icu v1.28.0 v1.28.0 Symfony polyfill for intl's ICU-related data and classes symfony/polyfill-intl-idn v1.28.0 v1.28.0 Symfony polyfill for intl's idn_to_ascii and idn_to_utf8 functions symfony/polyfill-intl-normalizer v1.28.0 v1.28.0 Symfony polyfill for intl's Normalizer class and related functions symfony/polyfill-mbstring v1.28.0 v1.28.0 Symfony polyfill for the Mbstring extension symfony/polyfill-php72 v1.28.0 v1.28.0 Symfony polyfill backporting some PHP 7.2+ features to lower PHP v... symfony/polyfill-php73 v1.28.0 v1.28.0 Symfony polyfill backporting some PHP 7.3+ features to lower PHP v... symfony/polyfill-php80 v1.28.0 v1.28.0 Symfony polyfill backporting some PHP 8.0+ features to lower PHP v... symfony/polyfill-php81 v1.28.0 v1.28.0 Symfony polyfill backporting some PHP 8.1+ features to lower PHP v... symfony/property-access v5.4.26 v7.0.0 Provides functions to read and write from/to an object or array us... symfony/property-info v5.4.24 v7.0.0 Extracts information about PHP class' properties using metadata of... symfony/psr-http-message-bridge v2.3.1 v7.0.2 PSR HTTP message bridge symfony/routing v5.4.34 v7.0.2 Maps an HTTP request to a set of configuration variables symfony/security-acl v3.3.3 v3.3.3 Symfony Security Component - ACL (Access Control List) symfony/security-core v5.4.30 v7.0.1 Symfony Security Component - Core Library symfony/security-csrf v5.4.27 v7.0.1 Symfony Security Component - CSRF Library symfony/security-guard v5.4.27 v5.4.27 Symfony Security Component - Guard symfony/security-http v5.4.31 v7.0.1 Symfony Security Component - HTTP Integration symfony/service-contracts v2.5.2 v3.4.1 Generic abstractions related to writing services symfony/string v5.4.34 v7.0.2 Provides an object-oriented API to strings and deals with bytes, U... symfony/translation v5.4.31 v7.0.2 Provides tools to internationalize your application symfony/translation-contracts v2.5.2 v3.4.1 Generic abstractions related to translation symfony/twig-bridge v5.4.34 v7.0.2 Provides integration for Twig with various Symfony components symfony/twig-bundle v5.4.31 v7.0.0 Provides a tight integration of Twig into the Symfony full-stack f... symfony/var-exporter v6.4.2 v7.0.2 Allows exporting any serializable PHP data structure to plain PHP ... ```

PHP version

$ php -v
PHP 8.2.15 (cli) (built: Jan 19 2024 23:54:57) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.2.15, Copyright (c) Zend Technologies
    with Zend OPcache v8.2.15, Copyright (c), by Zend Technologies

Subject

Summary

IDEs and static analyzers think that $author in this code is a Blog, not an Author:

/**
 * @extends AbstractAdmin<Blog>
 */
final class BlogAdmin extends AbstractAdmin
{
    protected function postUpdate(object $object): void
    {
        parent::postUpdate($object);
        $modelManager = $this->getModelManager();

        // Use the model manager to get an entity other than Blog
        $author = $modelManager->findOneBy(Author::class, ['id' => 1]);
    }

Detailed explanation

Sonata\AdminBundle\Model\ModelManagerInterface declares a class-level generic type T:

/**
 * A model manager is a bridge between the model classes and the admin functionality.
 *
 * @phpstan-template T of object
 */
interface ModelManagerInterface
{
    ...

This type is an entity class used to restrict various parameters and return values of the class's methods. It is set when an admin's type is specified like so:

/**
 * @extends AbstractAdmin<Blog>
 */
class BlogAdmin extends AbstractAdmin
{
    ...

...which specifies Sonata\AdminBundle\DependencyInjection\Admin\TaggedAdminInterface's generic type T, which is used in its definition of the return value of getModelManager():

    /**
     * @phpstan-return ModelManagerInterface<T>
     */
    public function getModelManager(): ModelManagerInterface;

That class-level type T for ModelManagerInterface is used for a number of methods, despite those methods being usable for any entity classes.

For example, in an admin that @extends AbstractAdmin<Blog>, calling $this->getModelManager()->findOneBy(Author::class, ['id' => 1]) is expected to return a Blog, but of course it returns an Author.

Minimal repository with the bug

https://github.com/amacrobert-meq/sonata-generic-type-example

Steps to reproduce

  1. Create a Sonata Admin Symfony project with at least 2 entities.
  2. Install PHPStan and set its level to at least 6 in phpstan.neon.
  3. Create an admin for one of the entities, specifying its generic type in PHPDoc (@extends AbstractAdmin<X> where X is the entity the admin is for)
  4. In any of the admin's methods, use $someOtherEntity = $this->getModelManager()->findOneBy(Y::class, []); where Y is an entity other than the admin's entity
  5. Run phpstan: vendor/bin/phpstan

Expected results

PHPStan resolves with no errors.

Actual results

PHPStan errors with the following:

 ------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   src/Admin/BlogAdmin.php
 ------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------
  24     Parameter #1 $class of method Sonata\AdminBundle\Model\ModelManagerInterface<App\Entity\Blog>::findOneBy() expects class-string<App\Entity\Blog>, string given.
 ------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------

Suggested solution

The generic type T on ModelManagerInterface should not be class-level, but method-level.

For example, this change would let IDEs and static analyzers know that the return value of findBy(A::class, []) is an array of A, without restricting the entity that is passed to findBy().

  /**
   * A model manager is a bridge between the model classes and the admin functionality.
-  *
-  * @phpstan-template T of object
   */
  interface ModelManagerInterface
  {
      ...
      /**
       * @param array<string, mixed> $criteria
       *
       * @return object[] all objects matching the criteria
       *
+      * @phpstan-template T of object
       * @phpstan-param class-string<T> $class
       * @phpstan-return T[]
       */
      public function findBy(string $class, array $criteria = []): array;
VincentLanglet commented 3 months ago

This was already discussed in https://github.com/sonata-project/SonataAdminBundle/pull/6721 and https://github.com/sonata-project/SonataAdminBundle/pull/7066

// Use the model manager to get an entity other than Blog
$author = $modelManager->findOneBy(Author::class, ['id' => 1]);

is technically not the proper way to get another entity because you cannot be sure the entity is managed by the same entityManager behind the modelManager implementation.

I would recommend to use $yourEntityManager->findOneBy() instead.

Also, a method-level template creates other issues as https://phpstan.org/r/f359d9d9-b98f-475e-9ec8-205070d14d0a.

Also, changing this, if it was decided as wanted, would be too little gain for too much work and there is not enought maintainer/contributor. So I'll close as not planned. Thanks for your understanding.