Skip to content

Modify FTP adapter to manage long paths #430

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions composer.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "knplabs/gaufrette",
"name": "FaustineLarmagna/gaufrette",
Copy link
Contributor

Choose a reason for hiding this comment

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

This must not be part of the branch you use to submit the pull -request. Open the PR using a feature branch containing the wanted changes but not the package renaming

"type": "library",
"description": "PHP5 library that provides a filesystem abstraction layer",
"description": "Fork of knplabs/gaufrette library: PHP5 library that provides a filesystem abstraction layer",
"keywords": ["file", "filesystem", "media", "abstraction"],
"minimum-stability": "dev",
"homepage": "http://knplabs.com",
Expand All @@ -14,6 +14,10 @@
{
"name": "The contributors",
"homepage": "http://github.com/knplabs/Gaufrette/contributors"
},
{
"name": "Faustine Larmagna",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be removed. Contributors are not all listed individually here (it would be unmaintainable). This is why the previous author is linking to the list of contributors on github

Copy link
Author

Choose a reason for hiding this comment

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

Yes. I wanted to see your comments on the code before bringing changes to this.

"homepage": "http://github.com/FausineLarmagna"
}
],
"repositories": [
Expand Down
175 changes: 136 additions & 39 deletions src/Gaufrette/Adapter/Ftp.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
*
* @package Gaufrette
* @author Antoine Hérault <[email protected]>
* @author <[email protected]>
*/
class Ftp implements Adapter,
FileFactory,
Expand Down Expand Up @@ -54,93 +55,122 @@ public function __construct($directory, $host, $options = array())
}

