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

Add a base interface for Middleware classes #232

Closed
rrajkomar opened this issue Oct 18, 2018 · 4 comments
Closed

Add a base interface for Middleware classes #232

rrajkomar opened this issue Oct 18, 2018 · 4 comments
Labels

Comments

@rrajkomar
Copy link
Contributor

rrajkomar commented Oct 18, 2018

Loooking at the current state of the Middleware classes in the bundle, I see that each Middleware class uses a different method name to return the \Closure to be used.

Don't you think it would be best to have a MiddlewareInterface to have something that's always similar ?
To be honest I'm not sure what to use for the name of the method that would return the Closure.

But currently depending on which middleware we have : profile, log, __invoke, and so on.

Note: if you're ok with the idea I can add this.

@gregurco
Copy link
Member

Generally middlewares don't have any interfaces and we can implement one for out 5 middlewares, but what to do with all other from packagist: https://packagist.org/?query=guzzle%20middleware
You can notice that there is no common standard and there is no benefit to introduce it just for 5 middlewares. It will cause problems to connect other.

@rrajkomar
Copy link
Contributor Author

I respectfully disagree.
I'm talking only about the middlewares in this bundle (and potentially others that would want to follow a well known best practice).
Adding an interface might not be a huge gain in anything other than cleaner code (and easier to follow) but it's still a good thing to do.

As for the last part, sorry but I don't follow...
In what way would external middleware be impacted ?
I never said the use of the interface should be imposed but merely recommended.
Each middleware is added to the handler stack at a different step of the process.
An external middleware would have to be added somewhere else (in another piece of code) so I don't see how this changes would impact it.

The only weird thing here is that within the same bundle different method names are used for things that essentially do the same thing : add a middleware closure to the stack...

Anyhow if this is not something you'd want to have... we can close this issue.

@gregurco
Copy link
Member

I agree with you, that all middlewares in bundle should follow the same style and I think the better result would be to use __invoke method but we can't do it right now, because it will break BC. I plan to do it before release v8.
What I think about interface - it makes no sense just to create interface. You should use it. For example, as dependency, like:

function foo(BarInterface $bar) { }

But we don't have any places to use it in this way. Do you see any other cases where interface will help us?

@rrajkomar
Copy link
Contributor Author

Unless we create a "proxy" method to register a middleware in the handler stack then no I don't think there's currently any use case like dependency injection.
But as soon as we agree that the different middleware classes within the bundle should follow the same style, it's all good. I'll wait for v8 to see this 😄

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