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

Fixed issue #7579 on the 2.7 branch #7766

Merged
merged 1 commit into from Dec 2, 2015
Merged

Fixed issue #7579 on the 2.7 branch #7766

merged 1 commit into from Dec 2, 2015

Conversation

wiserfirst
Copy link

Issue #7579 also exists on the 2.7 branch. On master branch, it was fixed by PR #7581. The same code also fix this issue for the 2.7 branch.

@dereuromark
Copy link
Member

Looks good to me.

@dereuromark dereuromark added this to the 2.7.8 milestone Dec 1, 2015
@wiserfirst
Copy link
Author

@dereuromark Thanks for the quick reply!

@@ -129,8 +129,12 @@ public function connect() {
$this->disconnect();
}

$hasProtocol = strpos($this->config['host'], '://') !== false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this using parse_url?

I.e.:

screenshot from 2015-12-01 15-03-38

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will have to ask @markstory .
I could think of performance reasons, even though it would be very minor - and only relevant for a lot of loop calls.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With parse_url the relevant code would look like this:

        $urlParts = parse_url($this->config['host']);
        $this->config['host'] = $urlParts['host'];
        if (isset($urlParts['scheme'])) {
            $this->config['protocol'] = $urlParts['scheme'];
        }

Maybe it is subject to taste, but to me this looks cleaner. Thanks for the reminder @AD7six

Update:

The code wouldn't work if the host doesn't contain the protocol part, for example $this->config['host'] = 'www.cakephp.org'. We need to check if 'host' exists in the parse result:

        $urlParts = parse_url($this->config['host']);
        if (isset($urlParts['host'])) {
            $this->config['host'] = $urlParts['host'];
        }
        if (isset($urlParts['scheme'])) {
            $this->config['protocol'] = $urlParts['scheme'];
        }

I'm not quite sure if parse_url is still a better solution considering this. Please give some advise, @dereuromark @AD7six . Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used string slicing because I didn't think of parse_url() when I wrote the code. It looks like parse_url() would be more complicated though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question answered 👍

The hopping from scheme to protocol to scheme is a bit awkward - but not worth worrying about.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I'll leave the code as it is now. 😈

@markstory markstory self-assigned this Dec 2, 2015
markstory added a commit that referenced this pull request Dec 2, 2015
Fixed issue incorrect SNI validation when host name contains protocol for SmtpTransport.
@markstory markstory merged commit 5852ece into cakephp:2.7 Dec 2, 2015
@wiserfirst wiserfirst deleted the split_protocol_from_host branch December 2, 2015 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants