Avoid conditional classes?

[ro0NL]

After bumping doctrine, our psalm analyse broke for all service entity repositories due #1599

1611 does not solve it AFAIK: https://psalm.dev/r/75d58de9af

in practice this means all our repositories got wiped from the baseline without any warning, and future issues arent spotted anymore

i suggest to remove @internal on LazyServiceEntityRepository so we can easily mass replace existing extends ServiceEntityRepository, rather than waiting for to be renamed ServiceEntityRepository when PHP 8.1 / Symfony 6.2 becomes required and have more maintenance, again.

Please KISS :)

[stof]

We don't want people to use the LazyServiceEntityRepository class directly, as that would mean we cannot actually remove it anymore.

To me, this should rather be fixed in Psalm to be able to analyze this code (this is something that should be doable given that phpstan is able to analyse it)

[ro0NL]

yes, but psalm wasnt fixed:

https://github.com/doctrine/DoctrineBundle/blob/96ffb1a9fc9ef2b6385710456c5bf7e7e8b05da0/psalm.xml.dist#L72-L77

We don't want people to use the LazyServiceEntityRepository class directly, as that would mean we cannot actually remove it anymore.

ServiceEntityRepository and LazyServiceEntityRepository are separate features IMHO, and would avoid all this :)

[ro0NL]

i'll wait to see how this plays out

[stof]

LazyServiceEntityRepository is meant to fix the compatibility of ServiceEntityRepository with cases where you mark your repository service as being a lazy service in a Symfony 6.2+ project (Symfony 6.2 changed the way lazy services are implemented), to actually keep things lazy (as the parent constructor performs some logic). This is really meant to be an implementation detail of ServiceEntityRepository for newer versions.

[ro0NL]

perhaps the trait should have been mocked instead,

personally im in the "bump your composer constraints and release a new version" camp, then it doesnt matter ServiceEntityRepository is changed or deprecated

[stof]

Well, the goal was to avoid dropping support for the 5.4 LTS version of Symfony (which would force us to keep maintaining an older version of the bundle until the EOM of that LTS version).

Once again, the fact that Psalm breaks with this conditional class definition is an issue in Psalm (and phpstan being able to support it is a proof that static analysis tools are able to support it).

[ro0NL]

yes, the problem is psalm v phpstan, dividing us everywhere :roll_eyes:

dont they have a shared spec or so

[ro0NL]

my main point here is we could have supported both tools, but we deliberately didnt

[stof]

My main point is that you should request psalm to fix their issue instead of asking us to avoid a feature that we need to use to avoid having to duplicate the maintenance work (we cannot drop support entirely for the Symfony LTS version for a bundle like DoctrineBundle).

[ro0NL]

i just wanted to point out that the current status quo doesnt work for psalm users

before requesting an issue, let's see how it plays out, because if im the only one, it's a non-issue ;)

[ostrolucky]

Not sure why are you directly referencing LazyServiceEntityRepository in your code. I think people don't have the issue because they don't do that.

Also, this shouldn't be such a big fuss if you can suppress the issue like we do here.

[ro0NL]

no, our repos are like:

/**
 * @extends ServiceEntityRepository<Some>
 */
class SomeRepository extends ServiceEntityRepository

before: https://psalm.dev/r/a1eabccf52 after: https://psalm.dev/r/dcecd42e5d

[ostrolucky]

I tried reproducing in symfony-demo and it doesn't have the issue, even though using the same thing. My bet is you have something wrong in your psalm config. Do you have eg. this?

<ignoreFiles>
    <directory name="vendor" />
</ignoreFiles>

[ro0NL]

i can reproduce it in demo too:

<?xml version="1.0" encoding="UTF-8"?>
<psalm
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xmlns="https://getpsalm.org/schema/config"
    xsi:schemaLocation="https://getpsalm.org/schema/config"
    findUnusedCode="true"
    findUnusedVariablesAndParams="true"
    findUnusedBaselineEntry="true"
    findUnusedPsalmSuppress="true"
>
    <projectFiles>
        <directory name="src"/>
        <ignoreFiles>
            <directory name="vendor"/>
        </ignoreFiles>
    </projectFiles>
</psalm>

I have no name!@e454d83e0566:/app$ psalm -v
Psalm 5.12.0@f90118cdeacd0088e7215e64c0c99ceca819e176

I have no name!@e454d83e0566:/app$ git status --porcelain 
?? .idea/
?? psalm.xml

I have no name!@e454d83e0566:/app$ psalm --no-cache --output-format=github src/Repository/PostRepository.php 
Target PHP version: 8.1 (inferred from composer.json) (unsupported extensions: pdo_sqlite).
Scanning files...
Analyzing files...

░

