refactor: remove new request with context #1857
Conversation
|
Can we just deprecate the old symbol and explain in the docs that it is useless? No need to break existing code for this IMHO. |
|
Alright, |
|
|
||
| // ServeHTTP executes a PHP script according to the given context. | ||
| func ServeHTTP(responseWriter http.ResponseWriter, request *http.Request) error { | ||
| func ServeHTTP(responseWriter http.ResponseWriter, request *http.Request, opts ...RequestOption) error { |
There was a problem hiding this comment.
Why adding a new parameter instead of using the request context?
There was a problem hiding this comment.
This way we only need to expose 1 function instead of 2 and don't have to do the additional allocation via the context. It's more about clarity though, directly passing a request with options just leaves less room for errors.
|
Another problem with this approach is that it's not possible anymore to expose some public custom values through the context to the calling code. It's a common practice for logging, tracing etc, for instance. TBH, I'm not sure this change is worth it. |
|
Not yet sure if exposing values via context is even a good idea, so I might revive a version of this at some point. |
This PR removes
NewRequestWithContext(slight breaking change for library users).Instead of:
Just: