Add 443 as SERVER_PORT(default) when request is https #2323

juanpagfe · 2017-10-31T22:12:11Z

Hi. I solved the #2318 issue. Just added a validation wich changes the default port when request is HTTPS to 443.

coveralls · 2017-10-31T22:30:39Z

Coverage increased (+0.004%) to 93.227% when pulling 706d94f on juanpagfe:mock-https-request into 403b798 on slimphp:3.x.

coveralls · 2017-10-31T23:26:06Z

Coverage increased (+0.01%) to 93.235% when pulling ce3eb14 on juanpagfe:mock-https-request into 403b798 on slimphp:3.x.

coveralls · 2017-10-31T23:50:09Z

Coverage increased (+0.01%) to 93.235% when pulling 63ba688 on juanpagfe:mock-https-request into 403b798 on slimphp:3.x.

akrabat

Some changes required as noted.

Also, please supply a unit test so that we don't regress your fix.

akrabat · 2017-11-01T08:53:07Z

Slim/Http/Environment.php

+        //Validates if default protocol is HTTPS to set default port 443
+        $defport = (isset($userData["HTTPS"]) ||
+                    (isset($userData["SERVER_PROTOCOL"]) && substr($userData["SERVER_PROTOCOL"], 0, 5) === "HTTPS"))
+                      ? 443 : 80;


Please use an if statement.

akrabat · 2017-11-01T08:54:43Z

Slim/Http/Environment.php

@@ -29,14 +29,19 @@ class Environment extends Collection implements EnvironmentInterface
     */
    public static function mock(array $userData = [])
    {
+        //Validates if default protocol is HTTPS to set default port 443
+        $defport = (isset($userData["HTTPS"]) ||


["HTTPS"] may contain the string off, which should use port 80.

akrabat · 2017-11-01T08:56:04Z

Slim/Http/Environment.php

@@ -29,14 +29,19 @@ class Environment extends Collection implements EnvironmentInterface
     */
    public static function mock(array $userData = [])
    {
+        //Validates if default protocol is HTTPS to set default port 443
+        $defport = (isset($userData["HTTPS"]) ||
+                    (isset($userData["SERVER_PROTOCOL"]) && substr($userData["SERVER_PROTOCOL"], 0, 5) === "HTTPS"))


[SERVER_PROTOCOL] will contain HTTP/1.0, HTTP/1.1 or HTTP/2. It will never contain HTTPS and so please do not check this key.

akrabat · 2017-11-01T08:58:43Z

Slim/Http/Environment.php

@@ -29,14 +29,19 @@ class Environment extends Collection implements EnvironmentInterface
     */
    public static function mock(array $userData = [])
    {
+        //Validates if default protocol is HTTPS to set default port 443
+        $defport = (isset($userData["HTTPS"]) ||


Use single quoted strings if there's no variable interpolation.

mahagr · 2017-11-02T08:02:40Z

Why not use REQUEST_SCHEME which is either http or https?

coveralls · 2017-11-02T15:03:49Z

Coverage increased (+0.02%) to 93.243% when pulling 4538ba0 on juanpagfe:mock-https-request into 403b798 on slimphp:3.x.

akrabat · 2017-11-02T15:05:46Z

Why not use REQUEST_SCHEME which is either http or https?

No reason that it also couldn't be checked. I hadn't heard of it, so didn't mention it.

akrabat · 2017-11-02T15:07:35Z

Slim/Http/Environment.php

        $data = array_merge([
            'SERVER_PROTOCOL'      => 'HTTP/1.1',
            'REQUEST_METHOD'       => 'GET',
+            'REQUEST_SCHEME'       => 'http',


If you are going to put REQUEST_SCHEME here, then it needs to be either http or https …

Similarly, if you're going to support it here, you should test for it in $userData for setting the port.

coveralls · 2017-11-02T15:12:12Z

Coverage increased (+0.02%) to 93.243% when pulling 0957d80 on juanpagfe:mock-https-request into 403b798 on slimphp:3.x.

coveralls · 2017-11-02T15:51:51Z

Coverage increased (+0.02%) to 93.243% when pulling 59d4e73 on juanpagfe:mock-https-request into 403b798 on slimphp:3.x.

coveralls · 2017-11-02T16:20:30Z

Coverage increased (+0.03%) to 93.255% when pulling a3b12b4 on juanpagfe:mock-https-request into 403b798 on slimphp:3.x.

akrabat · 2017-11-03T17:01:40Z

phpunit.xml.dist

@@ -12,7 +12,7 @@
         convertNoticesToExceptions="true"
         convertWarningsToExceptions="true"
         processIsolation="false"
-         stopOnFailure="false"
+         stopOnFailure="true"


Please remove this change.

coveralls · 2017-11-03T17:37:48Z

Coverage increased (+0.03%) to 93.255% when pulling a7802a9 on juanpagfe:mock-https-request into 403b798 on slimphp:3.x.

akrabat · 2017-11-03T18:19:06Z

Thank you @juanpagfe :)

akrabat · 2017-11-04T08:26:10Z

@juanpagfe Fancy creating a similar PR for https://github.com/slimphp/Slim-Http ?

mahagr · 2017-11-06T06:35:41Z

@akrabat Any change making the same change into Uri class:

https://github.com/slimphp/Slim/blob/3.x/Slim/Http/Uri.php#L169-L170

Mostly because of HTTPS isn't always available.

akrabat · 2017-11-06T07:43:48Z

@mahagr Which web server doesn’t set HTTPS when the connection is over TLS?

mahagr · 2017-11-06T08:32:44Z

See discussion in:

getgrav/grav@a9c8271
getgrav/grav#1698

juanpagfe · 2017-11-06T11:18:48Z

@akrabat Of course, I'll create a PR in a minute.

Add 443 as SERVER_PORT(default) when request is https

706d94f

Code style fixed

ce3eb14

Code style fixed

63ba688

akrabat requested changes Nov 1, 2017

View reviewed changes

Code fixed due recommendations. Test added.

4538ba0

akrabat reviewed Nov 2, 2017

View reviewed changes

Code style fixed

0957d80

Code style fixed

59d4e73

REQUEST_SCHEME validation.

a3b12b4

akrabat requested changes Nov 3, 2017

View reviewed changes

Update phpunit.xml.dist

a7802a9

akrabat approved these changes Nov 3, 2017

View reviewed changes

akrabat added this to the 3.9.0 milestone Nov 3, 2017

akrabat merged commit a7802a9 into slimphp:3.x Nov 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 443 as SERVER_PORT(default) when request is https #2323

Add 443 as SERVER_PORT(default) when request is https #2323

juanpagfe commented Oct 31, 2017

coveralls commented Oct 31, 2017

coveralls commented Oct 31, 2017

coveralls commented Oct 31, 2017

akrabat left a comment

akrabat Nov 1, 2017

akrabat Nov 1, 2017

akrabat Nov 1, 2017

akrabat Nov 1, 2017

mahagr commented Nov 2, 2017

coveralls commented Nov 2, 2017

akrabat commented Nov 2, 2017

akrabat Nov 2, 2017 •

edited

Loading

coveralls commented Nov 2, 2017

coveralls commented Nov 2, 2017

coveralls commented Nov 2, 2017

akrabat Nov 3, 2017

coveralls commented Nov 3, 2017

akrabat commented Nov 3, 2017

akrabat commented Nov 4, 2017

mahagr commented Nov 6, 2017

akrabat commented Nov 6, 2017

mahagr commented Nov 6, 2017 •

edited

Loading

juanpagfe commented Nov 6, 2017

Add 443 as SERVER_PORT(default) when request is https #2323

Add 443 as SERVER_PORT(default) when request is https #2323

Conversation

juanpagfe commented Oct 31, 2017

coveralls commented Oct 31, 2017

coveralls commented Oct 31, 2017

coveralls commented Oct 31, 2017

akrabat left a comment

Choose a reason for hiding this comment

akrabat Nov 1, 2017

Choose a reason for hiding this comment

akrabat Nov 1, 2017

Choose a reason for hiding this comment

akrabat Nov 1, 2017

Choose a reason for hiding this comment

akrabat Nov 1, 2017

Choose a reason for hiding this comment

mahagr commented Nov 2, 2017

coveralls commented Nov 2, 2017

akrabat commented Nov 2, 2017

akrabat Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Nov 2, 2017

coveralls commented Nov 2, 2017

coveralls commented Nov 2, 2017

akrabat Nov 3, 2017

Choose a reason for hiding this comment

coveralls commented Nov 3, 2017

akrabat commented Nov 3, 2017

akrabat commented Nov 4, 2017

mahagr commented Nov 6, 2017

akrabat commented Nov 6, 2017

mahagr commented Nov 6, 2017 • edited Loading

juanpagfe commented Nov 6, 2017

akrabat Nov 2, 2017 •

edited

Loading

mahagr commented Nov 6, 2017 •

edited

Loading