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

Modify FTP adapter to manage long paths #430

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

Modify FTP adapter to manage long paths #430

wants to merge 6 commits into from

Conversation

@FaustineLarmagna
Copy link

@FaustineLarmagna FaustineLarmagna commented Sep 26, 2016

In this pull request you will find the solution I used to fix an issue ( #431 ) when dealing with too long file/directory paths: move the pointer to the directory containing the target file (or directory) before doing any FTP actions. And put the pointer back to where it was before the action afterwards.


This change is Reviewable

@@ -1,7 +1,7 @@
{
"name": "knplabs/gaufrette",
"name": "FaustineLarmagna/gaufrette",
Copy link
Contributor

@stof stof Sep 26, 2016

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

@@ -14,6 +14,10 @@
{
"name": "The contributors",
"homepage": "http://github.com/knplabs/Gaufrette/contributors"
},
{
"name": "Faustine Larmagna",
Copy link
Contributor

@stof stof Sep 26, 2016

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

@FaustineLarmagna FaustineLarmagna Sep 26, 2016

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.

@@ -54,93 +55,122 @@ public function __construct($directory, $host, $options = array())
}

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

@stof stof Sep 26, 2016

Choose a reason for hiding this comment

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

should be reverted


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

if (!ftp_fget($this->getConnection(), $temp, $this->computePath($key), $this->mode)) {
Copy link
Contributor

@stof stof Sep 26, 2016

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

@FaustineLarmagna FaustineLarmagna Sep 26, 2016

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.

return false;
}

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

@stof stof Sep 26, 2016

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


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

@stof stof Sep 26, 2016

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

* @throws \RuntimeException
* @return bool
*/
protected function changeDirectory($directory)
Copy link
Contributor

@stof stof Sep 26, 2016

Choose a reason for hiding this comment

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

should be private

*
* @param string $key
*/
protected function moveToTargetDirectory($key)
Copy link
Contributor

@stof stof Sep 26, 2016

Choose a reason for hiding this comment

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

should also be private

@akerouanton
Copy link
Contributor

@akerouanton akerouanton commented Mar 2, 2017

@FaustineLarmagna Can you update your PR as requested please ?

@akerouanton akerouanton added this to the v1.0 milestone Jun 8, 2017
@fabschurt fabschurt removed their assignment Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants