Visibility addMiddleware on MiddlewareTrait

[geggleto]

In a recent blog post I made, I received some feedback that the addMiddleware() function on a Route was not meant from public consumption.

I find this rather odd, since it allows for certain optimizations and more readability. I don't see the harm in leaving it public but it was suggested that it should be made protected.

This thread is to discuss the issue.

[silentworks]

Here are the issues I have with this being used publicly and why I think it should be either protected or if being used in a tutorial the writer should state the side effects of not using the add method on the Route class instead.

If a user uses addMiddleware they have no access to the resolver unless they do this manually, it would mean that if I were to do:

$app->get('/', ....)->addMiddleware('Class:method');

That would not work as addMiddleware does not resolve this string. If I were to also pass through a closure I would get similar results If I were to try and access the container using the $this variable.

$app->get('/', ....)->addMiddleware(function () {
   $this->router // This would not work as this is only being bound inside of the Route class add method
});

Now the use case that @geggleto propose might be valid but I can't see how it would give better optimisations, based on what he stated on irc, calling addMiddleware would mean bypassing the resolveCallable which I don't think would give that much of a speed boost as even inside of the CallableResolver we wouldn't run many checks if what we are passing is already a callable https://github.com/slimphp/Slim/blob/3.x/Slim/CallableResolver.php#L78.

Having this method public, feels dangerous and has more cons than pros. To me this will end up with us having more issues coming in from users due to it not working as they would be expecting. I advocate against using it at all and think it should be made either protected or private.

[akrabat]

->add() is the public way to add middleware as far as I am aware. We went to a lot of effort to make ->add() work on route groups for consistency.

[akrabat]

Need to make addMiddleware protected

[akrabat]

Fixed with #1550