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

[FTP Driver] Support for implicit SSL #785

Closed
vyuldashev opened this issue Apr 26, 2017 · 39 comments

Comments

@vyuldashev
Copy link

commented Apr 26, 2017

Hello,

current ftp implementation supports only explicit SSL and does not work with implicit SSL connection, which is by default uses 990 port.

Also, there is no support for implicit SSL in PHP. Research gave me only solution using curl.

I can make pull request to make implicit SSL connection work using curl. Or community can provide another options?

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2017

I think that's a great idea. We need to write a completely FTP adapter based on the CURL. It's worth a try. Perhaps with it we will have less problems and easier to support.

@vyuldashev

This comment has been minimized.

Copy link
Author

commented Apr 26, 2017

Should it be in current driver or something called like CurlFtpDriver?

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2017

By analogy with other adapters, I think it should be called CurlFtp. But I'm not project's owner. It's just my opinion.

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2017

And like the other adapters, it should extend of the AbstractAdapter.

@frankdejonge

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

The adapter should indeed be called CurlFtp or CurlFtpAdapter which is more inline with the external ones. In the long term all the adapters will follow this naming convention, but there's no real benefit in renaming them now.

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2017

+1 on CurlFtp, because other adapters do not have "Adapter" at the ends.

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2017

@frankdejonge Do you mean it should be an external package?

@frankdejonge

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

We can develop it here and then move it out to release it, because then we can specify the right deps here. While curl is not specific people have complained about the FTP adapter being released here because you can't express that you require the ftp extension for it.

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2017

Yea, perhaps, as an external adapter, he really would be better, but I think it is necessary that it was in the thephpleague repos. Then it will be have more trust and stable. :)

@vyuldashev

This comment has been minimized.

Copy link
Author

commented Apr 26, 2017

@frankdejonge I will make a pull request then.

@frankdejonge

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

@vladimir-yuldashev cool 👍

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2017

And if it's like an external adapter, it's better to call it the CurlFtpAdapter (as other exetrnal adapters)

@vyuldashev

This comment has been minimized.

Copy link
Author

commented Apr 27, 2017

So, I started to implement adapter. There are only two methods remained to implement: setVisibility and getTimestamp.

I request a feedback if I am on the right direction: https://github.com/vladimir-yuldashev/flysystem/blob/master/src/Adapter/CurlFtp.php

Also, there is no way to mock curl functions for unit testing. I can create Curl class which will be curl wrapper or include existing curl wrappers from composer or just ignore testing some methods.

Suggestions?

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

I not use CURL like FTP client - don't know. About mock of the CURL, check this http://stackoverflow.com/questions/7911535/how-to-unit-test-curl-call-in-php

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

What result in that code?

curl_setopt($this->curl, CURLOPT_CUSTOMREQUEST, 'LIST -aln '.$directory);
$result = curl_exec($this->curl);

I think, it should be identical to ftpRawList.

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

$this->createCurl(); should be in the connect() method.

@vyuldashev

This comment has been minimized.

Copy link
Author

commented Apr 27, 2017

@RubtsovAV

I not use CURL like FTP client - don't know. About mock of the CURL, check this

Thats exactly what I mean.

I think, it should be identical to ftpRawList.

Right.

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

@vladimir-yuldashev

Right.

Then there must be a timestamp. Try to uncommenting a time in the normalizeUnixObject(), and then you can use getMetadata() for the getTimestamp method.

@vyuldashev

This comment has been minimized.

Copy link
Author

commented Apr 27, 2017

@RubtsovAV I tried to uncomment, but it's not so simple. FTP returns something like "Aug 5 2016" or "26 April 23:12"

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

Hm... Let's try that

curl_setopt($this->curl, CURLOPT_CUSTOMREQUEST, 'MDTM '.$path);
$result = curl_exec($this->curl);
@vyuldashev

This comment has been minimized.

Copy link
Author

commented Apr 27, 2017

@RubtsovAV I tried that, it returns just directory listing.

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

The MDTM command must returns the last-modified time of the given file on the remote host in the format "YYYYMMDDhhmmss": YYYY is the four-digit year, MM is the month from 01 to 12, DD is the day of the month from 01 to 31, hh is the hour from 00 to 23, mm is the minute from 00 to 59, and ss is the second from 00 to 59.

Syntax: MDTM remote-filename

What ftp server do you use?

@vyuldashev

This comment has been minimized.

Copy link
Author

commented Apr 27, 2017

