Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

PR #2787 - Fix Tests #1

Merged

Conversation

l0gicgate
Copy link

So I removed CallableResolver and renamed AdvancedCallableResolver to be just CallableResolver. We don't need to implement both, our implementation implements the AdvancedCallableResolverInterface which extends the regular CallableResolverInterface.

I fixed all the tests, just to point out that it was messed up because the regular CallableResolver was binding the closure automatically from within the resolve() method's logic and then the MiddlewareDispatcher was rebinding it again.

Anyway, it's all fixed, 100% coverage. Please review after merging.

@adriansuter
Copy link
Owner

Cool. I will review, after my cycling tour. Thanks.

The reason I left the CallableResolver in our code and tried to rewrite the test cases to test both cases was, that we can make sure that the "old" way was still working as expected for most scenarios.

If for example a user copied the whole code of the CallableResolver and only made small modifications to provide a custom resolver, then we can guarantee, that his code (based on our own "old" code) would still work.

So you think, we don't have to guarantee that?

@l0gicgate
Copy link
Author

We don’t have to guarantee anything about a user implementing their own CallableResolver. I did write some test cases for the case of a non AdvancedCallableResolverInterface though that cover everything.

@adriansuter adriansuter merged commit 4331ab1 into adriansuter:patch-callable-resolver Aug 11, 2019
l0gicgate pushed a commit that referenced this pull request Aug 11, 2019
@l0gicgate l0gicgate deleted the 2787-FixTests branch August 19, 2019 20:15
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants