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

Represent callable strings as strings #1741

Merged
merged 5 commits into from
May 28, 2024

Conversation

mfb
Copy link
Contributor

@mfb mfb commented May 18, 2024

I'm honestly not sure what the logic should be here. A simple callable string like "file" I don't think should be assumed to be a callable. But seems a lot more reasonable to represent a callable string containing "::" as callable. Here I also represent callable strings containing an underscore as callables.

Fixes #1734

@stayallive
Copy link
Collaborator

Honestly, how about we consider is_string() && function_exists() as a non-callable?

I feel like we can't do it 100% right anyway (without some very advanced reflection and docblock parsing and even then...) and the false-positive rate is going to be a lot smaller than the current situation.

@mfb mfb force-pushed the callable-strings-logic branch from 4617759 to 97712d0 Compare May 18, 2024 14:52
@mfb
Copy link
Contributor Author

mfb commented May 18, 2024

Ok, another take with the simpler logic, representing all callable strings as strings.

@mfb mfb changed the title Represent simple strings that happen to be callable as strings (#1734) Represent callable strings as strings (#1734) May 22, 2024
@mfb mfb changed the title Represent callable strings as strings (#1734) Represent callable strings as strings May 22, 2024
@cleptric
Copy link
Member

Please add a test.

Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

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

Thanks, this will reduce false-positives! 👍

@stayallive stayallive requested a review from cleptric May 27, 2024 09:14
@cleptric cleptric enabled auto-merge (squash) May 28, 2024 16:06
@cleptric cleptric merged commit 6051f41 into getsentry:master May 28, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strings that happen to be a php function are reported as a callable
3 participants