Skip to content

Conversation

@imanghafoori1
Copy link
Contributor

@imanghafoori1 imanghafoori1 commented Jan 4, 2019

(Following the currently open PR #27014) This is an other way of fixing the issue of "multiple calls to resolving callback" which came to my mind while I was under the shower today.🚿 🛀

Compared to the solution in: #27014 ,it requires a less code and is more understandable and logical.
Plus, it adds an opportunity to have a fully backward compatible new feature to the IOC container for resolving an object silently. (Which will suppress the resolving callbacks.)
for example : makeSilent(...

This way we fire the callback attached to the interface and skip the re-firing it when resolving the implementation.
which makes more sense, because the callback was actually attached to the interface like this :
app()->bind(someInterface::class, 'Myclass'); `

Anyway I do not know which one you like most, so I leave them both open.

@X-Coder264
Copy link
Contributor

Changing a protected method signature is a breaking change and as such must target the master branch.

About boolean flags in methods - I'm not a fan of them for reasons listed here.

@imanghafoori1
Copy link
Contributor Author

@X-Coder264 No problem with targeting the master branch.
But how it can be breaking??
since we just added an extra parameter with a default value that does not change anything.
And it is not something that is publicly called by the laravel users.

@driesvints
Copy link
Member

@imanghafoori1 people might be overwriting the method

@imanghafoori1
Copy link
Contributor Author

@X-Coder264
I too do not like boolean flag in method signatures. (it is considered to be bad practice.)
We can make a wrapping method (for example resolveSilent()) around the resolve(..., true).
But I am really afraid that the PR might get rejected because of many changes. like previous one got.

That can be fixed in the following PRs separately.

I have made a proposal for that,

laravel/ideas#1470

@imanghafoori1
Copy link
Contributor Author

imanghafoori1 commented Jan 4, 2019

Ok if they override it, why they should pass a third parameter for the
parent::resolve(); ?

If they continue to use it normally and pass the 2 parameters nothing will change.
Let me stress on it, the default value of $silent is false so it does not change the previous behavior.

@imanghafoori1
Copy link
Contributor Author

imanghafoori1 commented Jan 4, 2019

Anyway the PR to the master branch already exists, but it is closed, I can close this and open that.

@X-Coder264
Copy link
Contributor

It's a breaking change because you are gonna get this:

http://sandbox.onlinephpfunctions.com/code/6908b81f74b823e7ac3030ff00e14872b4baf533

Warning: Declaration of Y::foo(bool $x) should be compatible with X::foo(bool $x, bool $y = true) in [...][...] on line11

@imanghafoori1
Copy link
Contributor Author

Aha, Got it.
Thanks for your careful code reviews.
@X-Coder264
@driesvints

@imanghafoori1 imanghafoori1 force-pushed the fix_extra_calls_other_way branch from 8cdd1f2 to f53313a Compare January 4, 2019 16:54
@imanghafoori1
Copy link
Contributor Author

Now we have a little but more change but it is fully backward compatible.
Any suggestion for a better method name for resolveSilent method ?!

@imanghafoori1 imanghafoori1 force-pushed the fix_extra_calls_other_way branch from f53313a to 98a52b8 Compare January 4, 2019 16:58
+ bug fix related to multiple calls to "resolving" and "afterResolving" callbacks
@imanghafoori1 imanghafoori1 force-pushed the fix_extra_calls_other_way branch from 98a52b8 to 54b1a28 Compare January 4, 2019 17:03
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