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

Two static functions with the same name #62

Merged
merged 2 commits into from
Jul 4, 2021
Merged

Two static functions with the same name #62

merged 2 commits into from
Jul 4, 2021

Conversation

ThibaudDauce
Copy link
Contributor

@ThibaudDauce ThibaudDauce commented May 17, 2021

If two static functions have the same name (in two different classes), the once cache is shared between the two resulting in wrong response during the second call (we can invert the two calls, the second one is always the wrong one).

I've just added a failing test. I read a little bit the code but didn't find the correct way to fix the bug…

In the once function, the $object variable is different for the two classes but the $hash is the same.

@dhrrgn
Copy link

dhrrgn commented Jul 1, 2021

The issue is because the hash only uses the function name as a prefix. This is fine for non-static methods, because the object is unique. However, the "object" is a string with static method, so it uses the Cache instance as the object key in the WeakMap (see here). So, all the cache for all static methods is under the same object key, hence the conflict.

The fix would be to include the class name in the hash prefix for static methods (could probably just always include it really).

@freekmurze
Copy link
Member

@ThibaudDauce could you add that fix to this PR?

@ThibaudDauce
Copy link
Contributor Author

@freekmurze Fixed in a9728f8

@freekmurze freekmurze merged commit bc989e8 into spatie:main Jul 4, 2021
@freekmurze
Copy link
Member

Thanks!

@lloricode
Copy link

thanks I notice this issue, I just forgot to report,
and because of this, i will remove temporary fix on my code, since already fix by this

thanks @ThibaudDauce, @spatie

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.

4 participants