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

feat(laravel): add middleware granularity #6962

Merged
merged 8 commits into from
Feb 24, 2025

Conversation

jonerickson
Copy link
Contributor

@jonerickson jonerickson commented Feb 10, 2025

Q A
Branch? 4.0
Tickets
License MIT
Doc PR TBD

Problem

This PR aims to give more granularity when setting middleware for different features/routes this package provides. The most notable is the ability to enable the GraphQL IDE independent of the actual endpoint. Second, a developer can set middleware explicitly for the GraphQL routes.

Story

An application may want to expose the GraphQL IDE for consumer use. This endpoint may want to use the web and auth middleware for session authentication which typically is not wanted/needed for the API endpoints where a dev would probably opt for some kind of JWT/API/OAuth implementation.

Or, a dev may want to allow GraphQL requests, but does not want the IDE to be exposed in production.

Adding these settings allows more flexibility on how these features can be used.

Changes

  1. Add ability to disable GraphQL IDE graphhiql independent of the graphql POST endpoint.
  2. Set middleware individually on the GraphQL graphql endpoint and GraphQL IDE graphiql endpoint.
  3. Remove unused api-platform.defaults.middleware config setting? I could not find any reference to this config setting and it clashes/confusing with the global middleware set in api-platform.routes.middleware.
  4. Moved and renamed api-platform.defaults.route_prefix setting to api-platform.routes.prefix. Backwards compatibility is provided. This aims to move all routes related config under a common config key versus spread out in the config file.

@jonerickson jonerickson marked this pull request as draft February 10, 2025 21:19
@jonerickson jonerickson force-pushed the add-middleware-granularity branch 2 times, most recently from 8906f4f to fd58ca5 Compare February 21, 2025 22:25
@jonerickson jonerickson changed the base branch from main to 4.0 February 21, 2025 22:33
@jonerickson jonerickson force-pushed the add-middleware-granularity branch from fd58ca5 to 24fd3cd Compare February 21, 2025 22:40
@jonerickson jonerickson force-pushed the add-middleware-granularity branch from 24fd3cd to 1257699 Compare February 21, 2025 22:42
@jonerickson
Copy link
Contributor Author

jonerickson commented Feb 21, 2025

@soyuka I still dont understand what is going on with the routing. It is behaving very strangely. First of all, getRoutePrefix() is returning null. Which then causes L38 of rtrim() to fail. Second, the middleware() calls continue to fail as getMiddleware() on L43 is returning null as well which causes the middleware() call to return an array. How in the world have you gotten your tests to pass? I feel like I haven't changed anything that would cause this strange behavior.

I am not quite up-to-speed on the intricacies of API platform and how all the operations work, but something strange is happening with the routing. This may be linked to #6969. You should be able to call prefix() just fine and have the route correctly prefixed. You should not have to do what you did and compose the URI from scratch in #6978

@soyuka
Copy link
Member

soyuka commented Feb 22, 2025

First of all, getRoutePrefix() is returning null. Which then causes L38 of rtrim() to fail.

My bad on this I'm missing a test, weird that phpstan didn't trigger on this being nullable, my guess is that we ignore the routes/api.php file now.

Second, the middleware() calls continue to fail as getMiddleware() on L43 is returning null as well which causes the middleware() call to return an array. How in the world have you gotten your tests to pass? I feel like I haven't changed anything that would cause this strange behavior.

Yes but it's expected that getMiddleware can return null, this is why you should run:

if ($m = $operation->getMiddleware()) {
    $route = $route->middleware($m);
}

I am not quite up-to-speed on the intricacies of API platform and how all the operations work, but something strange is happening with the routing.

Nothing has an impact on routing except the lines you changed at #6969

@jonerickson
Copy link
Contributor Author

@soyuka Okay, based on #6980 it appears to be all good now. I changed the domain config to default to '' as when null, it causes the Uri parsing to fail. Thanks for the help!

@jonerickson jonerickson requested a review from soyuka February 22, 2025 16:44
@jonerickson jonerickson marked this pull request as ready for review February 22, 2025 16:44
@soyuka soyuka merged commit 1dfefda into api-platform:4.0 Feb 24, 2025
58 of 59 checks passed
@soyuka
Copy link
Member

soyuka commented Feb 24, 2025

Thanks @jonerickson !

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

Successfully merging this pull request may close these issues.

None yet

2 participants