Fix templated phpdoc return type

[mvorisek]

When split between @param and @psalm-param, only the @psalm-param is used by psalm/phpstan, ie class-string<T> is used instead of (too wide) class|class-string<T>.

In the 2nd commit, I fixed all other simillar problems in the whole repo. I used @.*(string.*\|class-s|class-string.*\|.*string) regex to find the lines to check/fix.

[mvorisek]

I regenerated the psalm baseline locally, but the CI is still failing. Please regenerate it before merge.

[derrabus]

I don't believe that this change is correct. Can you elaborate what issue you're trying to fix here?

[mvorisek]

phpstan cannot otherwise infer the resulting type for EntityManagerInterface::getClassMetadata() method, see the no longer needed baseline ignore [1]

@derrabus why do you think this change is not correct?

see other methods phpdocs in EntityManagerInterface class, they are all, except this one, in the form I fixed this one to

[1] and in my usecase, I needed to add Unable to resolve the template type T in call to method static method Doctrine\\Common\\Util\\ClassUtils::getClass\(\) phpstan ignore /wo this PR

[derrabus]

see the no longer needed baseline ignore
[…]
why do you think this change is not correct?

See all the new Psalm errors that pop up after your change. 🙂

[mvorisek]

see the no longer needed baseline ignore
[…]
why do you think this change is not correct?

See all the new Psalm errors that pop up after your change. 🙂

@derrabus looking into the psalm errors -> code: https://github.com/doctrine/orm/blob/2.15.4/lib/Doctrine/ORM/Cache.php#L42 it seems the are many places that accept a class name but are annotated as string instead of class-string.

I am not paid to maintain this project so I would be happy if this PR can be accepted and the psalm baseline regenerated (I tried myself on Windows, but the updated baseline did not fix all errors).

[derrabus]

I am not paid to maintain this project

Neither am I. 🤷🏻‍♂️

I would be happy if this PR can be accepted

I cannot merge a PR with a red CI and I won't fix other people's PRs in my freetime.

[SenseException]

@mvorisek What kind of problems do you have that you want to fix with this PR? Your last comment doesn't sound like that this is relevant to your daily work with ORM but more of an aid. We appreciate the effort people put into PRs as users (hopefully) appreciate the time we spent into the Doctrine project repositories on a regular basis.

We would like to know how this should be handled, because, as @derrabus already mentioned, this would rather be a hinderance to our also unpaid freetime if the failed build doesn't get fixed. I'd like to ask you to go the second half of what you've started.

[greg0ire]

All 4 of your commit messages are unhelpful, or sometimes even plain wrong, it seems. As I probably have already told you in the past, you should read the contribution guidelines to figure out what to write, they're fairly extensive on the topic.

As for the the CI build, if you want to contribute to Doctrine, you should setup a proper development environment.

[greg0ire]

EntityManager::getClassMetadata() also supports aliased names and needs to do so until Persistence 2 support is removed:

orm/lib/Doctrine/ORM/EntityManager.php

Lines 305 to 316 in f7e4b61

	* The class name must be the fully-qualified class name without a leading backslash
	* (as it is returned by get_class($obj)) or an aliased class name.
	*
	* Examples:
	* MyProject\Domain\User
	* sales:PriceRequest
	*
	* Internal note: Performance-sensitive method.
	*
	* {@inheritDoc}
	*/
	public function getClassMetadata($className)

Should the same be expected of any EntityManagerInterface implementation? If yes I'd say you're not so much fixing stuff as you are breaking it.

Fix templated phpdoc return type

Did you mean to write "parameter type" here?

[mvorisek]

@greg0ire see #10849 (comment) for repro, I meant "return type", as the "return type" is currently not correctly narrowed as the "parameter type" does allow any string

[greg0ire]

Related: #10853

[VincentLanglet]

* @param string $className
     * @psalm-param class-string<T> $className

Might be a good way to deprecate string param for static analysis user
and solve phpstan/phpstan#5175

[mvorisek]

What kind of problems do you have that you want to fix with this PR?

here is a phpstan repro: https://phpstan.org/r/b2cbe847-a748-4094-bc49-44bd3e200e12 explaining the need

notice the dumped type change when l6 is changed to @psalm-param class-string<T> $className

[derrabus]

This function accepts strings that are not FQCN, so changing the parameter type to class-string would be wrong. But if we a class-string<SomeClass> is passed, the return type to expect is ClassMetadata<SomeClass>.

Can we solve this with a conditional return type?

[VincentLanglet]

This function accepts strings that are not FQCN, so changing the parameter type to class-string would be wrong. But if we a class-string<SomeClass> is passed, the return type to expect is ClassMetadata<SomeClass>.

Can we solve this with a conditional return type?

There was an issue about this in phpstan
phpstan/phpstan#5175

[mvorisek]

Closing as it seems this change is not wanted, at least for ORM 2.x.

[VincentLanglet]

Closing as it seems this change is not wanted, at least for ORM 2.x.

I think the string support was deprecated so maybe the issue does not exist in 3.x anymore (or the phpdoc could be updated).

Need to check