[Filesystem] Add a cross-platform readlink method #17498

tgalopin · 2016-01-22T17:09:34Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

readlink() and realpath() have a completely different behavior under Windows and Unix:

realpath() resolves recursively the children links of a link until a final target is found on Unix and resolves only the next link on Windows ;
readlink() resolves recursively the children links of a link until a final target is found on Windows and resolves only the next link on Unix ;

I propose to solve this by implementing a helper method in the Filesystem component that would behave always the same way under all platforms.

fabpot · 2016-01-25T09:30:12Z

src/Symfony/Component/Filesystem/Filesystem.php

+     *
+     * @throws IOException When the link does not exist or is not readable.
+     */
+    public function readlink($path, $finalTarget = false)


I would have defaulted to true here. Or even better, why not create 2 methods?

Or just keep realpath, which is very useful. I'm not sure readlink is that useful anyway.

When you say realpath, what do you mean :) ? With final target to true ?

We are using both the "not final" and the "final" cases in Puli, I think two methods is a good idea indeed. How should we name them though? If we choose realpath/readlink, which convention do we choose? I would prefer the Unix one personnally (realpath = final, readlink = not final).

Indeed, the Linux convention seems the best one.

I did the change, but I think the name "realpath" is misleading. The PHP realpath do more things than resolving links (it also canonicalizes and normalizes the path: http://php.net/manual/fr/function.realpath.php). I'm not sure what happens in our case on Windows / Unix and if the behavior is really the same everywhere. Perhaps should we find a better name.

What do you think of readLink for "not final" and resolveLink for "final"?

tgalopin · 2016-01-26T12:48:32Z

Any opinion on the methods names here?

tgalopin · 2016-01-29T10:26:54Z

I rebased this on master. What do you think if this feature?

fabpot · 2016-03-04T10:56:35Z

I would use the method names from the Linux behavior.

nicolas-grekas · 2016-03-04T11:07:52Z

src/Symfony/Component/Filesystem/Filesystem.php

+        }
+
+        if (!is_link($path)) {
+            return $path;


why shouldn't this method work on regular files/dirs?

I'm not sure what you mean, but the support of non-links paths was something I found useful. This can be changed, I'm not sure what's the excepted behavior.

PHP's readlink() function will emit a warning if the given path is no link. I think we should throw an InvalidArgumentException then.

That seems appropriate indeed.

tgalopin · 2016-03-12T14:24:12Z

I did the changes.

xabbuh · 2016-03-13T19:01:40Z

looks like you need to fix the tests

Status: Needs Work

tgalopin · 2016-03-13T20:28:43Z

I fixed the tests

tgalopin · 2016-04-15T20:18:40Z

I think this is ready, targeting 3.1. Any review?

nicolas-grekas · 2016-04-15T20:51:17Z

src/Symfony/Component/Filesystem/Filesystem.php

+            throw new IOException(sprintf('The link %s does not exist and cannot be read.', $path));
+        }
+
+        if (!is_link($path)) {


Why this limitation ?

I'm not sure what behavior we should have. The native behavior is to return false, but that seems inapropriate to me as we should return a string. What would you expect?

Ok I understood, OK for exception here for me

tgalopin · 2016-04-19T11:09:07Z

By canonicalized, do you mean absolute or also fully resolved link?

nicolas-grekas · 2016-04-19T12:20:53Z

Both, ie what realpath does (on PHP/Linux)

fabpot · 2016-06-21T20:42:04Z

@tgalopin Any news on this PR? Is it worth finishing it?

tgalopin · 2016-06-21T21:14:55Z

@fabpot I think it is, I'll work on it.

tgalopin · 2016-07-29T12:18:57Z

After some research, I propose the following behavior (same for every platform), following the way the readlink command works:

Signature: readlink($path, $canonicalize = false)

By default ($canonicalize = false) corresponding to command readlink <path>

if $path does not exist or is not a link, return null
if $path is link, return the value of the link (ie. the next direct target of the link, without consideration of existence)

With canonicalization ($canonicalize = true) corresponding to command readlink -f <path>

if $path does not exist, return null
if $path exists, return the absolute, fully resolved final target of the link (in the case of bar -> baz -> foo, readlink(bar) would return /absolute/path/to/foo)

Is everyone OK with this behavior? If it's ok for everyone, I'll start working on it.

nicolas-grekas · 2016-07-29T12:39:35Z

Ok for me with this updated version (no exceptions but return null instead)

tgalopin · 2016-07-29T15:04:12Z

I applied this behavior and tests are green, I think this is ready for review.

…d links

nicolas-grekas · 2016-07-30T07:19:15Z

👍
Status: reviewed

fabpot · 2016-07-30T07:30:45Z

Thank you @tgalopin.

…lopin) This PR was merged into the 3.2-dev branch. Discussion ---------- [Filesystem] Add a cross-platform readlink method | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - `readlink()` and `realpath()` have a completely different behavior under Windows and Unix: - `realpath()` resolves recursively the children links of a link until a final target is found on Unix and resolves only the next link on Windows ; - `readlink()` resolves recursively the children links of a link until a final target is found on Windows and resolves only the next link on Unix ; I propose to solve this by implementing a helper method in the Filesystem component that would behave always the same way under all platforms. Commits ------- c36507e [Filesystem] Add a cross-platform readlink/realpath methods for nested links

