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

[5.7] Bug fix related to extra calls to resolving callbacks (The first way) #27014

Closed

Conversation

imanghafoori1
Copy link
Contributor

@imanghafoori1 imanghafoori1 commented Dec 31, 2018

Add missing tests for the resolving callbacks on the container

@imanghafoori1 imanghafoori1 force-pushed the Add_missing_resolving_tests branch 3 times, most recently from 38cd472 to a002637 Compare December 31, 2018 13:16
@imanghafoori1 imanghafoori1 changed the title [5.7] Added missing test for the resolving callback on the container [5.7] Bug fix related to multiple calls to resolving callbacks Dec 31, 2018
@imanghafoori1 imanghafoori1 changed the title [5.7] Bug fix related to multiple calls to resolving callbacks [5.7] Bug fix related to extra calls to resolving callbacks Dec 31, 2018
src/Illuminate/Container/Container.php Outdated Show resolved Hide resolved
@imanghafoori1 imanghafoori1 force-pushed the Add_missing_resolving_tests branch 5 times, most recently from da25f0a to e2194f0 Compare December 31, 2018 22:28
@imanghafoori1 imanghafoori1 force-pushed the Add_missing_resolving_tests branch 2 times, most recently from a374e3a to 5f01ad7 Compare January 1, 2019 19:24
@imanghafoori1
Copy link
Contributor Author

imanghafoori1 commented Jan 1, 2019

Why a new property ?

protected $boundToClassName = [];
Since the container $app->bind(interface::class, some::class); immediately wraps string concretes like (some::class) in a closure and normalizes them in the bind method. we can not detect which one is which down the road.
So, with the help of a new protected property on the container class we keep track of them.
And we can decide later what to do.

@imanghafoori1 imanghafoori1 force-pushed the Add_missing_resolving_tests branch 2 times, most recently from a685f3e to 9661ee1 Compare January 1, 2019 19:49
@imanghafoori1
Copy link
Contributor Author

imanghafoori1 commented Jan 1, 2019

Note : tests proof that the exact same problem exists for afterResolving and global callbacks too,
By placing the check around the $this->fireResolvingCallbacks($abstract, $object); we target all of them.

@imanghafoori1 imanghafoori1 changed the title [5.7] Bug fix related to extra calls to resolving callbacks [5.7] [WIP] Bug fix related to extra calls to resolving callbacks Jan 1, 2019
@imanghafoori1 imanghafoori1 force-pushed the Add_missing_resolving_tests branch 2 times, most recently from 3058aa5 to 744ea27 Compare January 1, 2019 20:12
@imanghafoori1 imanghafoori1 changed the title [5.7] [WIP] Bug fix related to extra calls to resolving callbacks [5.7] Bug fix related to extra calls to resolving callbacks Jan 1, 2019
@imanghafoori1 imanghafoori1 force-pushed the Add_missing_resolving_tests branch 2 times, most recently from fda65f3 to 85fe83c Compare January 1, 2019 21:16
@imanghafoori1
Copy link
Contributor Author

imanghafoori1 commented Jan 1, 2019

A lot of tests are added, I know they may look more than needed, but each of them cover a unique case and they all should exists.
And happy new year. 🎄 🎄

@imanghafoori1 imanghafoori1 force-pushed the Add_missing_resolving_tests branch 2 times, most recently from 1df63e6 to 0e01521 Compare January 2, 2019 13:56
+ bug fix related to multiple calls to "resolving" and "afterResolving" callbacks
@imanghafoori1
Copy link
Contributor Author

imanghafoori1 commented Jan 4, 2019

Note that an other way of fixing it is made here: #27061

which I think #27066 is a lot better, this way is somewhat hacky but seems to be fully backward compatible👎

@imanghafoori1 imanghafoori1 changed the title [5.7] Bug fix related to extra calls to resolving callbacks [5.7] Bug fix related to extra calls to resolving callbacks (The first way) Jan 4, 2019
@imanghafoori1 imanghafoori1 deleted the Add_missing_resolving_tests branch January 10, 2019 22:34
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

3 participants