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

API Strongly type everything #460

Conversation

maxime-rainville
Copy link
Contributor

Trying to strongly type everything.

Parent issue

@GuySartorelli
Copy link
Member

In some cases you've removed the params and returns from phpdoc blocks and in other cases you e retained it. We should be consistent about that (and probably have at least a semiformal policy about it to make it easier on ourselves when it comes time to strongly type other modules)

Copy link
Contributor Author

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to strongly type everything.

There's a few places where array definition where clearly meant to be callabale so I swapped those.

When the PHPDoc was just repeating the explicit types, I removed it.

src/Config/Configuration.php Outdated Show resolved Hide resolved
src/Config/Configuration.php Outdated Show resolved Hide resolved
src/Config/Configuration.php Outdated Show resolved Hide resolved
src/Config/Configuration.php Outdated Show resolved Hide resolved
src/PersistedQuery/PersistedQueryMappingProvider.php Outdated Show resolved Hide resolved
src/Auth/Handler.php Outdated Show resolved Hide resolved
/**
* Represents middleware for evaluating a graphql query
*/
interface Middleware
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find this being used anywhere.


namespace SilverStripe\GraphQL\Middleware;

trait MiddlewareConsumer
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't find this being used anywhere.

src/QueryHandler/QueryHandler.php Show resolved Hide resolved
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't gone through the whole PR 'cause it's pretty big - there's a lot of inconsistency about keeping or removing @param and @returns declarations in PHPDocs even when they match the typehints exactly.

There are also a couple of other small changes here (namely some missing typehints) - but again I haven't checked the whole PR much less had a hunt around outside the diff to see if there were others missed out.

src/Config/Configuration.php Outdated Show resolved Hide resolved
src/Config/ModelConfiguration.php Outdated Show resolved Hide resolved
src/Auth/MemberAuthenticator.php Outdated Show resolved Hide resolved
src/Controller.php Show resolved Hide resolved
src/Controller.php Show resolved Hide resolved
src/Middleware/QueryCachingMiddleware.php Outdated Show resolved Hide resolved
src/Middleware/QueryCachingMiddleware.php Outdated Show resolved Hide resolved
src/Middleware/QueryCachingMiddleware.php Outdated Show resolved Hide resolved
src/Middleware/QueryMiddleware.php Outdated Show resolved Hide resolved
src/PersistedQuery/FileProvider.php Outdated Show resolved Hide resolved
@maxime-rainville maxime-rainville force-pushed the pulls/master/strongly-type-everythynig branch from 5d4899e to 5d0b91a Compare April 27, 2022 01:00
@maxime-rainville
Copy link
Contributor Author

Just pushed a few extra changes to explicitly require all the libraries directly reference in the codebase and to make Versioned optional.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've still got a lot of PHPDocs here that are duplicating (and in at least one case in conflict with) the strong param and/or return types. I'm not going to go through and ping them all in the review because you'll still have to go through and find them all in your IDE to remove them, so it's just duplication of effort.
Consider this a blanket "Please remove the PHPDocs param and return declarations that are the same as the strong typehints, and validate any that are in conflict.

src/Config/Configuration.php Outdated Show resolved Hide resolved
src/PersistedQuery/FileProvider.php Outdated Show resolved Hide resolved
src/PersistedQuery/FileProvider.php Outdated Show resolved Hide resolved
src/PersistedQuery/FileProvider.php Outdated Show resolved Hide resolved
src/PersistedQuery/HTTPProvider.php Outdated Show resolved Hide resolved
src/QueryHandler/QueryHandler.php Outdated Show resolved Hide resolved
src/QueryHandler/QueryHandler.php Show resolved Hide resolved
src/Schema/Logger.php Show resolved Hide resolved
src/Schema/Logger.php Show resolved Hide resolved
src/Auth/BasicAuthAuthenticator.php Outdated Show resolved Hide resolved
src/Schema/DataObject/Plugin/Paginator.php Show resolved Hide resolved
src/Schema/Plugin/PaginationPlugin.php Outdated Show resolved Hide resolved
src/Schema/Plugin/SortPlugin.php Outdated Show resolved Hide resolved
src/Schema/Type/Type.php Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@maxime-rainville maxime-rainville force-pushed the pulls/master/strongly-type-everythynig branch 2 times, most recently from 6f40f65 to 45b5763 Compare May 2, 2022 21:58
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still a lot of types mentioned in PHPDocs. I've gone through and tagged them this time.
Note that in some cases it's possible that removing just the bits I've pinged will result in an empty PHPDoc - in those cases the whole PHPDoc should be thrown out.

I did find some missing types (see comments)

There's also a question in here about typing callables as array.

.travis.yml Outdated Show resolved Hide resolved
src/PersistedQuery/FileProvider.php Outdated Show resolved Hide resolved
src/PersistedQuery/FileProvider.php Outdated Show resolved Hide resolved
src/Schema/DataObject/Plugin/Paginator.php Show resolved Hide resolved
Comment on lines 14 to 15
* @param int $timeout
* @return null|string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param int $timeout
* @return null|string

Also remove the url param phpdoc.

src/Schema/Type/Enum.php Outdated Show resolved Hide resolved
src/Schema/Type/Enum.php Outdated Show resolved Hide resolved
src/Schema/Type/Type.php Outdated Show resolved Hide resolved
src/Schema/Type/TypeReference.php Show resolved Hide resolved
src/Schema/Type/TypeReference.php Outdated Show resolved Hide resolved
@maxime-rainville maxime-rainville force-pushed the pulls/master/strongly-type-everythynig branch from a6f1766 to a99055b Compare May 3, 2022 03:32
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. A few phpdocs missed but we've agreed over slack that some redundant phpdocs here or there isn't the end of the world, and shouldn't block this PR being merged.

Merge on green.

@GuySartorelli
Copy link
Member

Behat failure is on master already, so not related to this PR.

@GuySartorelli GuySartorelli merged commit 69333c6 into silverstripe:master May 3, 2022
@GuySartorelli GuySartorelli deleted the pulls/master/strongly-type-everythynig branch May 3, 2022 04:15
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.

3 participants