Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged

Conversation

@AlexandruGG
Copy link

@AlexandruGG AlexandruGG commented May 27, 2021
edited
Loading

Description

This PR adds type descriptors for Carbon date types Carbon and CarbonImmutable using the ReflectionDescriptor.

Made possible after: briannesbitt/Carbon#2344.

Features / Changes

  • Registers descriptors in extension.neon
  • Adds test cases in MyBrokenEntity.php

Copy link
Author

@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:

  1. 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

  2. I'm having trouble running a fresh composer install in 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].```

Copy link
Member

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...

Copy link
Author

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.

Copy link
Member

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.

Copy link
Author

AlexandruGG commented May 28, 2021
edited
Loading

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.

Copy link
Member

Oh, in that case we need a custom DoctrineTypeDescriptor implementation here :) So please submit the PR, thanks :)

@AlexandruGG AlexandruGG changed the title (削除) [WIP] Carbon Type Descriptors (削除ここまで) (追記) Carbon Type Descriptors (追記ここまで) May 28, 2021
@AlexandruGG AlexandruGG marked this pull request as ready for review May 28, 2021 13:39
Copy link
Author

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 :)

@AlexandruGG AlexandruGG force-pushed the feature/carbon-type-descriptors branch from 3f98b48 to 1e3c1ea Compare June 9, 2021 17:54
@AlexandruGG AlexandruGG changed the title (削除) Carbon Type Descriptors (削除ここまで) (追記) Carbon Type Descriptors via ReflectionDescriptor (追記ここまで) Jun 9, 2021
@AlexandruGG AlexandruGG marked this pull request as ready for review June 9, 2021 17:57
Copy link
Author

@ondrejmirtes hey 👋 , this is updated and ready for re-review now

@ondrejmirtes ondrejmirtes merged commit c35261f into phpstan:master Jun 10, 2021
Copy link
Author

thank you! 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /