Precise phpdoc of getRootEntities

[VincentLanglet]

No description provided.

[VincentLanglet]

Hi @greg0ire, I recently got a static analysis error because I was expecting getRootEntities to return an array of class string. Am I doing a wrong assumption or should the phpdoc be updated like I did in the PR (which imply the from phpdoc to be updated too).

[greg0ire]

I don't know for sure but it looks to me like you are right. If not, we will revert this PR and add a comment to explain :)

I think this comment needs to be changed as well:

orm/lib/Doctrine/ORM/QueryBuilder.php

Lines 493 to 494 in 8f77012

	* Gets the root entities of the query. This is the entity aliases involved
	* in the construction of the query.

Also, PRs that fix wrong things should target 2.12.x, but target that make things more precise should target 2.13.x

[VincentLanglet]

I don't know for sure but it looks to me like you are right. If not, we will revert this PR and add a comment to explain :)

I think this comment needs to be changed as well:

orm/lib/Doctrine/ORM/QueryBuilder.php

Lines 493 to 494 in 8f77012

	* Gets the root entities of the query. This is the entity aliases involved
	* in the construction of the query.

Also, PRs that fix wrong things should target 2.12.x, but target that make things more precise should target 2.13.x

Fixed :)

[greg0ire]

I just remembered 2 features that might explain why class-string was not used before. Please check what happens when you use them.

[greg0ire]

🤔 maybe this was referring to the deprecated but still existing class alias feature? #8818

I don't know if those are resolved at that point.

[greg0ire]

Likewise, there is a feature that allows to use an interface here: https://www.doctrine-project.org/projects/doctrine-orm/en/2.7/cookbook/resolve-target-entity-listener.html I think this assertion might be wrong because of that.

[VincentLanglet]

I didn't know how to solve both the static analysis error and the cs style error.

First, I added. /** @var class-string $from */ and got a cs error.
I tried again with @psalm-var, seems like it's ok like this.

It might be better to not doing any assertions so far to avoid any errors with the deprecated class-alias feature.

[VincentLanglet]

I just remembered 2 features that might explain why class-string was not used before.

Indeed, but since it's deprecated this will help to stop using them thanks to phpstan reporting static analysis errors then.
So I think the phpdoc update is a good thing. (But maybe not the assert)

[greg0ire]

Indeed, but since it's deprecated this will help to stop using them thanks to phpstan reporting static analysis errors then.
So I think the phpdoc update is a good thing.

I might agree if we were talking about parameter type declarations, but here, this is about return type declarations. We just cannot guarantee that we are going to return a class-string.

I think the way forward should be to check if yes or no, what I described does happen in practice (maybe interfaces and short aliases are resolved earlier?), and if yes, to document why we use string here so that we know when we can use class-string|interface-string.

[VincentLanglet]

I might agree if we were talking about parameter type declarations, but here, this is about return type declarations.

Currently the following code is reported as an error by phpstan

        $rootEntity = current($queryBuilder->getRootEntities());
        if (false === $rootEntity) {
            throw new \RuntimeException('There are not root entities defined in the query.');
        }

        $identifierFields = $queryBuilder
            ->getEntityManager()
            ->getMetadataFactory()
            ->getMetadataFor($rootEntity)
            ->getIdentifierFieldNames();

because getMetadataFor is called on a string and not a class-string like the phpdoc ask for it in persistence 3.
https://github.com/doctrine/persistence/blob/8a864b757a421faf78047d0ed153ded9e3c811eb/src/Persistence/Mapping/ClassMetadataFactory.php#L26

I don't feel right the fact that doctrine/orm is compatible with persistence 3 (https://github.com/doctrine/orm/blob/2.12.x/composer.json#L35) but let the user to handle this phpstan error.

We just cannot guarantee that we are going to return a class-string.

You never can guarantee that this is a class-string. In the following code

interface Foo
{
     /** @param class-string $string */
     public function setString(string $string): void;
     /** @return class-string */
     public function getString(): string;
}

If I can $foo->setString('notAClassString') I will not get a class-string when calling the getter.
It's supposed to be a class-string only if you respect the @param of others methods.

To me it's the same here. getRootEntities is supposed to return a class string IF you respect others params method phpdoc.
And when you can't and don't want to trust phpdoc, there is the option treatPhpdocAsCertain: false in phpstan.

