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

Fix: route registration of EntrypointController should be after GraphQL's EntrypointController #6667

Conversation

BurningDog
Copy link
Contributor

@BurningDog BurningDog commented Sep 23, 2024

Q A
Branch? 4.0
Tickets Closes #2767
License MIT

If GraphQL is enabled and API Platform's route prefix is configured to be an empty string instead of the default /api then the /graphql route is incorrectly handled by \ApiPlatform\Laravel\Controller\EntrypointController instead of ApiPlatform\Laravel\GraphQl\Controller\EntrypointController.

This is because the routes being handled by the former controller are registered before the GraphQL routes. Instead, they should be registered last.

@BurningDog
Copy link
Contributor Author

Hmmm...I'm not quite sure why the Behat tests are breaking. At first glance it's tempting to assume that routing is breaking, but I can add #[QueryParameter(key: 'author', filter: PartialSearchFilter::class)] to the Book model locally and run

curl -X 'GET' \
  'http://127.0.0.1:8000/books?page=1&author=Nikki' \
  -H 'accept: application/ld+json'

and get a response just fine.

So perhaps the mechanism for seeding data for the tests is now broken, and so no expected results are returned in the tests...but I've never used Behat and I'm not sure where the data fixtures are.

Advice welcome!

@soyuka soyuka force-pushed the fix/route-registration-of-entrypointcontroller-should-be-last branch from c48830f to 419c8d2 Compare September 23, 2024 19:41
@soyuka
Copy link
Member

soyuka commented Sep 23, 2024

No worries we're using only phpunit for Laravel, instructions to run tests are at src/Laravel/CONTRIBUTING.md.

Nice catch on the bug, of course route order has an impact, not sure why we didn't catch this with a route prefix though. I'll add a test later on, thanks!

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@soyuka soyuka merged commit 025f63e into api-platform:4.0 Sep 24, 2024
58 of 59 checks passed
@BurningDog BurningDog deleted the fix/route-registration-of-entrypointcontroller-should-be-last branch September 24, 2024 08:46
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.

3 participants