Fixed a bug where if url is http without explicit port and redirects …

[patrickmcsweeney]

…to https without explicit port we try to ssl connect on port 80

[k00ni]

Hi @patrickmcsweeney, thanks for the pull! Despite 2 of our tests currently fail, would you mind adding a test to show that the fix works?

[patrickmcsweeney]

Hi Konrad,

I have had a go and created a harness with a test in (on my local machine) but I dont really know how to meaningfully test this. The calls to global functions fsockopen and stream_socket_client make give you back socket which doesnt have a way to reliably get the URI. In an ideal world the static functions would be wrapped in a utility class which is passed in at construction but that is really significant change to make to give that the test suite is far from comprehensive. Do you have a suggestion for how to go about testing this.

An alternative would be to create a sub method (the current one is too long anyway really) called something like __urlPartsFromUrlAndPreviousUriParts and move the appropriate code into there. In an ideal world this method would be private but defeats the purpose which is to move the code out to be independently testable. That still doesnt give a way to test getHTTPSocket which will have changed a fair bit but its better than nothing.

Sorry to be difficult I am trying to help but ive spent a lot longer scratching my head about how to test this fix than I did working out how to implement the fix.

Kind regards Patrick

On Fri, Oct 19, 2018 at 1:45 PM Konrad Abicht notifications@github.com wrote:

Hi @patrickmcsweeney https://github.com/patrickmcsweeney, thanks for the pull! Despite 2 of our tests currently fail, would you mind adding a test to show that the fix works?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/semsol/arc2/pull/121#issuecomment-431350538, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQJw_LfyAp2oOQO7qkmf321m3bhQukeks5umclsgaJpZM4XwQPT .

-- To cope with the volume of email I get I rarely respond to email on the same day i receive it. If your message isn't actually urgent please be patient. If your message needs a same day response you can try contacting me on skype for business instant messenger. Please do not contact me by email about issues which you know should really be sent to serviceline@soton.ac.uk and then assigned to me as a ticket.

[k00ni]

Hi @patrickmcsweeney, you got fair points.

What do you think about a new function called getURIParts which mostly contains:

    $parts = parse_url($url);
    /* relative redirect */
    if (!isset($parts['port']) && $prev_parts && (!isset($parts['scheme']) || $parts['scheme'] == $prev_parts['scheme'])) {
      /* only set the port if the scheme has not changed. If the scheme changes to https assuming the port will stay as port 80 is a bad idea */
      $parts['port'] = $prev_parts['port'];
    }
    if (!isset($parts['scheme']) && $prev_parts) $parts['scheme'] = $prev_parts['scheme'];
    if (!isset($parts['host']) && $prev_parts) $parts['host'] = $prev_parts['host'];

    /* no scheme */
    if (!$this->v('scheme', '', $parts)) return $this->addError('Socket error: Missing URI scheme.');
    /* port tweaks */
    $parts['port'] = ($parts['scheme'] == 'https') ? $this->v1('port', 443, $parts) : $this->v1('port', 80, $parts);

and merges parts and prev parts. The return value could be a merged parts array.

This would allow us to test the parts-part separately.

Another idea: Could we create a system test which uses e.g. curl + a short test script to check your change?

Thanks for your time!

[patrickmcsweeney]

Ok I have re-factored and tested the new function. Its not perfect but better than no tests all. Avoided the system test using curl because i think it will be brittle compared unit testing.

[k00ni]

Awesome, thanks for your effort! I will wait with merging until next Monday, maybe @semsol want to comment here.