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

2.x Update PHPStan setup #2630

Merged
merged 14 commits into from Jan 24, 2023
Merged

2.x Update PHPStan setup #2630

merged 14 commits into from Jan 24, 2023

Conversation

gchtr
Copy link
Member

@gchtr gchtr commented Aug 15, 2022

Issue

There are some PHP libraries we can use to provide function stubs for PHPStan instead of adding them ourselves.

Solution

Use php-stubs/acf-pro-stubs and php-stubs/wp-cli-stubs.

For now, this isn’t working because the function signature for acf_get_field_type() in php-stubs/acf-pro-stubs.

 ------ -------------------------------------------------------
  Line   Integration/AcfIntegration.php
 ------ -------------------------------------------------------
  244    Result of function acf_get_field_type (void) is used.
  245    Result of function acf_get_field_type (void) is used.
  246    Result of function acf_get_field_type (void) is used.
  247    Result of function acf_get_field_type (void) is used.
  248    Result of function acf_get_field_type (void) is used.
  249    Result of function acf_get_field_type (void) is used.
  250    Result of function acf_get_field_type (void) is used.
  251    Result of function acf_get_field_type (void) is used.
  252    Result of function acf_get_field_type (void) is used.
 ------ -------------------------------------------------------

I guess it’s because ACF uses a return type of n/a in the DocBlock for acf_get_field_type(), which is translated to void. But actually it should return mixed, when we look at the underlying acf()->fields->get_field_type() function.

For an easy fix, we could switch to using acf()->fields->get_field_type() instead of acf_get_field_type(), but maybe this is an issue that can be fixed in the php-stubs/acf-pro-stubs library.

I hope that @szepeviktor can help us out here 😊.

Impact

Less code to manage.

Usage Changes

None.

@gchtr gchtr added the 2.0 label Aug 15, 2022
@szepeviktor
Copy link
Contributor

Use php-stubs/acf-pro-stubs and php-stubs/wp-cli-stubs.

I suspect these are fine packages :)

@gchtr
Copy link
Member Author

gchtr commented Aug 15, 2022

@szepeviktor Well, they apparently were made by someone who knows his stuff 😉💪

@szepeviktor
Copy link
Contributor

szepeviktor commented Aug 15, 2022

I guess it’s because ACF uses a return type of n/a in the DocBlock

I contacted Elliot and he was happy about docblock fixes but refused them.
Currently invalid docblocks are dumbfixed with string replacement.
https://github.com/php-stubs/acf-pro-stubs/blob/7e44b653bd72e5eebf725f3af6b5afaae3039dcf/generate.sh#L30-L43

Fixing those could be also be done with sed.

@szepeviktor
Copy link
Contributor

szepeviktor commented Aug 15, 2022

I would gladly apply a patch on top the released stubs if you provide me one fixing functions in includes/fields.php.
That would go like this: patch -p 1 < acf-pro-field-function-fixes.patch

@gchtr
Copy link
Member Author

gchtr commented Aug 15, 2022

@szepeviktor Thanks for your insights!

I contacted Elliot and he was happy about docblock fixes but refused them.

I see 🙃.

Currently invalid docblocks are dumbfixed by string replacement.
php-stubs/acf-pro-stubs@7e44b65/generate.sh#L30-L43

Yes, I saw that :). I’m not familiar enough with sed though.

I would gladly apply a patch on top the released stubs if you provide me one fixing functions in includes/fields.php.
That would go like this: patch -p 1 < acf-pro-field-function-fixes.patch

So, you mean I could provide you with a .patch file that fixes that particular function signature and you could apply that when generating the stubs? I think I could prepare that.

@szepeviktor
Copy link
Contributor

I think I could prepare that.

All right!
To be clear: please start with the stubs.

szepeviktor
szepeviktor previously approved these changes Oct 19, 2022
Copy link
Contributor

@szepeviktor szepeviktor left a comment

Choose a reason for hiding this comment

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

Very nice work 💎

@gchtr gchtr marked this pull request as ready for review October 19, 2022 20:55
@gchtr
Copy link
Member Author

gchtr commented Oct 19, 2022

I decided to go the easy way and just use acf()->fields->get_field_type() instead of acf_get_field_type() to make it work with the php-stubs/acf-pro-stubs package.

@gchtr gchtr added the Ready for Review Ready for a contrib to take a look at and review/merge label Dec 30, 2022
# Conflicts:
#	src/Post.php
@gchtr gchtr merged commit 89529f0 into 2.x Jan 24, 2023
@gchtr gchtr deleted the 2.x-phpstan-setup branch January 24, 2023 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 Ready for Review Ready for a contrib to take a look at and review/merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants