Modify detection of HTTPS #363
Conversation
This modifies the detection of HTTPS by checking whether the lower-case value of the HTTPS-key of the server variable equals 'on'. Before that checked whether the value did *not* match 'off' which meant that the nginx-default for non-HTTPS connections (which, according to the [documentation](http://nginx.org/en/docs/http/ngx_http_core_module.html#var_https) is an empty string) did in fact match and returned that the connection *is* encrypted. This fixes zendframework#362
There was a problem hiding this comment.
@heiglandreas It looks good now, just some minor formatting issue.
| <?php | ||
| /** | ||
| * @see https://github.com/zendframework/zend-diactoros for the canonical source repository | ||
| * @copyright Copyright (c) 2018 Zend Technologies USA Inc. (http://www.zend.com) |
| { | ||
| $server = [ | ||
| "HTTPS" => $httpsValue, | ||
| "SERVER_NAME"=> "localhost", |
There was a problem hiding this comment.
please use single quotes and always one space before and after =>.
|
|
||
| class MarshalUriFromSapiTest extends TestCase | ||
| { | ||
| /** @dataProvider returnsUrlWithCorrectHttpSchemeFromArraysProvider */ |
There was a problem hiding this comment.
please use multiline PHPDoc for method doc-comment:
/**
* @dataProvider returnsUrlWithCorrectHttpSchemeFromArraysProvider
*/| $server = [ | ||
| 'HTTPS' => $httpsValue, | ||
| 'SERVER_NAME' => 'localhost', | ||
| 'SERVER_PORT'=>'80', |
There was a problem hiding this comment.
... Thank you for noticing and investing the time into fixing a 10 month old bug. We really appreciate that. It would be easier for us to include the patch if we had a test that would catch a possible regression. So would you mind investing more time into adding one? And while you're at it, would you mind replacing the double quotes with single quotes and making sure that there is always one space around the '=>'? Thanks.
... Thanks for adding a test and for fixing the CodingStyle as well. I'Ve noticed one line that seems to have slipped your attention but don't worry, I'll fix that when I merge the PR.
Would we not actually need this PR merged ASAP for a project I would have stopped contributing after the first reply. And I can increasingly understand why people are moving – not away from ZF but – towards other frameworks.
As maintainers we need to have a more welcome and thanking attitude even towards other maintainers. Yes. I know it takes time that we as maintainers put into it in out free time but that is not an excuse. Code is not everything.
There was a problem hiding this comment.
@heiglandreas thanks for your effort, finding the bug and preparing the PR. I really appreciate it. I am not the maintainer of the repository, I am just reviewing to get the PR ready for the maintainer to merge ASAP. You don't need to fix these changes, as everything was minor, and as you noted could be fixed on merge.
Sorry that my PR was short, just noted whatever was missing, without all "thankful" messages. I am concrete, that's it. Sorry.
There was a problem hiding this comment.
@webimpress I can live with that. But between all concretenes it will scare new contributors away. So if we as contributors want to attract more people to help out and fix stuff we need to make them feel more welcome. Otherwise they will go somewhere else. And tell others that it's not a good experience to contribute here...
This adds the test and the CHANGELOG-entry for the changed method
This modifies the copyright-date, adds some spaces before and after the arrow, replaces double quotes with single quotes and modifies a function docBlock.
|
Thanks, @heiglandreas! |
This modifies the detection of HTTPS by checking whether the lower-case value of the HTTPS-key of the server variable equals 'on'. Before that checked whether the value did not match 'off' which meant that the nginx-default for non-HTTPS connections (which, according to the documentation is an empty string) did in fact match and returned that the connection is encrypted.
This fixes #362
Provide a narrative description of what you are trying to accomplish: See #362
masterbranch, and submit against that branch.CHANGELOG.mdentry for the fix.