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

Support returning an array or a string in count_chars() #3596

Merged
merged 1 commit into from
Nov 5, 2024
Merged

Support returning an array or a string in count_chars() #3596

merged 1 commit into from
Nov 5, 2024

Conversation

u01jmg3
Copy link
Contributor

@u01jmg3 u01jmg3 commented Nov 1, 2024

@u01jmg3

This comment was marked as resolved.

@staabm
Copy link
Contributor

staabm commented Nov 1, 2024

Still need to support the method returning false in PHP <8

for a php version dependent return-type a DynamicFunctionReturnTypeExtension would be a better fit then a stub.
sorry, I just saw this comment right now.

we have a few of them in the codebase already for inspiration.

@u01jmg3
Copy link
Contributor Author

u01jmg3 commented Nov 1, 2024

Yikes - as this codebase is so new to me, I'm struggling to know where to start. It looks like this is the first time a string function has made use of the DynamicReturnTypeExtension.

NativeReflectionEnumReturnDynamicReturnTypeExtension looks to be one of the simplest but I don't know what count_chars() needs for getClass()|isMethodSupported()|getTypeFromMethodCall() and all of this just to capture and also return false on PHP <8. 😮‍💨

  • Is there much value when PHPStan 2 requires ^8.1?
  • Is this PR no longer valid as is?

@staabm
Copy link
Contributor

staabm commented Nov 1, 2024

t looks like this is the first time a string function has made use of the DynamicReturnTypeExtension.

see e.g. https://github.com/phpstan/phpstan-src/blob/2.0.x/src/Type/Php/AbsFunctionDynamicReturnTypeExtension.php which is similar to what we need.

  • Is there much value when PHPStan 2 requires ^8.1?

PHPStan 2.x will have min php version of 7.4.
8.1 is only used for development in the code-base but is downgraded for 7.4 compat on release.

  • Is this PR no longer valid as is?

the test-cases are still useful

Copy link
Contributor

@staabm staabm left a comment

Choose a reason for hiding this comment

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

this looks pretty good already

Copy link
Contributor

@staabm staabm left a comment

Choose a reason for hiding this comment

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

Congratulations. You successfully build your first PHPStan extension :-)

@u01jmg3
Copy link
Contributor Author

u01jmg3 commented Nov 1, 2024

@staabm: thanks for your patience/guidance 🙏🏻

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@ondrejmirtes ondrejmirtes merged commit 8019243 into phpstan:2.0.x Nov 5, 2024
404 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

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.

None yet

5 participants