search

lazzard / php-ftp-client

:package: Provides helper classes and methods to manage FTP files in an OOP way.
MIT License
90 stars 18 forks source link

Implement isPassive method #39

Closed peter279k closed 1 year ago

peter279k commented 1 year ago

As title, implementing the getPassive method to get the current passive mode option value.

peter279k commented 1 year ago

My suggestion is to change the FtpClient\Connection\Connection::stream property to private to prevent changing the stream handle and make the property an internal detail of the abstract class only (FtpClient\Connection\Connection).

I think this solution is great and I will change the FtpClient\Connection\Connection::stream property to private later.

peter279k commented 1 year ago

After changing FtpClient\Connection\Connection::stream property to private, it seems that it will get the following error when running the test suites during the GitHub workflows running.

It presents the following error message:

PHPUnit [9](https://github.com/open-source-contributions/php-ftp-client/actions/runs/3583735387/jobs/6029629175#step:12:10).5.26 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.13
Configuration: /home/runner/work/php-ftp-client/php-ftp-client/phpunit.xml

EEEEEEEEEEEE......E.EEE......E.EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE 65 / 87 ( 74%)
EEEEEEEEEEEEEEEIEEEEEE                                            87 / 87 ([10](https://github.com/open-source-contributions/php-ftp-client/actions/runs/3583735387/jobs/6029629175#step:12:11)0%)

Time: 00:00.103, Memory: 8.00 MB

There were 72 errors:

1) Lazzard\FtpClient\Tests\Integration\Command\FtpCommandTest::testRaw
Lazzard\FtpClient\Exception\ConnectionException: [ConnectionException] - Invalid FTP connection, try to reopen the FTP connection.

/home/runner/work/php-ftp-client/php-ftp-client/src/Connection/Connection.php:80
/home/runner/work/php-ftp-client/php-ftp-client/src/FtpWrapper.php:81
/home/runner/work/php-ftp-client/php-ftp-client/src/Connection/Connection.php:151
/home/runner/work/php-ftp-client/php-ftp-client/src/Connection/Connection.php:[12](https://github.com/open-source-contributions/php-ftp-client/actions/runs/3583735387/jobs/6029629175#step:12:13)4
/home/runner/work/php-ftp-client/php-ftp-client/tests/integration/FtpConnectionFactory.php:23
/home/runner/work/php-ftp-client/php-ftp-client/tests/integration/Command/FtpCommandTest.php:[16](https://github.com/open-source-contributions/php-ftp-client/actions/runs/3583735387/jobs/6029629175#step:12:17)
......

Perhaps I also need to implement setStream method in the FtpClient\Connection\Connection class?

AmraniCh commented 1 year ago

I think the problem is with the subclasses that extend the abstract class which are the FtpClient\Connection\FtpConnection and FtpClient\Connection\FtpSSLConnection classes, they access the stream handle property directly, we have to use the getter method instead.

AmraniCh commented 1 year ago

As I see here in your changes, you add a new property to the subclasses whereas I meant changing the visibility of the stream property in the FtpClient\Connection\Connection class from protected to private, this is the problem that caused the tests to be failed, we have also as I've said access the property with the getter in the classes that extend the FtpClient\Connection\Connection class.

peter279k commented 1 year ago

I let the $stream be private property in the FtpClient\Connection\Connection class.

And implement the protected function setStream to let the subclass set the private $stream property.

Then using the setStream method in the subclass to set the private $stream property.

I think it cannot use the getStream directly because it always gets null stream.

Before using the getStream method, using the setStream to set the private $stream.

Making The setStream method protected I think it only let extended classes can call this method.

peter279k commented 1 year ago

I'm going to close the PR because it seems that we don't have the current way to resolve isPassive modifying issue.