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

noContent responses still get status code 200 added #324

Closed
spaceemotion opened this issue Aug 14, 2020 · 7 comments · Fixed by #331
Closed

noContent responses still get status code 200 added #324

spaceemotion opened this issue Aug 14, 2020 · 7 comments · Fixed by #331
Labels
bug Something isn't working pending This issue is pending review

Comments

@spaceemotion
Copy link
Contributor

  • Laravel Version: 7.21.0
  • PHP Version: 7.4.3
  • Blueprint Version: 1.16.0
  • Platform: Linux (Ubuntu 20)

Issue:

(This might be intended behavior)

Generated controllers that return response()->noContent() seem to still get 200 as their status code added. Afaik noContent() should return 204 (the default).

The code even mentions it being excluded, but the status code seems to be set incorrectly?

return sprintf('return response()->noContent(%s);', $this->status() === 204 ? '' : $this->status());

draft.yaml:

controllers:
  User:
    resource: api

generates:

return response()->noContent(200);
@spaceemotion spaceemotion added bug Something isn't working pending This issue is pending review labels Aug 14, 2020
@devmsh
Copy link
Contributor

devmsh commented Aug 15, 2020

As mentioned here, it must return 204 if there is no response.

Waiting for @jasonmccreary decision and I can fix this if he agreed about the change.

@jasonmccreary
Copy link
Collaborator

I'd want to check this as I am confused on what outputs what. In general, there is nothing wrong with a response()->noContent(200).

However, to @devmsh's comment, there may be something better depending on the context (e.g. the destroy response)

@devmsh
Copy link
Contributor

devmsh commented Aug 15, 2020

Unfortunately, I did not get it.

Both 200 and 204 works, but 204 seems more standardized in the restful API world.

@jasonmccreary
Copy link
Collaborator

jasonmccreary commented Aug 15, 2020

Interesting. On running the draft file above, I get an error:

  Argument 1 passed to Blueprint\Generators\Statements\ResourceGenerator::visibleColumns() must be an instance of Blueprint\Models\Model, null given, called in /Users/jason/workspace/turbo-zonda/vendor/laravel-shift/blueprint/src/Generators/Statements/ResourceGenerator.php on line 105

@devmsh
Copy link
Contributor

devmsh commented Aug 16, 2020

Just test it with v1.16.0 in a fresh Laravel installation and it works fine.

Maybe because Laravel has a default user model already?

@spaceemotion
Copy link
Contributor Author

So I looked through the unit tests and the ControllerGeneratorTest has a few drafts that have the same behavior. The culprit seems to be this default configuration for api resource controllers:

'api.destroy' => [
'delete' => '[singular]',
'respond' => 200,
],

Changing it to 204 seems to do the trick.

@jasonmccreary
Copy link
Collaborator

@SpaceMotion or @devmsh, can you submit the PR to fix this. I'm still not fully understanding the issue, other than a less than desired response code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pending This issue is pending review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants