Skip to content

Keep QueryBuilderType across method calls #266

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

Closed
Khartir opened this issue Jan 24, 2022 · 4 comments · Fixed by #288
Closed

Keep QueryBuilderType across method calls #266

Khartir opened this issue Jan 24, 2022 · 4 comments · Fixed by #288

Comments

@Khartir
Copy link
Contributor

Khartir commented Jan 24, 2022

I don't know if this is currently even possible, but it would be nice, if Phpstan could determine the type of the QueryBuilder, even if it is defined in another method. Currently something like this results in a mixed return type:

    /**
     * @return array<Entity>
     */
    public function reuseQueryBuilder(): array
    {
        return $this->getQueryBuilder()
            ->getQuery()
            ->getResult();
    }

    private function getQueryBuilder(): QueryBuilder
    {
        return $this->createQueryBuilder('test');
    }
@ondrejmirtes
Copy link
Member

Hi, AFAIK this is what searchOtherMethodsForQueryBuilderBeginning setting does, and it's even on by default:

searchOtherMethodsForQueryBuilderBeginning: true
so I'm not sure why it doesn't work for you?

Feel free to pair it with reportDynamicQueryBuilders: true (

reportDynamicQueryBuilders: false
) - you'll learn which query builders could not be determined by PHPStan because their code is too complicated.

@Khartir
Copy link
Contributor Author

Khartir commented Jan 25, 2022

OK, my original error was obviously more complex than this example and has been resolved with version 1.2.7. So thank you very much for that.
However, I noticed that in this simple case there is another problem. With reportDynamicQueryBuilders: true this logs "Could not analyse QueryBuilder with unknown beginning."
So I looked into this for a bit and I think I found the beginning of the reason, why this doesn't work. In this simple example, when
QueryBuilderDqlRule tries to resolve the type of getQuery, $this->getQueryBuilder() is a simple ObjectType, not SimpleQueryBuilderType.
If I add a method call on the QueryBuilder before calling getQuery, the type is properly resolved and no errors occur:

    /**
     * @return array<Entity>
     */
    public function reuseQueryBuilder(): array
    {
        return $this->getQueryBuilder()
            ->andWhere('test.foo = 1')
            ->getQuery()
            ->getResult();
    }

    private function getQueryBuilder(): QueryBuilder
    {
        return $this->createQueryBuilder('test');
    }

@alexander-schranz
Copy link

alexander-schranz commented Jan 26, 2022

After seeing the tweet here. I tried to give it a try on local project. I think I'm running into the same issue:

Which Repositories look like this: https://github.com/alexander-schranz/hexagonal-architecture-study/blob/e0b6cd46085e0cb548d02e648ad6d86255e9c301/src/event/src/Infrastructure/ORM/Doctrine/Repository/EventRepository.php#L136

In the findOneBy the result is still mixed:

$queryBuilder = $this->createQueryBuilder($filters);

$query = $queryBuilder->getQuery();
$result = $query->getResult();

\PHPStan\dumpType($result);

I tried to add a generic to the method return type e.g.:

* @return QueryBuilder<MyEntity>

But this ends still in mixed and a generic error:

  60     Dumped type: mixed
  119    PHPDoc tag @return contains generic type Doctrine\ORM\QueryBuilder<App\Event\Domain\Model\Event> but class Doctrine\ORM\QueryBuilder is not generic.

The reportDynamicQueryBuilders ends with:

Could not analyse QueryBuilder with unknown beginning.

I think it would be nice if QueryBuilder<T> could be used to hint phpstan about which entity this queryBuilder is for.

Maybe something which could be added to the doctrine codebase, not sure if the ORM QueryBuilder/Query can be a generic. /cc @derrabus

More details phpstan.neon, install versions, ...

My Package Versions related to phpstan:

jangregor/phpstan-prophecy                 1.0.0       Provides a phpstan/phpstan extension for phpspec/prophecy
phpstan/phpstan                            1.4.2       PHPStan - PHP Static Analysis Tool
phpstan/phpstan-doctrine                   1.2.7       Doctrine extensions for PHPStan
phpstan/phpstan-phpunit                    1.0.0       PHPUnit extensions and rules for PHPStan
phpstan/phpstan-symfony                    1.1.2       Symfony Framework extensions and rules for PHPStan
phpstan/phpstan-webmozart-assert           1.0.8       PHPStan webmozart/assert extension
thecodingmachine/phpstan-strict-rules      v1.0.0      A set of additional rules for PHPStan based on best practices followed at TheCodingMachine

My phpstan.neon

includes:
    - vendor/jangregor/phpstan-prophecy/extension.neon
    - vendor/phpstan/phpstan-symfony/extension.neon
    - vendor/phpstan/phpstan-doctrine/extension.neon
    - vendor/phpstan/phpstan-doctrine/rules.neon
    - vendor/phpstan/phpstan-phpunit/extension.neon
    - vendor/phpstan/phpstan-phpunit/rules.neon
    - vendor/phpstan/phpstan-webmozart-assert/extension.neon
    - vendor/thecodingmachine/phpstan-strict-rules/phpstan-strict-rules.neon
    - phpstan-baseline.neon

parameters:
    paths:
        - src
        - tests
        - config
    level: max
    excludePaths:
        - %currentWorkingDirectory%/vendor/*
        - %currentWorkingDirectory%/var/*
    doctrine:
        objectManagerLoader: tests/phpstan/object-manager.php
    symfony:
        container_xml_path: %currentWorkingDirectory%/var/cache/dev/App_KernelDevDebugContainer.xml
        console_application_loader: tests/phpstan/console-application.php

My object-manager.php:

<?php

declare(strict_types=1);

use App\Kernel;
use Doctrine\ORM\EntityManager;
use Doctrine\ORM\Events;
use Doctrine\ORM\Tools\ResolveTargetEntityListener;
use Symfony\Component\DependencyInjection\ContainerInterface;

require \dirname(__DIR__) . '/bootstrap.php';

$kernel = new Kernel($_SERVER['APP_ENV'], (bool) $_SERVER['APP_DEBUG']);
$kernel->boot();

/** @var ContainerInterface $container */
$container = $kernel->getContainer();

/** @var EntityManager $objectManager */
$objectManager = $container->get('doctrine')->getManager();

// remove ResolveTargetEntityListener from returned EntityManager to not resolve SuluPersistenceBundle classes
// this is a workaround for the following phpstan issue: https://github.com/phpstan/phpstan-doctrine/issues/98
$resolveTargetEntityListener = current(array_filter(
    $objectManager->getEventManager()->getListeners('loadClassMetadata'),
    static function ($listener) {
        return $listener instanceof ResolveTargetEntityListener;
    }
));

if ($resolveTargetEntityListener) {
    $objectManager->getEventManager()->removeEventListener([Events::loadClassMetadata], $resolveTargetEntityListener);
}

return $objectManager;

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants