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

Modify detection of HTTPS #363

Merged
merged 4 commits into from Jul 10, 2019

Conversation

Projects
None yet
3 participants
@heiglandreas
Copy link
Member

commented Jul 9, 2019

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

  • Are you fixing a bug?
    • Detail how the bug is invoked currently.
    • Detail the original, incorrect behavior.
    • Detail the new, expected behavior.
    • Base your feature on the master branch, and submit against that branch.
    • Add a regression test that demonstrates the bug, and proves the fix.
    • Add a CHANGELOG.md entry for the fix.
Modify detection of HTTPS
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 #362
@webimpress
Copy link
Contributor

left a comment

@heiglandreas We need unit test to cover that change.

@heiglandreas heiglandreas force-pushed the heiglandreas:patch-1 branch from 18aca8e to 4263a48 Jul 9, 2019

@webimpress
Copy link
Contributor

left a 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)

This comment has been minimized.

Copy link
@webimpress

webimpress Jul 9, 2019

Contributor

2019 please

{
$server = [
"HTTPS" => $httpsValue,
"SERVER_NAME"=> "localhost",

This comment has been minimized.

Copy link
@webimpress

webimpress Jul 9, 2019

Contributor

please use single quotes and always one space before and after =>.

class MarshalUriFromSapiTest extends TestCase
{
/** @dataProvider returnsUrlWithCorrectHttpSchemeFromArraysProvider */

This comment has been minimized.

Copy link
@webimpress

webimpress Jul 9, 2019

Contributor

please use multiline PHPDoc for method doc-comment:

/**
 * @dataProvider returnsUrlWithCorrectHttpSchemeFromArraysProvider
 */
$server = [
'HTTPS' => $httpsValue,
'SERVER_NAME' => 'localhost',
'SERVER_PORT'=>'80',

This comment has been minimized.

Copy link
@webimpress

webimpress Jul 10, 2019

Contributor

missing spaces around =>

This comment has been minimized.

Copy link
@heiglandreas

heiglandreas Jul 10, 2019

Author Member

... 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.

This comment has been minimized.

Copy link
@webimpress

webimpress Jul 10, 2019

Contributor

@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.

This comment has been minimized.

Copy link
@heiglandreas

heiglandreas Jul 10, 2019

Author Member

@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...

@webimpress
Copy link
Contributor

left a comment

LGTM 👍

heiglandreas added some commits Jul 9, 2019

Add test to verify Scheme retrieval is correct
This adds the test and the CHANGELOG-entry for the changed method
Apply changes to comply with CodeReview
This modifies the copyright-date, adds some spaces before and after the
arrow, replaces double quotes with single quotes and modifies a function
docBlock.

@weierophinney weierophinney force-pushed the heiglandreas:patch-1 branch from d8ebccc to 1a3db8a Jul 10, 2019

@weierophinney weierophinney merged commit 1a3db8a into zendframework:master Jul 10, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

weierophinney added a commit that referenced this pull request Jul 10, 2019

@weierophinney

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Thanks, @heiglandreas!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.