Skip to content

FTP: Implement MLSD for structured listing of directories#2345

Closed
blar wants to merge 3 commits intophp:masterfrom
blar:master
Closed

FTP: Implement MLSD for structured listing of directories#2345
blar wants to merge 3 commits intophp:masterfrom
blar:master

Conversation

@blar
Copy link
Copy Markdown
Contributor

@blar blar commented Jan 28, 2017

@krakjoe
Copy link
Copy Markdown
Member

krakjoe commented Jan 28, 2017

This doesn't appear to come with tests, is it possible to add some ?

Comment thread ext/ftp/php_ftp.c Outdated
PHP_FE(ftp_alloc, arginfo_ftp_alloc)
PHP_FE(ftp_nlist, arginfo_ftp_nlist)
PHP_FE(ftp_rawlist, arginfo_ftp_rawlist)
PHP_FE(ftp_mlsd, arginfo_ftp_mlsd)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indent

@krakjoe
Copy link
Copy Markdown
Member

krakjoe commented Jan 29, 2017

Merged 0103d1e

Thanks.

@krakjoe krakjoe closed this Jan 29, 2017
@KalleZ
Copy link
Copy Markdown
Member

KalleZ commented Jan 29, 2017

Nice work on this one.

Was thinking, what if we had an option to return the results in a nicer format? so instead of a string that will most likely needed to be parsed, then having it as an array. For example (using the first line from the test results):

<?php
var_dump(ftp_mlsd($ftp_s, '/hello/world', true)[0]);
?>

Output

array(7) {
  ["modify"]=>
  string(14) "20170127230002"
  ["perm"]=>
  string(7) "flcdmpe"
  ["type"]=>
  string(4) "cdir"
  ["unique"]=>
  string(11) "811U4340002"
  ["UNIX_group"]=>
  string(2) "33"
  ["UNIX_mode"]=>
  string(4) "0755"
  ["UNIX_owner"]=>
  string(2) "33"
}

@nikic
Copy link
Copy Markdown
Member

nikic commented Jan 29, 2017

Agree with @KalleZ, I think it would be good to parse the result.

@nikic
Copy link
Copy Markdown
Member

nikic commented Jan 29, 2017

Generally, I'd appreciate it if enhancement PRs were left open a bit longer, as they usually generate more discussion (see also: Recent PHP_OS_FAMILY merge). Merging a function addition after one day on a weekend does not really allow for any discussion.

@krakjoe
Copy link
Copy Markdown
Member

krakjoe commented Jan 29, 2017

@nikic it was merged into master only ... plenty of time for implementing whatever improvements are necessary.

In my defence, FTP is extremely poorly maintained, to the point that it's one of the extensions Stas has shortlisted for removal ... I just didn't think anyone would be interested in it ...

I do obviously agree, my mistake ... I guess we should leave enhancements open for a minimum of one week ?

@nikic
Copy link
Copy Markdown
Member

nikic commented Jan 29, 2017

@krakjoe The trouble with improving things after they're merged is that someone needs to make sure it happens. In this specific case I suspect that neither @KalleZ nor me care much about this, so it's likely that this will be forgotten after a couple of days.

I don't want to impose too much process on this (after all, we also don't want to have PRs sitting around indefinitely), so I don't think we need to have a specific minimum period at this point -- just that one day is a bit tight.

@krakjoe
Copy link
Copy Markdown
Member

krakjoe commented Jan 29, 2017

Fair enough, duly noted.

@KalleZ
Copy link
Copy Markdown
Member

KalleZ commented Jan 30, 2017

You are right @nikic, I'm not a heavy user myself of the FTP extension, mainly just trying to make my voice heard and give our end users a proper solution, not a half one :)

@blar @krakjoe inputs on my proposal?

@krakjoe
Copy link
Copy Markdown
Member

krakjoe commented Jan 30, 2017

Sounds fine.

@blar would you mind opening a PR to implement that idea ?

@blar
Copy link
Copy Markdown
Contributor Author

blar commented Feb 4, 2017

I created a pull request #2364 FTP: Rename function for MLSD. I am working on the parser for the file list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants