Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Finalhandler production stacktrace fix #46

Closed
wants to merge 3 commits into from
Closed

Finalhandler production stacktrace fix #46

wants to merge 3 commits into from

Conversation

michaelmoussa
Copy link
Contributor

@weierophinney this addresses #45 and makes it possible to create the FinalHandler in a factory and worry about the original response later.

One thing I wasn't sure about was whether error details should be in the case of 'env' !== 'production' vs. 'env' === 'development'. Production is surely "NO" and development is surely "YES", but other environments like staging, qa, etc. may depend on the implementor. Perhaps a boolean display_error_details option would be better than env? If so, what would you do with the original env? Remove it completely? Or automatically set display_error_details to true if env != production and false otherwise for BC?

Details will only be displayed if the 'env' option IS set, and it is set
to something other than "production"
This is to allow consumers, particularly Expressive, to create a FinalHandler
with configuration settings prior to the $response being available, and
then inject the original response later. Otherwise, FinalHandler will
always produce a 404 for successful requests, because any response it
receives would differ from the original NULL response.
@michaelmoussa
Copy link
Contributor Author

Addendum is a tiny bonus to fix another FinalHandler glitch.

@nesl247
Copy link

nesl247 commented Mar 15, 2016

Just ran into this issue myself and then I remembered you mentioning this PR. This is a huge issue in my opinion, so hopefully it gets merged in soon.

@weierophinney weierophinney added this to the 1.2.0 milestone Mar 17, 2016
@weierophinney weierophinney self-assigned this Mar 17, 2016
@weierophinney
Copy link
Member

I'd do $env !== 'production', and the default will be production; that way you can override the value in local environments, but it defaults to "safe for production". I can make that change on merge.

@weierophinney
Copy link
Member

I can make that change on merge.

Never mind; that's how it already reads. 😄

@weierophinney
Copy link
Member

Marking as 1.2.0, as it introduces new functionality.

weierophinney added a commit that referenced this pull request Mar 17, 2016
…cktrace-fix

Finalhandler production stacktrace fix
weierophinney added a commit that referenced this pull request Mar 17, 2016
Patch #39 added the method `testInvokingWithExceptionInNonProductionModeIncludesPrevTraceInResponseBody`.
The test fails after adding the code from #46, as that code modifies
`FinalHandler` to never provide exception details unless in
non-production mode (which is the default). This patch updates the test
to set the final handler used in the test setup to set the environment
to development so that the expectations pass.
weierophinney added a commit that referenced this pull request Mar 17, 2016
weierophinney added a commit that referenced this pull request Mar 17, 2016
@weierophinney
Copy link
Member

Merged to develop for release with 1.2.0.

weierophinney added a commit that referenced this pull request Mar 17, 2016
@michaelmoussa michaelmoussa deleted the finalhandler-production-stacktrace-fix branch March 17, 2016 16:29
@michaelmoussa
Copy link
Contributor Author

👍 thanks!!

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

Successfully merging this pull request may close these issues.

None yet

3 participants