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

Introduce phpstan #5

Open
wants to merge 3 commits into
base: feat/source-wordpress-v4
Choose a base branch
from

Conversation

szepeviktor
Copy link

@TylerBarnes Is this PR okay?
Should I send it to your repo?

@szepeviktor
Copy link
Author

Please see individual commits.

@@ -297,7 +297,7 @@ function updateUserIsPublic( $post_id, $post ) {
? $current_user->data->user_nicename
: "User";

$relay_id = Relay::toGlobalId( 'user', $user_id );
$relay_id = Relay::toGlobalId( 'user', strval( $user_id ) );
Copy link
Author

@szepeviktor szepeviktor Apr 27, 2020

Choose a reason for hiding this comment

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

@TylerBarnes ID should be a string as it goes into base64_encode
See https://github.com/ivome/graphql-relay-php/blob/master/src/Relay.php#L188

@TylerBarnes
Copy link
Owner

Thanks for this @szepeviktor !
I'll defer to @jasonbahl on this as he's much better on the PHP side than I am.
One thing I will note is that I intentionally removed composer packages before because it was bloating the plugin size and some people couldn't install the zip. This is more about our release process (there isn't one currently :p ). We'll be giving this plugin an overhaul soon and possibly moving it to it's own repo so we can put it on packagist. When we do that we'll be able to set up a better release process that keeps the final zip size smaller.

@szepeviktor
Copy link
Author

szepeviktor commented May 1, 2020

All right.
👀 PHPStan keeps several eyes on your code with a little time investment: writing only testable code.
Please consider raising Level to max.

@szepeviktor
Copy link
Author

@jasonbahl Friendly ping 🏓

@jasonbahl
Copy link
Collaborator

Hey, I don't maintain this plugin anymore.

I think it makes a lot of sense to add phpstan though! Use it on all my plugins now

@szepeviktor
Copy link
Author

@TylerBarnes What do you suggest?

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