I have no name!@e454d83e0566:/app$ git diff
diff --git a/src/Repository/PostRepository.php b/src/Repository/PostRepository.php
index cb49cb8..7b86e34 100644
--- a/src/Repository/PostRepository.php
+++ b/src/Repository/PostRepository.php
@@ -36,10 +36,10 @@ class PostRepository extends ServiceEntityRepository
 {
     public function __construct(ManagerRegistry $registry)
     {
-        parent::__construct($registry, Post::class);
+        parent::__construct($registry, BLA::class);
     }

...skipping 1 line
+    public function findLatest(int $page = 1, Tag $tag = null): WRONG
     {
         $qb = $this->createQueryBuilder('p')
             ->addSelect('a', 't')

I have no name!@e454d83e0566:/app$ psalm --no-cache --output-format=github src/Repository/PostRepository.php 
Target PHP version: 8.1 (inferred from composer.json) (unsupported extensions: pdo_sqlite).
Scanning files...
Analyzing files...

░

If at this point we remove the conditional in ServiceEntityRepository, we get:

I have no name!@e454d83e0566:/app$ psalm --no-cache src/Repository/PostRepository.php 
Target PHP version: 8.1 (inferred from composer.json) (unsupported extensions: pdo_sqlite).
Scanning files...
Analyzing files...

E

To whom it may concern: Psalm cannot detect unused classes, methods and properties
when analyzing individual files and folders. Run on the full project to enable
complete unused code detection.

ERROR: PropertyNotSetInConstructor - src/Repository/PostRepository.php:35:7 - Property App\Repository\PostRepository::$lazyObjectState is not defined in constructor of App\Repository\PostRepository or in any private or final methods called in the constructor (see https://psalm.dev/074)
class PostRepository extends ServiceEntityRepository

ERROR: PropertyNotSetInConstructor - src/Repository/PostRepository.php:35:7 - Property App\Repository\PostRepository::$_entityName is not defined in constructor of App\Repository\PostRepository or in any private or final methods called in the constructor (see https://psalm.dev/074)
class PostRepository extends ServiceEntityRepository

ERROR: PropertyNotSetInConstructor - src/Repository/PostRepository.php:35:7 - Property App\Repository\PostRepository::$_em is not defined in constructor of App\Repository\PostRepository or in any private or final methods called in the constructor (see https://psalm.dev/074)
class PostRepository extends ServiceEntityRepository

ERROR: PropertyNotSetInConstructor - src/Repository/PostRepository.php:35:7 - Property App\Repository\PostRepository::$_class is not defined in constructor of App\Repository\PostRepository or in any private or final methods called in the constructor (see https://psalm.dev/074)
class PostRepository extends ServiceEntityRepository

ERROR: InternalMethod - src/Repository/PostRepository.php:39:9 - The method Doctrine\Bundle\DoctrineBundle\Repository\LazyServiceEntityRepository::__construct is internal to Doctrine and Doctrine but called from App\Repository\PostRepository::__construct (see https://psalm.dev/175)
        parent::__construct($registry, BLA::class);

ERROR: UndefinedClass - src/Repository/PostRepository.php:39:40 - Class, interface or enum named App\Repository\BLA does not exist (see https://psalm.dev/019)
        parent::__construct($registry, BLA::class);

ERROR: UndefinedClass - src/Repository/PostRepository.php:42:65 - Class, interface or enum named App\Repository\WRONG does not exist (see https://psalm.dev/019)
    public function findLatest(int $page = 1, Tag $tag = null): WRONG

ERROR: InvalidReturnType - src/Repository/PostRepository.php:42:65 - The declared return type 'App\Repository\WRONG' for App\Repository\PostRepository::findLatest is incorrect, got 'App\Pagination\Paginator' (see https://psalm.dev/011)
    public function findLatest(int $page = 1, Tag $tag = null): WRONG

ERROR: TooManyArguments - src/Repository/PostRepository.php:45:15 - Too many arguments for method Doctrine\ORM\QueryBuilder::addselect - saw 2 (see https://psalm.dev/026)
            ->addSelect('a', 't')

ERROR: InvalidReturnStatement - src/Repository/PostRepository.php:58:16 - The inferred type 'App\Pagination\Paginator' does not match the declared return type 'App\Repository\WRONG' for App\Repository\PostRepository::findLatest (see https://psalm.dev/128)
        return (new Paginator($qb))->paginate($page);

ERROR: ImplicitToStringCast - src/Repository/PostRepository.php:95:16 - The declared return type for App\Repository\PostRepository::extractSearchTerms expects 'array<array-key, string>', 'array<array-key, Symfony\Component\String\AbstractUnicodeString&Symfony\Component\String\UnicodeString>' provided with a __toString method (see https://psalm.dev/060)
     * @return string[]

------------------------------
11 errors found
------------------------------

ref https://github.com/doctrine/DoctrineBundle/pull/1611#issuecomment-1594242005

[ostrolucky]

Ah so your issue isn't false positive but false negative

[ro0NL]

i think so. my issue is repositories arent analyzed anymore, and it affects the baseline :}

im gonna leave it at this.

[ostrolucky]

Well as was mentioned you should create issue on psalm repo. I don't see any links there so far.