-
Notifications
You must be signed in to change notification settings - Fork 85
Added support for x-forwarded-proto header to request object #134
Added support for x-forwarded-proto header to request object #134
Conversation
|
Relates to #123 |
68d63c7 to
02fc764
Compare
Xerkus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all, i think proper place to add support for the header is not here but at https://github.com/zendframework/zend-http/blob/release-2.7.0/src/PhpEnvironment/Request.php#L254
|
|
||
| use Zend\Uri\Http as Http; | ||
|
|
||
| class HttpsUri extends Http |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason to introduce this class.
$uri->setScheme('https'); provides exactly the same behavior
| } | ||
|
|
||
| $headers = $this->getHeaders()->toArray(); | ||
| if (array_key_exists('X-Forwarded-Proto', $headers) && $headers['X-Forwarded-Proto'] === 'https') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will end up overriding scheme each time Uri is fetched from Request. Getters must not have side effects
| */ | ||
| public function getScheme() | ||
| { | ||
| return 'https'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks Uri contract by ignoring scheme changes
Besides, internal logic will be broken in unpredictable ways where scheme property is used directly
| $this->uri = new HttpUri($this->uri); | ||
| } | ||
|
|
||
| $headers = $this->getHeaders()->toArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need all the headers. Use has() and get()
|
There are multiple ways of doing this. First, as suggested in #123, you could write a controller plugin for this. In that case, you can look not only at the scheme of the URI composed by the request, but also at the Second, you could create a listener that triggers at either low priority in the Third, we could directly support this via the All three of these are preferable to the approach you are presenting here. As @Xerkus notes, the changes you are introducing mean that users would be surprised if the scheme did not update. Additionally, it still requires the user to update their system to inject the alternate URI instance — and if you have to do that, one of the above approaches will be just as successful. I will consider an approach in this package that implements the third option I outline above, or an approach in the zend-mvc pacakge that implements the second option above. |
|
Closing due to inactivity. Re-submit if you are able to implement one of the options outlined in my last comment above. |
|
Just one addition to this. We use a nginx reverse proxy to terminate SSL and the X-Forwarded-Proto header is not available in our setup. Instead |
No description provided.