search

LaravelCollective / remote

Remote SSH Access for the Laravel Framework
MIT License
276 stars 106 forks source link

Added timeout #2 #18

Closed acasar closed 9 years ago

acasar commented 9 years ago

This fixes BC problem with https://github.com/LaravelCollective/remote/pull/17 (original issue: https://github.com/LaravelCollective/remote/issues/16) so that it works even if the timeout config is not defined. I successfully tested the implementation on a real project and I also cleaned up the original PR to remove all unnecessary changes.

Can this be merged?

tshafer commented 9 years ago

Im going to review all pr’s and issues tonight for all of collective reps.

—  Tom Shafer

From: Anže Časar notifications@github.com Reply: LaravelCollective/remote reply@reply.github.com Date: November 23, 2015 at 8:36:09 AM To: LaravelCollective/remote remote@noreply.github.com Subject:  [remote] Added timeout #2 (#18)

This fixes BC problem with #17 (original issue: #16) so that it works even if the timeout config is not defined. I successfully tested the implementation on a real project and I also cleaned up the original PR to remove all unnecessary changes.

Can this be merged?

You can view, comment on, or merge this pull request online at:

  https://github.com/LaravelCollective/remote/pull/18

Commit Summary

Implemented timeout File Changes

M config/remote.php (1) M src/Connection.php (7) M src/RemoteManager.php (4) M src/SecLibGateway.php (13) M tests/RemoteSecLibTest.php (2) Patch Links:

https://github.com/LaravelCollective/remote/pull/18.patch https://github.com/LaravelCollective/remote/pull/18.diff — Reply to this email directly or view it on GitHub.

ikari7789 commented 9 years ago

@acasar I'm failing to see how this is any different from my implementation? What did you add?

ikari7789 commented 9 years ago

@acasar I saw the difference finally. I added the same check to mine along with checks for host and username as well. I don't think there is a need for two pull requests.

acasar commented 9 years ago

@ikari7789 I detailed the BC problem in the original issue https://github.com/LaravelCollective/remote/issues/16 but I presumed you didn't have time to fix it since there was no reply. So I made the change myself and also cleaned up some unnecessary changes in docblocks along the way.

Before I close this PR, it seems there are still some CS issues in your updated PR? I don't mind either way, as long as this issue is fixed :)

ikari7789 commented 9 years ago

Yeah, I'll fix it up tomorrow at work. Sorry for the lack of reply.

Sent from my iPhone

On Nov 24, 2015, at 19:13, Anže Časar notifications@github.com wrote:

@ikari7789 I detailed the BC problem in the original issue #16 but I presumed you didn't have time to fix it since there was no reply. So I made the change myself and also cleaned up some unnecessary changes in docblocks along the way.

Before I close this PR, it seems there are still some CS issues in your updated PR? I don't mind either way, as long as this issue is fixed :)

— Reply to this email directly or view it on GitHub.

ikari7789 commented 9 years ago

I fixed the StyleCI warnings and removed the /\ @var Connection $connection **/ comments. The other docblock changes are to reflect the new version of PHPseclib and I don't really feel like being bothered to make a new pull request just to update the docblocks (when I'm the one who should have updated them in the first place with my pull request that updated PHPseclib).

acasar commented 9 years ago

@ikari7789 Thanks. Hope this get merged soon :)