/**
* {@inheritDoc}
* @param string $key
* @return bool|string
Copy link
Contributor

Choose a reason for hiding this comment

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

should be reverted

*/
public function read($key)
{
$this->ensureDirectoryExists($this->directory, $this->create);
//change to the directory that contain the file
$this->moveToTargetDirectory($key);
$file = basename($key);
Copy link
Contributor

Choose a reason for hiding this comment

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

not using $this->computePath($key) anywhere anymore looks weird to me

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, this code is from early this year. I wanted to know if you would change the feature completely before updating it. I will make the changes on another branch.
Thanks for the review.


$temp = fopen('php://temp', 'r+');

if (!ftp_fget($this->getConnection(), $temp, $this->computePath($key), $this->mode)) {
if (!ftp_fget($this->getConnection(), $temp, $file, $this->mode)) {
//change back to ftp root directory
$this->changeDirectory($this->directory);
return false;
}

rewind($temp);
$contents = stream_get_contents($temp);
//change back to ftp root directory
$this->changeDirectory($this->directory);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not put this in the middle of the code retrieving the content from $temp, for readability

fclose($temp);

return $contents;
}

/**
* {@inheritDoc}
* @param string $key
* @param string $content
* @return bool|int
*/
public function write($key, $content)
{
$this->ensureDirectoryExists($this->directory, $this->create);

$path = $this->computePath($key);
$directory = dirname($path);

$this->ensureDirectoryExists($directory, true);
//change to the directory that will contain the written file
$this->moveToTargetDirectory($key);
$file = basename($key);

$temp = fopen('php://temp', 'r+');
$size = fwrite($temp, $content);
rewind($temp);

if (!ftp_fput($this->getConnection(), $path, $temp, $this->mode)) {
if (!ftp_fput($this->getConnection(), $file, $temp, $this->mode)) {
//change back to ftp root directory
$this->changeDirectory($this->directory);
fclose($temp);

return false;
}

//change back to ftp root directory
$this->changeDirectory($this->directory);
fclose($temp);

return $size;
}

/**
* {@inheritDoc}
* @param string $sourceKey
* @param string $targetKey
* @return bool
*/
public function rename($sourceKey, $targetKey)
{
$this->ensureDirectoryExists($this->directory, $this->create);

$sourcePath = $this->computePath($sourceKey);
$targetPath = $this->computePath($targetKey);
$targetDirectory = dirname($targetPath);

$this->ensureDirectoryExists($targetDirectory, true);

$this->ensureDirectoryExists(dirname($targetPath), true);
if (dirname($sourcePath) == $targetDirectory) {
//change to the directory that contains the file to rename
$this->changeDirectory($targetDirectory);

return ftp_rename($this->getConnection(), $sourcePath, $targetPath);
$result = ftp_rename($this->getConnection(), basename($sourceKey), basename($targetKey));

//change back to ftp root directory
$this->changeDirectory($this->directory);
} else {
// don't change current ftp directory. May fail if paths are too long
$result = ftp_rename($this->getConnection(), $sourcePath, $targetPath);
}

return $result;
}

/**
* {@inheritDoc}
* @param string $key
* @return bool
*/
public function exists($key)
{
$this->ensureDirectoryExists($this->directory, $this->create);
//change to the directory that contain the source file
$this->moveToTargetDirectory($key);

$file = $this->computePath($key);
$lines = ftp_rawlist($this->getConnection(), '-al ' . dirname($file));
$lines = ftp_rawlist($this->getConnection(), '-al ' . '.');

if (false === $lines) {
return false;
}

$pattern = '{(?<!->) '.preg_quote(basename($file)).'( -> |$)}m';
$pattern = '{(?<!->) '.preg_quote(basename($key)).'( -> |$)}m';
foreach ($lines as $line) {
if (preg_match($pattern, $line)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this matching is broken now, as $line will not contain the full path anymore

//change back to ftp root directory
$this->changeDirectory($this->directory);
return true;
}
}

//change back to ftp root directory
$this->changeDirectory($this->directory);
return false;
}

/**
* {@inheritDoc}
* @return mixed
*/
public function keys()
{
Expand All @@ -152,7 +182,8 @@ public function keys()
}

/**
* {@inheritDoc}
* @param string $prefix
* @return array
*/
public function listKeys($prefix = '')
{
Expand Down Expand Up @@ -181,13 +212,20 @@ public function listKeys($prefix = '')
}

/**
* {@inheritDoc}
* @param string $key
* @return int
* @throws \RuntimeException
*/
public function mtime($key)
{
$this->ensureDirectoryExists($this->directory, $this->create);
//change directory to avoid truncation of too long paths
$this->moveToTargetDirectory($key);
$file = basename($key);

$mtime = ftp_mdtm($this->getConnection(), $file);

$mtime = ftp_mdtm($this->getConnection(), $this->computePath($key));
//change back to ftp root directory
$this->changeDirectory($this->directory);

// the server does not support this function
if (-1 === $mtime) {
Expand All @@ -198,21 +236,30 @@ public function mtime($key)
}

/**
* {@inheritDoc}
* @param string $key
* @return bool
*/
public function delete($key)
{
$this->ensureDirectoryExists($this->directory, $this->create);
//change to directory containing the $key to delete
$this->moveToTargetDirectory($key);
$keyName = basename($key);

if ($this->isDirectory($key)) {
return ftp_rmdir($this->getConnection(), $this->computePath($key));
$result = ftp_rmdir($this->getConnection(), $keyName);
} else {
$result = ftp_delete($this->getConnection(), $keyName);
}

return ftp_delete($this->getConnection(), $this->computePath($key));
//move back to ftp root directory
$this->changeDirectory($this->directory);

return $result;
}

/**
* {@inheritDoc}
* @param string $key
* @return bool
*/
public function isDirectory($key)
{
Expand Down Expand Up @@ -263,13 +310,15 @@ public function listDirectory($directory = '')
$this->fileData = array_merge($fileData, $this->fileData);

return array(
'keys' => array_keys($fileData),
'dirs' => $dirs
'keys' => array_keys($fileData),
'dirs' => $dirs
);
}

/**
* {@inheritDoc}
* @param string $key
* @param Filesystem $filesystem
* @return File
*/
public function createFile($key, Filesystem $filesystem)
{
Expand Down Expand Up @@ -342,6 +391,8 @@ protected function createDirectory($directory)
*/
private function isDir($directory)
{
$currentPath = ftp_pwd($this->getConnection());

if ('/' === $directory) {
return true;
}
Expand All @@ -350,12 +401,17 @@ private function isDir($directory)
return false;
}

// change directory again to return in the base directory
ftp_chdir($this->getConnection(), $this->directory);
// change directory again to return to where we were
ftp_chdir($this->getConnection(), $currentPath);

return true;
}

/**
* @param string $directory
* @param bool|true $onlyKeys
* @return array
*/
private function fetchKeys($directory = '', $onlyKeys = true)
{
$directory = preg_replace('/^[\/]*([^\/].*)$/', '/$1', $directory);
Expand Down Expand Up @@ -458,7 +514,8 @@ private function parseRawlist(array $rawlist)
/**
* Computes the path for the given key
*
* @param string $key
* @param $key
* @return string
*/
private function computePath($key)
{
Expand Down Expand Up @@ -502,7 +559,7 @@ private function connect()
$this->connection = ftp_connect($this->host, $this->port);
} else {
if(function_exists('ftp_ssl_connect')) {
$this->connection = ftp_ssl_connect($this->host, $this->port);
$this->connection = ftp_ssl_connect($this->host, $this->port);
} else {
throw new \RuntimeException('This Server Has No SSL-FTP Available.');
}
Expand Down Expand Up @@ -553,8 +610,48 @@ private function close()
}
}

/**
* @param $info
* @return bool
*/
private function isLinuxListing($info)
{
return count($info) >= 9;
}
}

/**
* Change current directory to the specified directory
*
* @param string $directory
* @throws \RuntimeException
* @return bool
*/
protected function changeDirectory($directory)
Copy link
Contributor

Choose a reason for hiding this comment

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

should be private

{
$moved = ftp_chdir($this->getConnection(), $directory);

if (false === $moved) {
throw new \RuntimeException(sprintf('Could not move to the \'%s\' directory.', $directory));
}
}

/**
* Change current directory to the directory of
* the $key file path.
* This allows to avoid truncation of too long file paths
* or failure of ftp php functions due to too long file paths
*
* @param string $key
*/
protected function moveToTargetDirectory($key)
Copy link
Contributor

Choose a reason for hiding this comment

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

should also be private

{
$this->ensureDirectoryExists($this->directory, $this->create);

$path = $this->computePath($key);
$directory = dirname($path);

$this->ensureDirectoryExists($directory, true);

$this->changeDirectory($directory);
}
}