@RubtsovAV I tried several ftp servers and none of them returned timestamp.

You can try for example public ftp server: speedtest.tele2.net

@vyuldashev

This comment has been minimized.

Copy link
Author

commented Apr 28, 2017

I think I got getTimestamp work by tweaking normalizeUnixObject method. But I decided not to change parent because it may break other FTP drivers.

https://github.com/vladimir-yuldashev/flysystem/blob/master/src/Adapter/CurlFtp.php#L453

@vyuldashev

This comment has been minimized.

Copy link
Author

commented Apr 28, 2017

Alright, guys. Seems like I finished. All methods, including setVisibility work.

What next?

@frankdejonge Should I make a pull request now? Or should we make it an external adapter?

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2017

@vladimir-yuldashev
This should work better:

public function getTimestamp($path)
{
    curl_setopt($curl, CURLOPT_CUSTOMREQUEST, 'MDTM '.$path);

    $rawResponse = '';
    curl_setopt($curl, CURLOPT_HEADERFUNCTION, function($ch, $string) use (&$rawResponse) {
        $length = strlen($string);
        $rawResponse .= $string;
        return $length;
    });
    $result = curl_exec($curl);

    // reset the curl options for unset CURLOPT_HEADERFUNCTION
    curl_reset($curl);

    $lastRow = array_pop(array_filter(explode("\n", $rawResponse)));
    list($code, $time) = explode(' ', $lastRow);
    if ($code !== '213') {
        throw new RuntimeException("wrong response code");
    }

    sscanf($time, "%4u%2u%2u%2u%2u%2u", $year, $mon, $mday, $hour, $min, $sec);
    return mktime($hour, $min, $sec, $mon, $mday, $year);
}

But after that we need to reset the curl options for unset CURLOPT_HEADERFUNCTION. Therefore, you either need to reconnect, or make a method for setting the initial options. I think the second variant will be better.

@vyuldashev

This comment has been minimized.

Copy link
Author

commented Apr 28, 2017

@RubtsovAV curl_exec returns false

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2017

Yes, and so had to use CURLOPT_HEADERFUNCTION. As you can see, the timestamp is taken from $rawResponse, which parsed from the callback function.

@vyuldashev

This comment has been minimized.

Copy link
Author

commented Apr 28, 2017

@RubtsovAV
You rock! I made small optimisation.

Now driver is totally ready. Expecting reply from @frankdejonge about further steps.

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2017

@vladimir-yuldashev Yea! :) Thanks . But what about unit tests?

@vyuldashev

This comment has been minimized.

Copy link
Author

commented Apr 28, 2017

@RubtsovAV I am not sure if we can introduce Curl class into the main repository.

@frankdejonge

This comment has been minimized.

Copy link
Member

commented Apr 28, 2017

@vladimir-yuldashev as @RubtsovAV mentioned, we first need to have some tests in place to test the adapter. I think in this case integration tests make sense, perhaps a docker based thing?

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2017

@vladimir-yuldashev Will be wait @frankdejonge ...

@vladimir-yuldashev Sorry for the tediousness, but maybe you'll add to the connect method this:

if ($this->isConnected()) {
    $this->disconnect();
}

For clear the curl resource.

@frankdejonge

This comment has been minimized.

Copy link
Member

commented Apr 28, 2017

@RubtsovAV I don't understand your last comment.

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2017

@frankdejonge While I write my message, you already came :) I already have case integration tests based on the docker. You can see it there #720 . But I don't undestand that why him failing a checks. I wrote it for the Ftp adapter and I think it can be adapted for the CurlFtp also.

@vyuldashev

This comment has been minimized.

Copy link
Author

commented Apr 28, 2017

@frankdejonge Also, phpunit seems to be outdated in composer.json. Can I update that?

@vyuldashev

This comment has been minimized.

Copy link
Author

commented Apr 28, 2017

@frankdejonge I am not so proficient in docker, so I would like to introduce Curl class in League\Flysystem\Util namespace and use Curl object instead of curl functions. Will it suit?

@RubtsovAV

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2017

@vladimir-yuldashev Let's do it as an external adapter? I can help with that.

My skype: RubtcovAV

@vyuldashev

This comment has been minimized.

Copy link
Author

commented May 7, 2017

So, we are developed an external adapter with functional tests using docker.
It's already in composer and we just tagged v1.0.0.

Repository: https://github.com/vladimir-yuldashev/flysystem-curlftp

@vyuldashev vyuldashev closed this May 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.