-
Notifications
You must be signed in to change notification settings - Fork 112
Carbon Type Descriptors via ReflectionDescriptor #192
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
Carbon Type Descriptors via ReflectionDescriptor #192
Conversation
@ondrejmirtes hello!
Firstly, thank you for this extension and PHPStan in general, it's very useful! 👍
I'm looking to add type descriptors for Carbon date types and wanted to check two things first:
-
Is this something that you consider within the scope of this extension? Or is it more something like a custom type that the end-user should add themselves. I thought it would be useful because the library is widely used
-
I'm having trouble running a fresh
composer installin this project. I tried various things but still could not get it working. Am I missing something?
$ composer install
No lock file found. Updating dependencies instead of installing from lock file. Use composer update over composer install if you do not have a lock file.
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.
Problem 1
- phpstan/phpstan-doctrine is present at version 0.12.x-dev and cannot be modified by Composer
- Conclusion: don't install doctrine/common 3.1.x-dev (conflict analysis result)
- Conclusion: don't install doctrine/common 3.2.x-dev (conflict analysis result)
- Conclusion: don't install doctrine/common 3.0.3 (conflict analysis result)
- Conclusion: don't install doctrine/common 3.1.0 (conflict analysis result)
- Conclusion: don't install doctrine/common 3.1.1 (conflict analysis result)
- Conclusion: don't install doctrine/common 3.1.2 (conflict analysis result)
- Root composer.json requires doctrine/persistence ^1.1 || ^2.0 -> satisfiable by doctrine/persistence[v1.1.0, ..., 1.4.x-dev, 2.0.0, ..., 2.3.x-dev].
- 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.10.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.0.x-dev (conflict analysis result)
- Root composer.json requires doctrine/orm ^2.9.1 -> satisfiable by doctrine/orm[2.9.1, 2.9.x-dev, 2.10.x-dev].```
Hi, can you link to those Carbon Doctrine types you want to describe? Thanks.
As for your installation problem:
- Make sure you use Composer 2
- Try deleting composer.lock and vendor/.
- Try it with --ignore-platform-reqs
Check out the CI scripts, it's not doing anything special...
Hi, can you link to those Carbon Doctrine types you want to describe? Thanks.
As for your installation problem:
- Make sure you use Composer 2
- Try deleting composer.lock and vendor/.
- Try it with --ignore-platform-reqs
Check out the CI scripts, it's not doing anything special...
Hey, thanks for the fast reply.
I managed to run the installation with --ignore-platform-reqs 👍 .
Regarding the types, here they are:
They both use the CarbonTypeConverter to do the conversions from/to database.
The easiest way forward would be for the library to add proper typehints - the methods convertToPHPValue and convertToDatabaseValue don't have the correct return typehints. And after that registering the ReflectionDescriptor would be sufficient. (https://github.com/phpstan/phpstan-doctrine#reflectiondescriptor).
Alternatively an implementation like https://github.com/phpstan/phpstan-doctrine/blob/master/src/Type/Doctrine/Descriptors/Ramsey/UuidTypeDescriptor.php would also work.
Thanks for the details.
Looks like not even the Doctrine date types have those type hints 🤷♂️ .
Also it might be difficult due to the way the code is structured as the converter is used for both Carbon and CarbonImmutable. The return type of convertToPHPValue will be the class instance returned by getCarbonClassName, which gets overridden in here. Is it possible to convey this properly via type hints?
The above is mostly why I thought a custom descriptor like DateTimeImmutableType would be the easiest, unless I'm missing something.
Oh, in that case we need a custom DoctrineTypeDescriptor implementation here :) So please submit the PR, thanks :)
Oh, in that case we need a custom DoctrineTypeDescriptor implementation here :) So please submit the PR, thanks :)
Added now, please run the workflow as it's my first time contributing here :)
bca5b04 to
24f7283
Compare
3f98b48 to
1e3c1ea
Compare
@ondrejmirtes hey 👋 , this is updated and ready for re-review now
thank you! 💯
Uh oh!
There was an error while loading. Please reload this page.
Description
This PR adds type descriptors for Carbon date types Carbon and CarbonImmutable using the ReflectionDescriptor.
Made possible after: briannesbitt/Carbon#2344.
Features / Changes
extension.neonMyBrokenEntity.php