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

Connection reuse refactoring to rid of globals #787

Merged
merged 1 commit into from
Sep 2, 2015

Conversation

dbalabka
Copy link
Contributor

Refactoring that was discussed in #786

@dbalabka dbalabka force-pushed the remove-global-and-earn-25$ branch 3 times, most recently from f0ae796 to afc933e Compare August 27, 2015 13:54
@dbalabka
Copy link
Contributor Author

rebased

@terrafrost
Copy link
Member

I'll try to take a look either this evening or within the next few days as time permits.

Thanks!

@terrafrost
Copy link
Member

So overall I think it's a good PR! Email me at terrafrost@php.net your PayPal address and I'll PayPal you the money!

@bantu
Copy link
Member

bantu commented Aug 30, 2015

@terrafrost Let's pay after this was merged?

*/
public function getResourceId()
{
return '{' . spl_object_hash($this) . '}';
Copy link
Member

Choose a reason for hiding this comment

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

Why the { } ?

Copy link
Member

Choose a reason for hiding this comment

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

From the line he added in _parse_path...

if (preg_match('/^{[a-z0-9]+}$/i', $host)) {

If he doesn't do the curly brackets than localhost would match the regex and it'd try to re-use a non-existant SSH2::$connections['localhost'] connection.

Here's how you'd do it with libssh2:

<?php
$ssh2 = ssh2_connect('domain.tld');
ssh2_auth_password($ssh2, 'user', 'pass');

$sftp = ssh2_sftp($ssh2);

$arr = scandir("ssh2.sftp://$sftp/home/user/public_html");

print_r($arr);

If you do echo "ssh2.sftp://$sftp/home/user/public_html" you get ssh2.sftp://Resource id #4/home/user/public_html.

idk I think {} works..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{} symbols that should not be in URL according to RFC. Same symbols Symfony2 use for place holders in URL routes. So it should safe us from any conflicts, because otherwise regexp will match all alphanumeric domains.

@terrafrost
Copy link
Member

@terrafrost Let's pay after this was merged?

That works!

@terrafrost
Copy link
Member

@bantu - since you're active in this ticket lol... is #749 ok to merge? It's ready and waiting!

@bantu
Copy link
Member

bantu commented Aug 30, 2015

This fixes #572 ?

@dbalabka
Copy link
Contributor Author

@bantu yes. It was completely removed

@dbalabka
Copy link
Contributor Author

@terrafrost please invest these money into project. I renounce the prize. Thanks you for your honesty.

@terrafrost
Copy link
Member

@terrafrost please invest these money into project. I renounce the prize. Thanks you for your honesty.

Will do - thanks again for the PR!!

@terrafrost
Copy link
Member

So I think this is good to merge! I can do it but maybe you'd rather do it @bantu ?

}

/**
* Return existed connection
Copy link
Member

Choose a reason for hiding this comment

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

existing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@bantu
Copy link
Member

bantu commented Aug 31, 2015

@terrafrost I'll take care of it as soon as the outstanding comments are addressed.

@dbalabka
Copy link
Contributor Author

dbalabka commented Sep 1, 2015

@bantu branch was rebased. Please check fixes according your comments

@bantu
Copy link
Member

bantu commented Sep 2, 2015

👍 Thanks a lot

bantu added a commit to bantu/phpseclib that referenced this pull request Sep 2, 2015
…-25$

Connection reuse refactoring to rid of globals

* torinaki/remove-global-and-earn-25$:
  Connection reuse refactoring to rid of globals
@bantu bantu merged commit 70dd67c into phpseclib:master Sep 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants