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 string callable in case of non-static method #821

Merged
merged 16 commits into from Oct 14, 2019
Merged

Handle string callable in case of non-static method #821

merged 16 commits into from Oct 14, 2019

Conversation

Jean85
Copy link
Collaborator

@Jean85 Jean85 commented May 23, 2019

This is an improvement over #815. Thanks to @dmitry-pro for spotting the bug to start with.

@Jean85
Copy link
Collaborator Author

Jean85 commented May 23, 2019

Umh, there's something strange... I cannot reproduce the bug outside of Sentry's tests: https://3v4l.org/1CEnC

What's really triggering the issue in here?

@dmytro-pro
Copy link

@Jean85 don't have much time for testing, but it is
https://3v4l.org/XMiVr
Strange that it doesn't trigger in Your (previous) example - when I caught this error, it was found in public dynamic method, not function.
I'll test it more when I free.

@Jean85
Copy link
Collaborator Author

Jean85 commented May 23, 2019

I've found the difference: the issue is present with functions, not with class methods: https://3v4l.org/4ibt7

Not sure why there's a difference, the bug seems to be very subtle.

[EDIT] I have found the root cause, and it's not a bug: https://bugs.php.net/bug.php?id=61467#1332546423

As explained by Rasmus himself, the point is that the callable type has to trigger the autoloader and invoke it to check if it's an actual callable or a simple string, so that deprecation is unavoidable.

@ste93cry
Copy link
Collaborator

Unfortunately we cannot remove the typehint or we will have a BC

@Jean85
Copy link
Collaborator Author

Jean85 commented May 23, 2019

Unfortunately we cannot remove the typehint or we will have a BC

Crap that's true... maybe we can hotfix it in the previous method, but it would still be broken for other usages

@chekalsky
Copy link

@Jean85 When are you planning to release it? Pretty annoying bug :-)

src/Serializer/AbstractSerializer.php Outdated Show resolved Hide resolved
@Jean85 Jean85 requested a review from ste93cry October 2, 2019 12:52
@Jean85 Jean85 changed the base branch from master to develop October 2, 2019 12:54
@Jean85 Jean85 closed this Oct 2, 2019
@Jean85 Jean85 reopened this Oct 2, 2019
@ste93cry ste93cry changed the base branch from develop to master October 2, 2019 18:22
@ste93cry ste93cry added this to the 2.2 milestone Oct 2, 2019
Copy link
Collaborator

@ste93cry ste93cry left a comment

Choose a reason for hiding this comment

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

Would you be so kind to fix the remaining issues and rebase the PR on top of the master branch? Also does the test in AbstractSerializerTest covers this issue?

src/Serializer/AbstractSerializer.php Outdated Show resolved Hide resolved
src/Serializer/AbstractSerializer.php Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@Jean85 Jean85 requested a review from ste93cry October 14, 2019 08:03
@ste93cry ste93cry merged commit be044bf into master Oct 14, 2019
@ste93cry ste93cry deleted the pr_815 branch October 14, 2019 09:09
@Jean85 Jean85 mentioned this pull request Oct 31, 2019
@emircanerkul
Copy link

Hi @ste93cry @Jean85

I face the same issue now while using sentry/sentry 3.22.1.

Is there any regression?

Deprecated: Use of "static" in callables is deprecated in /app/vendor/sentry/sentry/src/Serializer/AbstractSerializer.php on line 265 Deprecated: Use of "static" in callables is deprecated in /app/vendor/sentry/sentry/src/Serializer/AbstractSerializer.php on line 265 Deprecated: Use of "static" in callables is deprecated in /app/vendor/sentry/sentry/src/Serializer/AbstractSerializer.php on line 265 Deprecated: Use of "static" in callables is deprecated in /app/vendor/sentry/sentry/src/Serializer/AbstractSerializer.php on line 265 Deprecated: Use of "static" in callables is deprecated in /app/vendor/sentry/sentry/src/Serializer/AbstractSerializer.php on line 265 Deprecated: Use of "static" in callables is deprecated in /app/vendor/sentry/sentry/src/Serializer/AbstractSerializer.php on line 265

@ste93cry
Copy link
Collaborator

ste93cry commented Apr 4, 2024

It may be, even though it's the first time it is reported since years... I suggest to try version 4.x of the SDK, and if the issue persist open a new issue.

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.

None yet

6 participants