…(tgalopin) This PR was merged into the master branch. Discussion ---------- [Filesystem] Add documentation for the readlink method Following the merge of symfony/symfony#17498, this PR introduces documentation for the new methd `readlink`. Commits ------- 3572a3d [Filesystem] Add documentation for the readlink method

carsonbot added the Status: Needs Review label Jan 22, 2016

tgalopin mentioned this pull request Jan 22, 2016

Add cross-platform readLink method webmozart/path-util#19

Closed

tgalopin force-pushed the readlink branch from e70815a to 7f5981e Compare January 22, 2016 17:11

fabpot reviewed Jan 25, 2016
View reviewed changes

tgalopin force-pushed the readlink branch from 7f5981e to fb2da3a Compare January 25, 2016 10:03

javiereguiluz added the Filesystem label Jan 26, 2016

tgalopin force-pushed the readlink branch from fb2da3a to 22d9cc1 Compare January 29, 2016 10:26

tgalopin force-pushed the readlink branch from 22d9cc1 to f7d9c5d Compare February 1, 2016 17:13

javiereguiluz added the Feature label Mar 3, 2016

nicolas-grekas reviewed Mar 4, 2016
View reviewed changes

tgalopin force-pushed the readlink branch 3 times, most recently from 0b94a11 to f90548a Compare March 12, 2016 14:22

carsonbot added Status: Needs Work and removed Status: Needs Review labels Mar 13, 2016

tgalopin force-pushed the readlink branch from f90548a to 911beda Compare March 13, 2016 20:22

tgalopin force-pushed the readlink branch 3 times, most recently from af2ad79 to 583fdf1 Compare April 15, 2016 20:09

tgalopin force-pushed the readlink branch from 583fdf1 to 5517d20 Compare April 15, 2016 20:19

nicolas-grekas reviewed Apr 15, 2016
View reviewed changes

tgalopin force-pushed the readlink branch 2 times, most recently from faa689c to 259b4dd Compare April 22, 2016 18:46

tgalopin force-pushed the readlink branch from 259b4dd to fc3ef8d Compare July 29, 2016 12:21

tgalopin force-pushed the readlink branch 6 times, most recently from 8efe4a5 to e79b1a5 Compare July 29, 2016 14:53

tgalopin force-pushed the readlink branch 2 times, most recently from fa3b7fd to 7ed04a4 Compare July 29, 2016 15:28

[Filesystem] Add a cross-platform readlink/realpath methods for neste…

c36507e

…d links

tgalopin force-pushed the readlink branch from 7ed04a4 to c36507e Compare July 29, 2016 15:39

carsonbot added Status: Reviewed and removed Status: Needs Work labels Jul 30, 2016

fabpot merged commit c36507e into symfony:master Jul 30, 2016

tgalopin mentioned this pull request Sep 7, 2016

[Filesystem] Add documentation for the readlink method symfony/symfony-docs#6956

Merged

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

tgalopin deleted the readlink branch November 8, 2019 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Filesystem] Add a cross-platform readlink method #17498

[Filesystem] Add a cross-platform readlink method #17498

tgalopin commented Jan 22, 2016

fabpot Jan 25, 2016

fabpot Jan 25, 2016

tgalopin Jan 25, 2016

fabpot Jan 25, 2016

tgalopin Jan 25, 2016

tgalopin Jan 25, 2016

tgalopin commented Jan 26, 2016

tgalopin commented Jan 29, 2016

fabpot commented Mar 4, 2016

nicolas-grekas Mar 4, 2016

tgalopin Mar 4, 2016

xabbuh Mar 7, 2016

tgalopin Mar 7, 2016

tgalopin commented Mar 12, 2016

xabbuh commented Mar 13, 2016

tgalopin commented Mar 13, 2016

tgalopin commented Apr 15, 2016

nicolas-grekas Apr 15, 2016

tgalopin Apr 15, 2016

nicolas-grekas Apr 15, 2016

tgalopin commented Apr 19, 2016

nicolas-grekas commented Apr 19, 2016 •

edited

Loading

fabpot commented Jun 21, 2016

tgalopin commented Jun 21, 2016

tgalopin commented Jul 29, 2016 •

edited

Loading

nicolas-grekas commented Jul 29, 2016 •

edited

Loading

tgalopin commented Jul 29, 2016

nicolas-grekas commented Jul 30, 2016

fabpot commented Jul 30, 2016

[Filesystem] Add a cross-platform readlink method #17498

[Filesystem] Add a cross-platform readlink method #17498

Conversation

tgalopin commented Jan 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgalopin commented Jan 26, 2016

tgalopin commented Jan 29, 2016

fabpot commented Mar 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgalopin commented Mar 12, 2016

xabbuh commented Mar 13, 2016

tgalopin commented Mar 13, 2016

tgalopin commented Apr 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgalopin commented Apr 19, 2016

nicolas-grekas commented Apr 19, 2016 • edited Loading

fabpot commented Jun 21, 2016

tgalopin commented Jun 21, 2016

tgalopin commented Jul 29, 2016 • edited Loading

nicolas-grekas commented Jul 29, 2016 • edited Loading

tgalopin commented Jul 29, 2016

nicolas-grekas commented Jul 30, 2016

fabpot commented Jul 30, 2016

nicolas-grekas commented Apr 19, 2016 •

edited

Loading

tgalopin commented Jul 29, 2016 •

edited

Loading

nicolas-grekas commented Jul 29, 2016 •

edited

Loading