[greg0ire]

Even if we respect the @param of all of Doctrine, we don't have a gurantee that this is going to be a class-string, right? If you use a short namespace alias, for instance, you won't. And I don't think we exclusively take non-deprecated things into account in phpdoc.

I don't feel right the fact that doctrine/orm is compatible with persistence 3 (https://github.com/doctrine/orm/blob/2.12.x/composer.json#L35) but let the user to handle this phpstan error.

Well we can't know if the user is indeed using Persistence 3, can we? I'm afraid you're going to have to use assert(class_exists($rootEntity)) in your code, sorry.

[VincentLanglet]

Even if we respect the @param of all of Doctrine, we don't have a gurantee that this is going to be a class-string, right? If you use a short namespace alias, for instance, you won't.

If you respect the @param class-string, then the @return will be a class string. Because a short namespace is not a class-string, so you're not respecting the phpdoc.

It's the same than this return

orm/lib/Doctrine/ORM/Configuration.php

Line 611 in 2e4a872

* @psalm-return ?class-string

It won't return a class-string, if you don't pass one to the setter

orm/lib/Doctrine/ORM/Configuration.php

Line 600 in 2e4a872

public function addCustomStringFunction($name, $className)

(which is currently asking for string, but even if you change for class-string, it won't force the user to pass a class-string).

[greg0ire]

If you respect the @param class-string,

Which one? The one you are introducing in this PR? This conversation is going in circles, maybe because you think I am talking about the code after this PR, when I really am talking about the code before this PR. Before this PR, there is (rightly IMO) no class-string in the constructor of From, and IMO it should stay that way, because IMO it's supported (if resolution is supposed to happen after). Maybe there should be a deprecation if you don't pass a class-string BTW. Also, I think you found an issue in Configuration.

[VincentLanglet]

If you respect the @param class-string,

Which one? The one you are introducing in this PR?

Yes, I'm talking about the @param class-string I add.
This because this @param is supposed to be respected that I can say that the @return is supposed to be true.

This conversation is going in circles, maybe because you think I am talking about the code after this PR, when I really am talking about the code before this PR. Before this PR, there is (rightly IMO) no class-string in the constructor of From, and IMO it should stay that way, because IMO it's supported (if resolution is supposed to happen after).

I don't try to go in circles, but I really don't understand your point.

You're saying that currently there is no class-string in the constructor so I cannot add a @return class-string. But that's why I also want to update the From phpdoc. It's not supposed to be called with something else than a class-string. Updating the phpdoc is going in the same direction that the deprecation of the class alias. BTW the description of the from param is The class name. ; so it was supposed to be a class name and nothing else.
Also, it's "just" phpdoc so it will not create any runtime errors, it will just guide the user to correctly use those methods without using deprecated feature.

If I use the same argument than you, I could say that getFrom should returns mixed and not string.
Because there is no typehint in the constructor so

new From(1, 1, 1);
new From(new \stdClass(), new \stdClass(), new \stdClass());
new From([], [], []);

are currently working code.

What the @return string is saying is "If you respect the @param from the constructor, you'll end with a string in the getFrom". I'm just changing this sentence with class-string.

Maybe there should be a deprecation if you don't pass a class-string BTW.

Sure

[greg0ire]

I don't try to go in circles, but I really don't understand your point.

I'm not saying you are trying to go in circle, just noticing that's what is happening, and I remain hopeful that we are going to come to an understanding 😉

But that's why I also want to update the From phpdoc. It's not supposed to be called with something else than a class-string.

I think this is where we disagree: calling it with something else than a class-string (so a short alias) is deprecated, but not wrong. It can happen and is supported.

Updating the phpdoc is going in the same direction that the deprecation of the class alias

That's where we disagree and where that discussion should focus. And by we, I'm not sure I mean just "you and I", but from what I've seen, Doctrine as a whole. We preserve documentation for deprecated things, be it in phpdoc or in actual documentation.

I think we might accept this PR if it's really causing a lot of pain for you, but I don't think we would let it act as a precedent for other PRs changing phpdoc to document only non-deprecated behaviors. I'm adding more people as reviewers so that we can get other points of view on this.