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

Middleware Callable Resolver Support Bug #2919

Closed
kwhat opened this issue Jan 7, 2020 · 3 comments
Closed

Middleware Callable Resolver Support Bug #2919

kwhat opened this issue Jan 7, 2020 · 3 comments
Labels

Comments

@kwhat
Copy link

kwhat commented Jan 7, 2020

In the Slim 4 feedback I had some criticism for the slim 4 middleware process. As promised, I have some more details about the issues I was having. The support added back in #2780 was a good idea, however, it appears to have some flawed logic related to middleware construction if added as a string that is defined in the container. Ex: $app->add(ExceptionHandler::class);

What I expect: The class should be resolved in the container and use the $defaultMethod specified to CallableResolver::resolveByPredicate() by the AdvancedCallableResolverInterface::resolveMiddleware() method.

What is actually happening: The middleware is resolved as a string, constructed with the container and assumed to use the '__invoke' method.

The solution: The source of the issue appears to be a general over engineering of the CallableResolver. The resolveByPredicate() method is unneeded, and I generally wonder if the AdvancedCallableResolverInterface is needed at all. I have included a proposed fix in PR form to remove the resolveByPredicate() method and refactor the resolveSlimNotation() method to be a little more ergonomic.

After reviewing the new Slim 4 code base, this feels like a common trend when compared to Slim 2/3. Things are not particularly gratuitous, there were just a lot of features added, and the simplicity got lost in the implementation details.

kwhat added a commit to kwhat/Slim that referenced this issue Jan 7, 2020
kwhat added a commit to kwhat/Slim that referenced this issue Jan 7, 2020
kwhat added a commit to kwhat/Slim that referenced this issue Jan 7, 2020
kwhat added a commit to kwhat/Slim that referenced this issue Jan 7, 2020
kwhat added a commit to kwhat/Slim that referenced this issue Jan 8, 2020
@kwhat
Copy link
Author

kwhat commented Jan 8, 2020

There is still one failing test, but I didn't see this one until I pulled the most recent 4.x and appears to be unrelated to this change. I did look at the MiddlewareDispatcher in terms of callable resolution... woof. For all the effort that went into decoupling stuff for 4.x, I wonder why this class is also a CallableResolver. I am also wondering if the fixes I put in place will actually fix the issue I am experiencing.

@l0gicgate
Copy link
Member

I don't actually understand the issue you're experiencing.

Can you show your ExceptionHandler class? Does it implement MiddlewareInterface?

@kwhat
Copy link
Author

kwhat commented Jan 8, 2020

I finally tracked it back to a freaking container issue, ie the class exists but its not in the container (when I thought it was) so it was getting constructed with ContainerInterface which is the intended behavior. I am not super stoked that this took 2 days to sort though, the old and new code are functioning identical, do with this issue/pr whatever you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants