-
Notifications
You must be signed in to change notification settings - Fork 109
Addition of new middleware, RoutesBuilderMiddleware! #103
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
Conversation
Hi. I like the code, looks clean. I have two questions:
|
Thanks! I've been running a heavily hacked together version of this locally for a while now, but I've only just now got around to thinking about how to upstream things.
I'm currently using a third party Laravel library in quite a few projects that allows me to essentially abstract out how models, relationships and ultimately API endpoints/routes are defined - in what are essentially config files. The project is https://laraveljsonapi.io As information about what is considered an acceptable request body, expected response body, relationships etc is already well known, and more importantly, is very strictly defined - we can avoid the effort of manually describing each model, relationship, endpoint etc and through the use of Laravel Magic™, automatically generate this documentation. Very useful when working on projects with anywhere from 10 to 300 distinctive programmatically defined endpoints(!) to audit what's actually available (and if it can be pruned!) A side bonus of this is that it creates time to write the docs for endpoints that aren't using that JSON:API package.
I had a feeling this would be raised as a concern, however what I expected would have been raised was me changing the The reason why I did this, ultimately, was an attempt to keep consistency with how the I'm struggling to find a way to pivot from config, to a route builder/collector with a way of passing middlewares across. The only way I can think, would be to do something like: <?php
namespace Vyuldashev\LaravelOpenApi\Builders;
class PathsBuilder
{
...
public function build(string $collection, array $middlewares): array {
$middlewares = collect($middlewares);
$pathMiddlewares = $middlewares->filter(fn ($middleware) => is_a($middleware, PathMiddleware::class, true));
$routeBuilderMiddlewares = $middlewares->filter(fn ($middleware) => is_a($middleware, RouteInformationMiddleware::class, true));
return $this->routes($routeBuilderMiddlewares)
->filter(static function (RouteInformation $routeInformation) {
...
})
->map(static function (RouteInformation $item) use ($routeBuilderMiddlewares) {
foreach ($pathMiddlewares as $middleware) {
app($middleware)->before($item);
}
return $item;
});
}
} ...and pass in config like: $config = [
'collections' => [
'default' => [
'middlewares' => [
'paths' => [
\Path\To\MyPathMiddleware::class,
\Path\To\MyRouteInformationMiddleware::class,
],
],
],
],
]; If you're okay with that, this would ensure 100% method compatibility, just makes things slightly less clearer. |
PS: will probably change |
Update: I've misunderstood how |
@vyuldashev I've changed how middlewares work internally, following the notes issued in the documentation. If users of this library have followed documentation correctly, then their middlewares will continue to work. I have now made it so that multiple types of middlewares can be thrown into a builder and only the correct flow will be evaluated. In addition I have made it so that there's no changes to any public methods, the only prototype change is a protected function which shouldn't be relied upon for prototype compatibility. I will be very appreciative of your thoughts RE: these changes. |
@vyuldashev I was wondering if you had the chance to review these changes? |
@Westie Hello, will definitely review soon, thanks. |
Hi @vyuldashev, have you had a chance to review this PR yet? :-) |
I'm working on a project that involves autodocumenting API endpoints, as well as manually documenting endpoints that are not procedurally generated.
The purpose of this PR is to introduce a new middleware -
RouteInformationMiddleware
- that allows a developer to have a bit more control over the RouteInformation object.In addition, as part of this PR, I would also like to propose that we can now optionally pass the
RouteInformation
instance to any operation builder.Tests and doc changes supplied.
Furthermore: the code in #55, this has component middleware layer only running if there are already components assigned.
I personally think that behaviour is incorrect but I'd love your thoughts @vyuldashev and @nandi95