Skip to content

Discover repository class from #[Entity] attribute on PHP 8 #238

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
wants to merge 1 commit into from

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Dec 16, 2021

By doing it like this, we don't need to instantiate the object manager at all which makes PHPStan run faster and more reliable. Very often I had to manually clear my Symfony cache and re-run PHPStan.

Unfortunately, BetterReflection doesn't support attributes yet so therefore we need to use native ReflectionClass.

See phpstan/phpstan#5863 (reply in thread)

@ruudk ruudk force-pushed the read-from-attribute branch 5 times, most recently from 87e4bf7 to d524d5d Compare December 16, 2021 14:45
@ruudk
Copy link
Contributor Author

ruudk commented Dec 16, 2021

Hmm, I don't understand why on PHP 8.0 it fails.

@ondrejmirtes
Copy link
Member

Please try the nice way via ClassReflection::getNativeReflection() and require phpstan/phpstan: "^1.3" here.

@ruudk ruudk force-pushed the read-from-attribute branch from d524d5d to 2eae4a5 Compare December 16, 2021 15:39
@ondrejmirtes
Copy link
Member

The fact that PHPStan reports an error like that is weird and I should look into it, but it might still work correctly. Hard to tell without a test.

@ruudk
Copy link
Contributor Author

ruudk commented Dec 16, 2021

For some reason I cannot install the dependencies for this project anymore:


  Problem 1
    - Root composer.json requires doctrine/mongodb-odm ^1.3 || ^2.1 -> satisfiable by doctrine/mongodb-odm[1.3.0-RC1, ..., 1.3.x-dev, 2.1.0, ..., 2.3.x-dev].
    - doctrine/orm[2.9.1, ..., 2.11.x-dev] require doctrine/common ^3.0.3 -> satisfiable by doctrine/common[3.0.3, ..., 3.2.x-dev].
    - Conclusion: don't install doctrine/common 3.2.0 (conflict analysis result)
    - Root composer.json requires doctrine/orm ^2.9.1 -> satisfiable by doctrine/orm[2.9.1, ..., 2.11.x-dev].

Weird that it just works on CI.

@ruudk ruudk force-pushed the read-from-attribute branch from 2eae4a5 to d037bd6 Compare December 16, 2021 15:43
@ondrejmirtes
Copy link
Member

Yeah, you need --ignore-platform-reqs if you don't have the mongodb PHP extension installed locally.

@ruudk ruudk force-pushed the read-from-attribute branch from d037bd6 to 125fecf Compare December 16, 2021 16:24
@ruudk
Copy link
Contributor Author

ruudk commented Dec 16, 2021

That works, weird that the error doesn't mention ext-mongodb

I don't know how to solve the error on PHP 8. It seems that PHPStan doesn't understand ReflectionAttribute.

@ondrejmirtes
Copy link
Member

I really don't know, it works in the playground: https://phpstan.org/r/fa2489cc-dfd7-4a61-8865-c29e83a8a2a6

@ruudk
Copy link
Contributor Author

ruudk commented Dec 16, 2021

\PHPStan\dumpType($this->reflectionProvider->getClass($className));
\PHPStan\dumpType($this->reflectionProvider->getClass($className)->getNativeReflection());
\PHPStan\dumpType($this->reflectionProvider->getClass($className)->getNativeReflection()->getAttributes());
  101    Dumped type: PHPStan\Reflection\ClassReflection
  102    Dumped type: ReflectionClass
  103    Call to an undefined method ReflectionClass::getAttributes().

🤷

@ondrejmirtes
Copy link
Member

I know! It's caused by config.platform.php here:

"config": {
"platform": {
"php": "7.3.24",
"ext-mongo": "1.6.16"
},
"sort-packages": true

@ruudk ruudk force-pushed the read-from-attribute branch from 125fecf to 719b5cc Compare December 16, 2021 17:03
@ruudk
Copy link
Contributor Author

ruudk commented Dec 16, 2021

I removed the php platform and now phpunit fails. Shouldn't PHPunit be installed differently on PHP < 7.3?

I fixed the name of the mongodb extension, as it was wrong.

@ruudk ruudk force-pushed the read-from-attribute branch 4 times, most recently from e0b1153 to c9f9444 Compare December 16, 2021 19:08
@ruudk
Copy link
Contributor Author

ruudk commented Dec 16, 2021

Ok, 2 problems left:

  1. How to prevent Call to an undefined method ReflectionClass::getAttributes(). on < PHP 8? My previous method_exists complains on PHP 8 that it's always true

  2. PHP 7.1 becomes a problem as it requires https://packagist.org/packages/doctrine/mongodb-odm#1.3.0 that needs a deprecated mongo extension. Should we just install with --ignore-platform-req=php on PHP 7.1? It's a bit like it was, with the fake 7.3 in composer config platform.

@ruudk ruudk force-pushed the read-from-attribute branch 3 times, most recently from 80d1c57 to 1eee2a6 Compare December 18, 2021 11:33
@ruudk ruudk mentioned this pull request Dec 18, 2021
@ruudk ruudk force-pushed the read-from-attribute branch from 1eee2a6 to 670690e Compare December 18, 2021 12:35
@ruudk
Copy link
Contributor Author

ruudk commented Dec 18, 2021

Oke, so the only problem left seems to be a bug in PHPStan, am I right?

https://phpstan.org/r/a34ea8cd-2986-4bbf-82e2-a1041cb4a6de

If so, I can report it on phpstan/phpstan

@ruudk ruudk force-pushed the read-from-attribute branch 2 times, most recently from f8e3f88 to 2179d23 Compare December 30, 2021 08:22
@ruudk
Copy link
Contributor Author

ruudk commented Dec 30, 2021

@ondrejmirtes I hoped that PHPStan 1.3 would solve the problem with attributes, as the release notes mention this:

This brings plenty of bugfixes, for example PHP 8.0 Attributes can now be read in static reflection context

but I still cannot solve the problem.

Any idea how to proceed?

@ondrejmirtes
Copy link
Member

What PHPStan 1.3 fixed was an internal PHP error when accessing ClassReflection::getNativeReflection()->getAttributes() in some cases.

Are you trying to solve the problem on PHP 7.x from this example? https://phpstan.org/r/a34ea8cd-2986-4bbf-82e2-a1041cb4a6de This is usually solved by having different PHPStan configs for different PHP versions (when a project is analysed on different PHP cersions).

One notable example is PHPStan itself: https://github.com/phpstan/phpstan-src/blob/master/build/ignore-by-php-version.neon.php

This file is included in phpstan.neon: https://github.com/phpstan/phpstan-src/blob/b614f4af90b26e24d653b9d11a27679ee9b459ad/build/phpstan.neon#L10

By doing it like this, we don't need to instantiate the object manager at all.
@ruudk ruudk force-pushed the read-from-attribute branch from 2179d23 to 22f9661 Compare January 2, 2022 18:20
@ruudk
Copy link
Contributor Author

ruudk commented Jan 2, 2022

@ondrejmirtes It worked 🎉 Ready for review.

@ruudk
Copy link
Contributor Author

ruudk commented Jan 3, 2022

While writing some tests, I notice that $this->reflectionProvider->hasClass($className) returns false, and therefore it skips my code. Isn't it possible to reflect unknown classes?

@ondrejmirtes
Copy link
Member

Looks like I'm gonna solve this in a general way for all the rules :) #253

@ruudk ruudk deleted the read-from-attribute branch January 21, 2022 12:54
@ruudk
Copy link
Contributor Author

ruudk commented Jan 21, 2022

🥳🥳🥳🥳🥳 thank you

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.

2 participants