-
Notifications
You must be signed in to change notification settings - Fork 38
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
Custom implementation of CallableResolver breaks deferred middleware #51
Comments
Thanks for the report. Could you detail what is the extra-processing that is missing? (the line you pointed to doesn't make it obvious to me) Do we need to implement |
@mnapoli sure thing.
When calling Implementing However, I don't really understand why |
As an aside, I feel like Slim (and by extension, Slim Bridge) has gotten overly complicated. Instead of just embracing PSR-15, we have this legacy "callable but maybe not" logic all over the place. |
This issue is still open and still is a BIG issue. Because make impossible to use Middleware on slim 4. |
Just to make it clear, it is very much possible to use middleware on Slim ... // instead of
$slim->add(Middleware::class);
// do this
$slim->add($container->get(Middleware::class)); Don't always rely on magic. P.S.: If you want to make the middleware lazy loaded, try this approach: $slim->add(new GenericMiddleware(function(Request $request, Handler $next) use($container): Response {
$mw = $container->get(Middleware::class);
return $mw->process($request, $next);
})); (see this package or use your own implementation) Edit: in fact, you don't even need the |
@dakujem yes i know that is possible. Find funny when people use word "magic" for describing something. Sometime when you use the word Programming must be fun and simples! |
I did not mean it disrespectfully. By "magic" I mean something that makes your dev life easier, but usually comes at cost. Transparency and flexibility are the most frequent tradeoff. |
@dakujem sorry, if i sound "arrogant" but i'm not english native. But yeah, i'm agree with you. I prefered transparency and flexibility over "magic". |
I would also add that we should try to have the bridge not change default slim usage or at least document the issue clearly. Even if documentation existed, I think that using This issue tripped me up for some time before finding this thread a while ago and almost made me uninstall the bridge and make something custom which would have been silly now that I know the issue but even still, it feels like this is unexpected behaviour since the bridge should be able to be built and then used like a normal slim app therein. |
@jamesrweb yeah, in order to use Slim Bridge you need to resolve your self middleware from container before putting on |
@gravataLonga sometimes that is really problematic. For instance, if a middleware creates an object from data in the request and puts it into the container. Such object creation may not be entirely ideal, but sometimes it is necessary. The Slim-Bridge fundamentally changes Slim functionality by preventing the usage of middleware as class strings to support invoker features. This is not clearly documented and, in my opinion, not necessary. The only time this is useful is when using anonymous functions as route handlers. |
@shadowhand yeah i know and know your pain, i'm not maintainer of this package FYI. Still wait for good solution for this. |
workaround used in our projects: <?php
namespace MyProject\DI;
use Invoker\CallableResolver as InvokerCallableResolver;
use Psr\Container\ContainerInterface;
use Slim\CallableResolver as SlimCallableResolver;
use Slim\Interfaces\AdvancedCallableResolverInterface;
class CallableResolver implements AdvancedCallableResolverInterface
{
private $callableResolver;
private $slimCallableResolver;
public function __construct(ContainerInterface $container)
{
$this->callableResolver = new InvokerCallableResolver($container);
$this->slimCallableResolver = new SlimCallableResolver($container);
}
public function resolve($toResolve): callable
{
return $this->callableResolver->resolve($toResolve);
}
public function resolveRoute($toResolve): callable
{
return $this->resolve($toResolve);
}
public function resolveMiddleware($toResolve): callable
{
return $this->slimCallableResolver->resolveMiddleware($toResolve);
}
}
...
use MyProject\DI\CallableResolver;
use Slim\Factory\AppFactory;
use Slim\Interfaces\CallableResolverInterface;
class MyAppFactory {
public static function createSlimApp(ContainerInterface $container)
{
$callableResolver = new CallableResolver($container);
$container->set(CallableResolverInterface::class, $callableResolver);
...
return AppFactory::createFromContainer($container);
}
} |
I fail to understand what the struggle here is. How simple it is to use the middleware like this, if the bridge does not work for you? $slim->add(
fn(
Request $request,
Handler $next,
): Response => $slim->getContainer()->get(MyMiddleware::class)->process($request, $next)
); It's easy to create a reusable function that does this for you (you can place it into a class obviously): $mwLazy = function (string $className) use ($slim): callable {
return fn(
Request $request,
Handler $next,
): Response => $slim->getContainer()->get($className)->process($request, $next);
};
$slim->add($mwLazy(MyMiddleware::class)); Or If the power of the advanced resolved is desired, why not create it yourself? $mwResolver = fn(
string|MiddlewareInterface $middleware
): MiddlewareInterface => (new \Slim\CallableResolver($slim->getContainer()))->resolve($middleware);
$slim->add($mwResolver(MyMiddleware::class)); @kakenbok above provided a viable solution that wraps the functionality of both the DI-bridge and the original Slim resolver. That is the most advanced solution you can use if you can manage the bootstrapping. 💪 |
@dakujem the problem is this library don't preserver behavior of slim that is descrive here: If i have more time will try to explain more later. |
I stumbled upon this issue as well in my current project, and I adapted @kakenbok's code to a pull request which could hopefully fix this issue. |
The struggle is that the feature that works in Slim is broken by Bridge. That is a concrete problem, regardless of opinions on magic of the feature itself and workarounds for it. |
I opened an issue upstream as well, because arguably if the intent is to let a resolver throw an exception and proceed to fallback code then it should handle this case without issue. The problem is Invoker exception being thrown isn't caught upstream because it doesn't extend |
I poked through the code some more and I think it's not just that resolving middleware is broken, it's that Slim and Bridge callable resolution had significantly diverged in what they consider a valid input and result.
Whether one sees that as Bridge breaking Slim or Bridge improving Slim is matter of point of view. The question is, does Bridge intend to be incompatible with Slim on this? If the answer is "no", then Bridge's callable resolution needs to be rewritten to be a superset of Slim's, not a competing alternative. If the answer is "yes", some doc updates are probably in order to emphasize incompatibility. |
@Rarst this is an excellent summary. The thing is that this bridge has existed for a while, across multiple Slim versions. Slim introduced a notation ( So far, the Slim notation is not supported by this bridge (in controllers). Regarding middleware, I think the bridge should be consistent with controllers.
That might be worth indeed. |
I don't think anyone is especially "at fault" here, just that two projects ended up falling out of sync. :) Personally I would prefer a superset rebuild, if you are open to it I can try find time to contribute that? Otherwise I would also encourage to let user decide which resolution logic they want when using Bridge. |
Having figured out what the differences are, this should work with Bridge to add and resolve PSR middleware from a class name (array syntax causes deprecation notices since something tries to call it as static method): $app->add(Middleware::class . '::process'); |
A valid PSR15 middleware being added via |
That could be an interesting possibility. The main thing I would worry about is making sure that we don't loose anything from the current implementation, i.e. all these things work:
If you are interested I'd love to see that, ideally I'd love to involve 1 person (besides me) to maintain this package. |
PRed a little more backwards compatible take on bringing these things back to parity with Slim in #71, testing/feedback welcomed. :) |
I really like the simplicity of #70. |
please do #70. |
Yes, it's actual problem |
Guys, I've solved the problem. You can choose 3 ways: 1) This package (php-di/bridge) has the if ($callable instanceof MiddlewareInterface) {
return fn ($request, $next) => $callable->process($request, $next);
} to the public function resolve($callable): callable
{
if (is_string($callable) && strpos($callable, '::') !== false) {
$callable = explode('::', $callable, 2);
}
$callable = $this->resolveFromContainer($callable);
if (! is_callable($callable)) {
// new code
if ($callable instanceof MiddlewareInterface) {
return fn ($request, $next) => $callable->process($request, $next);
}
throw NotCallableException::fromInvalidCallable($callable, true);
}
return $callable;
} Also I sent a pull-request with this correction to the BUT I don't recommend to use this way and this package for one important reason - It decrease performance. The point is that the if ($this->callableResolver) {
$callable = $this->callableResolver->resolve($callable);
} But there is no need to invoke the $callable = $this->callableResolver->resolve($this->callable); and then passes the resolved return $strategy($callable, $request, $response, $this->arguments); The 2) You can use my component written for the Slim microframework - https://github.com/solventt/slim-route-strategy. The package has detailed documentation, in particular the 3) Also you can write your own route strategy for the Slim. About the invocation strategy you can read in the Slim docs. But it requires you to learn the Slim internals. |
What is state of this issue? Still deciding which solution is best? Love to know if I can help |
@gravataLonga there have been at least 3 solutions contributed and proposed but we need a maintainer such as @mnapoli to actually decide on one option that works best for the eco system and merge. Basically just waiting for that. |
Can I get 1 or 2 confirmation that #71 is good for you? Just to be clear, please don't review #71 and nitpick on formatting or other things like that (the PR has been opened for a while and I would understand that the author may not be around to update it). I'm just trying to make sure that #71 is an acceptable solution. If it is, I can merge and release. |
The other option is from @solventt which would also work nicely IMO since it addresses topics upstream too. I'm ok with anything that fixes the issue but I'd also like it to be a considered fix from the maintainer side and not just a "that one will do" choice. I trust @mnapoli to choose the right PR for the ecosystem and if #71 is it, I'm on board. 🚀 |
Am around, if any tweaks needed. :) |
Thank you all, and @Rarst! I've tagged https://github.com/PHP-DI/Slim-Bridge/releases/tag/3.2.0 |
That's not what the comment is saying exactly, and I don't think Slim 5 development has even started at this point (there are no issues or branch open).
There is a certain amount of duplication, because (as per issue you linked) it didn't quite came out "perfect" upstream and had to be evolved within backwards compatibility of major version. When you say "performance problem" do you mean actual measurable regression in profiling or just that there is some code duplication going on? |
A quote from the issue: "Ultimately in Slim 5 we should get rid of AdvancedCallableResolver and unify the 2."
Its not excluded. But in the PR one of the authors of Slim told me: "We won't be able to merge this until we bump PHP lowest version support to 7.4 in about a month unfortunately". Now Slim version is 4.9.0. Do you think changing the lowest version of php will be released in the patch version? But I don't rule anything out here either.
Of course I didnt benchmark performance but if you did, we'd love to see it. This is just my opinion. And this duplication is not a simple code, but the functionality of Slim: And the maintainer of this package told me: "I don't think this might be the right approach since it introduces a special behavior for a PSR class. The Invoker tries to stay generic, we don't really want a if for a specific class in there". Judging by his words, he is concerned about the quality of organization of the code. And it seemed to me that the duplication of Slim functionality he may also consider as not a very acceptable thing. |
My point was that it's less "this interface is gone and supporting it is moot" more "things will evolve in some way in next major version eventually". Even if the name of advanced interface is retired, I would expect the functionality be in line with it, rather than regress to an earlier state. In a nutshell, Bridge is quite involved customization and it will have to adapt to any major Slim release in a ways that are hard to predict.
Not this specifically, but this hadn't raised any performance concerns for me. Just clarifying if this had been a performance problem for you, or you just consider it could be more optimal/fast. I consider those to be two different classes of an issue. :)
Dependency updates do not enforce a major version bump in semver, however I am not involved in development and what they plan to do. I'd be surprised to see a major Slim version with only PHP dependency changed. In a nutshell, yes this will all probably will have to be revisited for Slim 5 regardless, no it's probably not any time soon. |
This is one of the reasons why I recommended above to write your own route strategy (see Slim docs) - this is the best solution in this situation, that's what I did. The route strategy is Slim's native tool for providing functionality, which this package also provides. |
@solventt let's leave it at that please, this discussion is pinging 10 different people. If anyone sees performance issues, please detail them with numbers. Anything else isn't useful. In Slim 5, things will change. That's a given. We can't support Slim 5 until we know what it will be. |
Assuming that
MyMiddleware implements MiddlewareInterface
, it should be possible to do:However, this breaks completely because the
CallableResolver
that is injected byBridge::create()
doesn't do any of the extra processing that Slim's does to handle middleware using the advanced callable resolver.The text was updated successfully, but these errors were encountered: