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 absolute path for sftp sync #141

Merged
merged 4 commits into from May 4, 2018

Conversation

Projects
None yet
2 participants
@vitalybaev
Copy link
Contributor

vitalybaev commented May 1, 2018

I've realized, that if I write absolute path with SFTP sync, it doesn't work.
Problem appears because Sftp->getRemoteDirectoryList method exploding path with leading / to array with empty string as first element, causing error at phpseclib\Net\SFTP line 711.
Also I've added filtering empty parts of path since phpseclib causing error when it happens

@sebastianfeldmann sebastianfeldmann merged commit a73f052 into sebastianfeldmann:master May 4, 2018

2 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sebastianfeldmann

This comment has been minimized.

Copy link
Owner

sebastianfeldmann commented May 4, 2018

Hi Vitaly,
sorry I was out with food poisoning the last 2 days.

Great jobs once again.

@sebastianfeldmann

This comment has been minimized.

Copy link
Owner

sebastianfeldmann commented May 4, 2018

Is there a specific reason, why you changed the visibility to public?
Or just for the test? :)

@sebastianfeldmann sebastianfeldmann self-assigned this May 4, 2018

@sebastianfeldmann sebastianfeldmann added this to the 5.1.0 milestone May 4, 2018

@vitalybaev

This comment has been minimized.

Copy link
Contributor Author

vitalybaev commented May 4, 2018

Oh, hope you're now feel good!

Yes, it's only for regression tests, is it ok?

@sebastianfeldmann

This comment has been minimized.

Copy link
Owner

sebastianfeldmann commented May 4, 2018

Thanks, feeling much better now!

Yes it is ok, since I don't inject the Sftp (PHPSecLib) instance it is hard to mock/test otherwise :)

@vitalybaev

This comment has been minimized.

Copy link
Contributor Author

vitalybaev commented May 4, 2018

Ok, I saw an issue about integration tests (with Docker for example) when we could really test backups, sync and other, but I'm not sure it worth the effort for now.

@sebastianfeldmann

This comment has been minimized.

Copy link
Owner

sebastianfeldmann commented May 4, 2018

I'm not sure either, further more I'm not really a "Docker Expert" nor even a regular "Docker User" at all. So this is a thing I'm ignoring for quite some time now ;)

@vitalybaev

This comment has been minimized.

Copy link
Contributor Author

vitalybaev commented May 4, 2018

I use Docker for integration tests at my prod projects (but only with GitLab CI) and it's not so difficult.
So, in the future we could set it up!

Unit testing does big job here and integration tests could help to find bugs only with real execution (like my pull request about wrong argument for mongodump).

@sebastianfeldmann

This comment has been minimized.

Copy link
Owner

sebastianfeldmann commented May 4, 2018

You are absolutely right :)

@sebastianfeldmann

This comment has been minimized.

Copy link
Owner

sebastianfeldmann commented May 4, 2018

I made some adjustments to SftpTest and managed to make Sftp::getRemoteDirectoryList private again.

I accomplished this by mocking the PHPSecLib Sftp creation. For this to work I had to make the Sftp::login method protected. I prefer this way because so we don't grant public access to internal business logic.

And as the say "Once it's public you gonna maintain it for the rest of your life" ;)

Pushed to master some minutes ago.

@vitalybaev

This comment has been minimized.

Copy link
Contributor Author

vitalybaev commented May 7, 2018

Hi, sorry about late answer. You are absolutely right!

@sebastianfeldmann

This comment has been minimized.

Copy link
Owner

sebastianfeldmann commented May 7, 2018

Don't worry about it, hope you are fine.

Sadly this is leading to some conflicts in your WIP pull request. But I think you can resolve those rather quickly.

@vitalybaev

This comment has been minimized.

Copy link
Contributor Author

vitalybaev commented May 7, 2018

Sure, I gonna sync my master with upstream and merge it into my feature branch, is it ok?

@sebastianfeldmann

This comment has been minimized.

Copy link
Owner

sebastianfeldmann commented May 7, 2018

Off course

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.