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

[HttpKernel] Additional HTTP exceptions #5312

Closed
wants to merge 16 commits into from
Closed

[HttpKernel] Additional HTTP exceptions #5312

wants to merge 16 commits into from

Conversation

ramsey
Copy link
Contributor

@ramsey ramsey commented Aug 21, 2012

I wanted to continue using exceptions for many other types of HTTP 4xx and 5xx status codes, particularly because I like the notion that I can trap the exception based on the type, rather than inspecting HttpException to see what status code it contains. I've decided to contribute these back to the Symfony HttpKernel component, in case others find them useful.

  • Bug fix: no
  • Feature addition: yes
  • Backwards compatibility break: no
  • Symfony2 tests pass: yes
  • Fixes the following tickets: -
  • Todo: -
  • License of the code: MIT
  • Documentation PR: n/a

This pull request provides the following new HttpKernel Exceptions:

Exception Applicable HTTP Status Code
BadRequestHttpException 400 Bad Request
ConflictHttpException 409 Conflict
GoneHttpException 410 Gone
InternalServerErrorHttpException 500 Internal Server Error
LengthRequiredHttpException 411 Length Required
NotAcceptableHttpException 406 Not Acceptable
PreconditionFailedHttpException 412 Precondition Failed
PreconditionRequiredHttpException 428 Precondition Required
ServiceUnavailableHttpException 503 Service Unavailable
TooManyRequestsHttpException 429 Too Many Requests
UnauthorizedHttpException 401 Unauthorized
UnsupportedMediaTypeHttpException 415 Unsupported Media Type

All the tests have been placed in the FlattenExceptionTest, since that's where the previous status code and method tests for HttpException exceptions are located, but I can move them to a more logical location, if needed.

† These codes have been included from RFC 6585, Additional HTTP Status Codes.

Ben Ramsey and others added 15 commits August 21, 2012 17:04
* upstream/master: (201 commits)
  Update src/Symfony/Bundle/FrameworkBundle/composer.json
  small optimization
  fixes pre for var_dump with xdebug
  [FrameworkBundle] Allow to set null for the handler in NativeSessionStorage
  added a missing phpdoc param
  [HttpFoundation] fixed undefined offset for assoc arrays in HeaderBag
  [HttpFoundation] fix #5271 (duplicated header in JsonResponse)
  fixed typos in the UPGRADE file
  fixed typo in the UPGRADE file
  [Form] Reintroduced the option "invalid_message_parameters"
  [Form] Fixed support for preferred choices in "entity" type
  Typo in UPGRADE-2.1
  [EventDispatcher] Adding IteratorAggregate to GenericEvent
  fix CS into Finder
  0x02 -> \MongoBinData::BYTE_ARRAY
  Alter upgrade notes with changes to _form_is_choice_selected twig function
  MongoBinData constructor now require "type" parameter
  Revert "merged branch guilhermeblanco/patch-6 (PR #4456)"
  add format to exception message
  Adding more specific debug bar reset CSS.
  ...
@travisbot
Copy link

This pull request passes (merged d4e234f into 7a233bc).

@stof
Copy link
Member

stof commented Aug 21, 2012

I would remove the InternalServerError one, as you get a 500 for any non-http exception anyway

@stof
Copy link
Member

stof commented Aug 21, 2012

and please rebase your branch to get rid of the merge commit at the beginning of your PR

@ramsey
Copy link
Contributor Author

ramsey commented Oct 3, 2012

@stof Can you provide some pointers on how to get rid of the merge commit through a rebase? I generally avoid rebase, so doing this is fairly new to me. I realize that what created the commit was a non-fast-forward merge, so I just need to understand how to get it to apply merges cleanly without creating merge commits like that. Thanks!

@alexandresalome
Copy link

@ramsey

# suppose you are on branch-feature
# given origin is the symfony repository
git pull --rebase origin/master

# equivalent to
git fetch origin
git rebase origin/master

If you want to rebase your branch on 2.1:

git pull --rebase origin/2.1

@stof
Copy link
Member

stof commented Oct 4, 2012

@ramsey http://symfony.com/doc/current/contributing/code/patches.html#rework-your-patch for the documentation about the way to do it

@stof
Copy link
Member

stof commented Oct 13, 2012

@ramsey ping

@fabpot
Copy link
Member

fabpot commented Oct 29, 2012

I'm going to finish this PR, but #5862 should be merged first.

@ramsey
Copy link
Contributor Author

ramsey commented Oct 29, 2012

Sorry I've been sitting on this for so long. I haven't had time to figure out how to get rid of the merge commit, as @stof requested. That's the only outstanding issue here, as far as I know.

@fabpot
Copy link
Member

fabpot commented Oct 29, 2012

I can see another issue. For some HTTP codes, you can/should/must add some headers or some content (like for the 406 status code).

Don't worry about the merge commit or the rebasing, I will take care of that myself when merging the PR.

@ramsey
Copy link
Contributor Author

ramsey commented Oct 29, 2012

I don't know what header is required for the 406 status code. RFC 2616 doesn't specify one. It does say "the response should include an entity containing a list of available entity characteristics and locations."

In other exceptions, such as UnauthorizedHttpException, TooManyRequestsHttpException, and ServiceUnavailableException, I did provide the required headers.

On Monday, October 29, 2012 at 10:48 AM, Fabien Potencier wrote:

I can see another issue. For some HTTP codes, you can/should/must add some headers or some content (like for the 406 status code).
Don't worry about the merge commit or the rebasing, I will take care of that myself when merging the PR.


Reply to this email directly or view it on GitHub (#5312 (comment)).

@fabpot
Copy link
Member

fabpot commented Oct 29, 2012

merged, thanks.

@fabpot fabpot closed this in 27ee846 Oct 29, 2012
@ramsey
Copy link
Contributor Author

ramsey commented Oct 29, 2012

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants