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

Maintain 200 status on 404 #92

Merged
merged 1 commit into from
Aug 29, 2020

Conversation

anatolinicolae
Copy link
Contributor

@anatolinicolae anatolinicolae commented Nov 7, 2018

Maintain HTTP status as 200 when erroring on 404 when PHP could still find the resource.

A case happened today while using L5 Swagger's API Documentation which loads assets dynamically, building asset path and then routing via Laravel to the file itself.

Test case:

  • Create project
  • Add L5 Swagger and set it up as basic install steps describe
  • Visit /api/documentation and get 404 on styles and scripts

Unfortunately, since we have static files served first, we don't actually find the file therefore falling into a 404 status code. The current error setup brings allows us to still resolve in Laravel.

Since the assets are found after that, we still have a 404 status this time with the content. So it's basically not found, but found.

Erroring and maintaining 200 as status code allows us to leverage Laravel headers for the correct status, which is not forced to 200 if the content is found in the internal redirects, but it is set to 404 on an actual 404.

Signed-off-by: Anatoli Nicolae <[email protected]>
@fideloper
Copy link
Contributor

Hi!
To clarify - is there ever a time when a 200 is returned when it should be a 404 with this change?

@anatolinicolae
Copy link
Contributor Author

Should not, but will run some tests tomorrow. Since we're already throwing the 404 to Laravel, which sets the 404 status code, everything should be handled correctly.

@fideloper
Copy link
Contributor

Great! I think I agree also (haha) but would be curious about the results of a test against a missing static asset vs a swagger API call vs a regular laravel route.

@anatolinicolae
Copy link
Contributor Author

So far it looks like it's working correctly...
screenshot 2018-11-15 at 11 33 36
screenshot 2018-11-15 at 11 33 22
screenshot 2018-11-15 at 11 33 12
screenshot 2018-11-15 at 11 32 53

@MFlor
Copy link

MFlor commented Aug 28, 2020

I ran into the same issue today - is there a plan for merging this fix, so that we do not need to add this fix manually to new or updated vessel installations?

@fideloper
Copy link
Contributor

Wow, Nov 2018! I apologize for never merging this in after @anatolinicolae did the work of testing it out.

@fideloper fideloper merged commit a9a8f1b into shipping-docker:master Aug 29, 2020
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