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

Handle FuncCall nodes that represent first class callables #32

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

spawnia
Copy link

@spawnia spawnia commented Apr 19, 2022

Stumbled upon this while working on a fix for #31. The original fix no longer applies, but I think this is still valuable to ensure PHP 8.1 compatibility.

@dbrekelmans
Copy link
Collaborator

@spawnia Could you resolve the conflict?

Also would this interact with #33? I'm thinking it might be best to merge this PR before #33, what do you think?

@spawnia spawnia force-pushed the handle-potential-first-class-callable branch from 33e3983 to abaeae5 Compare November 25, 2024 08:54
@spawnia
Copy link
Author

spawnia commented Nov 25, 2024

@dbrekelmans I rebased atop the latest changes. I think #33 and this can be applied independently from one another. The condition I put to check for first class callables should come before the condition added there.

@shish
Copy link
Collaborator

shish commented Feb 19, 2025

Hi all, new maintainer here, hoping to get this merged if it's still relevant. Coming in with little context, what does the PR do? Could it include a unit test showing what fails now, and succeeds after the patch is applied?

(There are merge conflicts and failing tests, and I daren't try to fix it myself without understanding the intention behind the code 😅 )

…first-class-callable

# Conflicts:
#	composer.json
#	src/Rules/UseSafeFunctionsRule.php
@spawnia
Copy link
Author

spawnia commented Feb 24, 2025

Thank you @shish for looking into this. I tried adding a failing test. I think first-class callables should also be recognized by this rule, such as json_encode(...).

@shish
Copy link
Collaborator

shish commented Feb 25, 2025

I have enabled the CI, but it looks like the added unit test is failing both before and after the changes to add support for it o.o;;

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