Skip to content
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

Use ::class to get the fully qualified name of the class #118

Closed
wants to merge 1 commit into from

Conversation

IanDelMar
Copy link
Sponsor Contributor

From PHPStan 1.8.6 return '\WP_Theme'; no longer works. PHPStan skips the dynamic return type when analyzing an instance of WP_Theme.

@IanDelMar
Copy link
Sponsor Contributor Author

The 3 failures from the checks are not related to this PR.

@herndlm
Copy link
Contributor

herndlm commented Oct 6, 2022

The change makes sense I guess, but I wonder why this behaviour changed in phpstan and if it is a BC break. You don't know what in phpstan broke it do you?

@szepeviktor
Copy link
Owner

szepeviktor commented Oct 6, 2022

In string form there must not be a leading backslash.

@szepeviktor
Copy link
Owner

szepeviktor commented Oct 6, 2022

Sorry guys, this is too hard for me.
Please see phpstan/phpstan#8118 - this is about failing tests, not this PR

@szepeviktor
Copy link
Owner

This is how FQCN should be written #119

FQCN only for docblocks and exception.

I'm hesitant with extends and implements. I do not see a reason except name collision.

@szepeviktor
Copy link
Owner

You have to know that this repo was not meant to be for PHP code.
Only for stubs+some shell code.

@szepeviktor
Copy link
Owner

phpstan/phpstan-src#1758

How to go on?
PHPStan v1.8.7 outputs class names without a backslash.
Requiring this latest version is not the nicest thing in the world.

@herndlm
Copy link
Contributor

herndlm commented Oct 6, 2022

I think the easiest thing indeed would be to just bump the minimal required phpstan version. People can just update it IMO

@szepeviktor
Copy link
Owner

the easiest thing

I let other people choose that.

@IanDelMar
Copy link
Sponsor Contributor Author

@herndlm I don't know what causes the issue or whether one of the sources mentioned by @szepeviktor is the cause.

@szepeviktor The changes proposed in #119 do not resolve the issue addressed by this PR. Using #119 makes PHPStan "ignore" the dynamic method return type in v1.8.5, v1.8.6 and v1.8.7.

@IanDelMar
Copy link
Sponsor Contributor Author

Using

public function getClass(): string
{
        return \WP_Theme::class;
}

also matches the dynamic return type example from the docs:

public function getClass(): string
{
	return \Doctrine\ORM\EntityManager::class;
}

public function isMethodSupported(MethodReflection $methodReflection): bool
{
	return $methodReflection->getName() === 'merge';
}

public function getTypeFromMethodCall(
	MethodReflection $methodReflection,
	MethodCall $methodCall,
	Scope $scope
): Type
{ ... }

@szepeviktor
Copy link
Owner

It is hard to be a repo owner!

@IanDelMar
Copy link
Sponsor Contributor Author

@herndlm I don't know what causes the issue or whether one of the sources mentioned by @szepeviktor is the cause.

@szepeviktor The changes proposed in #119 do not resolve the issue addressed by this PR. Using #119 makes PHPStan "ignore" the dynamic method return type in v1.8.5, v1.8.6 and v1.8.7.

It seems, it was my fault and #119 solves the issue.

@szepeviktor
Copy link
Owner

szepeviktor commented Oct 6, 2022

@johnbillion Could you jump in a resolve #118 (comment) wich a single click? 🙃

@johnbillion
Copy link
Sponsor Contributor

How does the WP_Theme class exist during static analysis? Surely the change should be to just remove the leading slash from the string.

@herndlm
Copy link
Contributor

herndlm commented Oct 6, 2022

How does the WP_Theme class exist during static analysis? Surely the change should be to just remove the leading slash from the string.

👍 + phpstan version bump, no? Otherwise people come with old phpstan versions and complain the slash is missing I guess. If I understood the problem..

@herndlm
Copy link
Contributor

herndlm commented Oct 6, 2022

But sorry, I was just assuming that the fixed version doesn't play nice with an old phpstan. If it does, perfect :)

Btw @szepeviktor does it make sense to test with lowest and highest deps in CI maybe to verify things like that?

@szepeviktor
Copy link
Owner

szepeviktor commented Oct 6, 2022

does it make sense to test with lowest and highest deps in CI

@herndlm I know how a full-blown software project looks like :) This is definitely not like that.

@szepeviktor
Copy link
Owner

szepeviktor commented Oct 6, 2022

Does anyone know how to get phpstan version in DynamicMethodReturnTypeExtension?

@IanDelMar
Copy link
Sponsor Contributor Author

Does anyone know how to get phpstan version in DynamicMethodReturnTypeExtension?

What about \PHPStan\Internal\ComposerHelper::getPhpStanVersion()?

@szepeviktor
Copy link
Owner

What about \PHPStan\Internal\ComposerHelper::getPhpStanVersion()?

Thank you!
I've tried it here


and it errored out.

Could you try it also?

@herndlm
Copy link
Contributor

herndlm commented Oct 6, 2022

That method was added very recently btw, which means most likely that you'd have to bump the phpstan version ;)

@herndlm
Copy link
Contributor

herndlm commented Oct 6, 2022

You could require composer 2 and use the native PackageVersions thingy or you could use that package that does the same thing. But another dep might mean another potential conflict and increased complexity I guess :/

Also: rather not use somehing from phpstan internal, this will surely break at some point. Maybe it's namespace is even prefixed or smth like that

@IanDelMar
Copy link
Sponsor Contributor Author

That method was added very recently btw, which means most likely that you'd have to bump the phpstan version ;)

That's probably why the following worked for me (v1.8.2)

use PHPStan\Internal\ComposerHelper;

$version = ComposerHelper::getPhpStanVersion();

if (version_compare($version, '1.8.5', '>') ) {
   ...
} else {
   ...
}

rather not use somehing from phpstan internal, this will surely break at some point.

Good point.

@szepeviktor
Copy link
Owner

szepeviktor commented Oct 6, 2022

@IanDelMar Could you please add "phpstan/phpstan": "< 1.8.6" to your Composer configuration?
I need time to digest all these.

@IanDelMar
Copy link
Sponsor Contributor Author

@IanDelMar Could you please add "phpstan/phpstan": "< 1.8.6" to your Composer configuration? I need time to digest all these.

Sure. Take your time.

@szepeviktor
Copy link
Owner

@IanDelMar Done in master :)

@szepeviktor szepeviktor closed this Nov 9, 2022
@szepeviktor
Copy link
Owner

Thank you.

@IanDelMar IanDelMar deleted the patch-1 branch November 9, 2022 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants