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

Fix for the isDir function in the FTP Adapter #607

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Romain
Copy link

@Romain Romain commented Apr 30, 2019

The isDir function seems to be broken in the FTP Adapter.
When I try to retrieve a document through a stream, I get this error: ftp_chdir(): /documents/your-file-name.jpg: No such file or directory

Here is the complete trace:

in vendor/knplabs/gaufrette/src/Gaufrette/StreamWrapper.php->stat (line 210) 
in vendor/knplabs/gaufrette/src/Gaufrette/Stream/InMemoryBuffer.php->isDirectory (line 159) 
in vendor/knplabs/gaufrette/src/Gaufrette/Filesystem.php->isDirectory (line 333)
in vendor/knplabs/gaufrette/src/Gaufrette/Adapter/Ftp.php->isDir (line 222)
in vendor/knplabs/gaufrette/src/Gaufrette/Adapter/Ftp.phpftp_chdir (line 369)
in vendor/knplabs/gaufrette/src/Gaufrette/Adapter/Ftp.php (line 369)

Changing the way we perform this test by using the PHP function is_dir instead of the ftp_chdir one fixes this error.

Copy link
Author

@Romain Romain left a comment

Choose a reason for hiding this comment

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

I created a private method for the connection URL.
I also kept the chdir method as the main approach and fallback on is_dir with passive mode set to true if we catch and exception with chdir.

Copy link
Contributor

@nicolasmure nicolasmure left a comment

Choose a reason for hiding this comment

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

Thanks for the update ;)

src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
@Romain
Copy link
Author

Romain commented Jun 18, 2019

@nicolasmure I pushed a fix with your comments applied in it.

Copy link
Contributor

@nicolasmure nicolasmure left a comment

Choose a reason for hiding this comment

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

Can you also squash your commits please ? :)

src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
@Romain
Copy link
Author

Romain commented Jun 20, 2019

@nicolasmure Got it. Pushed a change accordingly to your comment.

Copy link
Contributor

@nicolasmure nicolasmure left a comment

Choose a reason for hiding this comment

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

Thanks for the changes :)

There are also some failing tests, do you mind to fix them ?

phpunit :

There were 2 failures:
1) Gaufrette\Functional\Adapter\FtpTest::shouldCheckIfFileExists
Failed asserting that true is false.
/home/travis/build/KnpLabs/Gaufrette/tests/Gaufrette/Functional/Adapter/FunctionalTestCase.php:91

2) Gaufrette\Functional\Adapter\FtpTest::shouldFailWhenTryMtimeForKeyWhichDoesNotExist
Failed asserting that 1561017417 is false.
/home/travis/build/KnpLabs/Gaufrette/tests/Gaufrette/Functional/Adapter/FunctionalTestCase.php:119

and also the phpspec for this adapter.

You can run the specs with

$ docker/run-task php7.2 vendor/bin/phpspec run spec/Gaufrette/Adapter/Ftp.php

and the integration tests with

$ docker/run-task php7.2 vendor/bin/phpunit tests/Gaufrette/Functional/Adapter/FtpTest.php

src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
@Romain
Copy link
Author

Romain commented Jun 21, 2019

Your comments have been integrated. These are nice improvements. ;)

Copy link
Contributor

@nicolasmure nicolasmure left a comment

Choose a reason for hiding this comment

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

Can you also update the tests accordingly please ?

src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
@Romain
Copy link
Author

Romain commented Jul 26, 2019

@nicolasmure I'm not sure how to update the tests since they're not checking the way we connect to the server.

src/Gaufrette/Adapter/Ftp.php Outdated Show resolved Hide resolved
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.

6 participants