Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromtgalopin:readlink
Jul 30, 2016

Conversation

@tgalopin
Copy link
Contributor

QA
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

readlink() andrealpath() 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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, the Linux convention seems the best one.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

What do you think ofreadLink for "not final" andresolveLink for "final"?

@tgalopin
Copy link
ContributorAuthor

Any opinion on the methods names here?

@tgalopin
Copy link
ContributorAuthor

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

@fabpot
Copy link
Member

I would use the method names from the Linux behavior.

thrownewIOException(sprintf('The link %s does not exist and cannot be read.',$path));
}

if (!is_link($path)) {

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

PHP'sreadlink() function will emit a warning if the given path is no link. I think we should throw anInvalidArgumentException then.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

That seems appropriate indeed.

@tgalopintgalopinforce-pushed thereadlink branch 3 times, most recently from0b94a11 tof90548aCompareMarch 12, 2016 14:22
@tgalopin
Copy link
ContributorAuthor

I did the changes.

@xabbuh
Copy link
Member

looks like you need to fix the tests

Status: Needs Work

@tgalopin
Copy link
ContributorAuthor

I fixed the tests

@tgalopintgalopinforce-pushed thereadlink branch 3 times, most recently fromaf2ad79 to583fdf1CompareApril 15, 2016 20:09
@tgalopin
Copy link
ContributorAuthor

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

if (!$this->exists($path)) {
thrownewIOException(sprintf('The link %s does not exist and cannot be read.',$path));
}

Choose a reason for hiding this comment

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

Why this limitation ?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

Ok I understood, OK for exception here for me

@tgalopin
Copy link
ContributorAuthor

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

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedApr 19, 2016•
edited
Loading

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

@tgalopintgalopinforce-pushed thereadlink branch 2 times, most recently fromfaa689c to259b4ddCompareApril 22, 2016 18:46
@fabpot
Copy link
Member

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

@tgalopin
Copy link
ContributorAuthor

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

@tgalopin
Copy link
ContributorAuthor

tgalopin commentedJul 29, 2016•
edited
Loading

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

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

By default ($canonicalize = false) corresponding to commandreadlink <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 commandreadlink -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 ofbar -> 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
Copy link
Member

nicolas-grekas commentedJul 29, 2016•
edited
Loading

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

@tgalopintgalopinforce-pushed thereadlink branch 6 times, most recently from8efe4a5 toe79b1a5CompareJuly 29, 2016 14:53
@tgalopin
Copy link
ContributorAuthor

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

@tgalopintgalopinforce-pushed thereadlink branch 2 times, most recently fromfa3b7fd to7ed04a4CompareJuly 29, 2016 15:28
@nicolas-grekas
Copy link
Member

👍
Status: reviewed

@fabpot
Copy link
Member

Thank you@tgalopin.

@fabpotfabpot merged commitc36507e intosymfony:masterJul 30, 2016
fabpot added a commit that referenced this pull requestJul 30, 2016
…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
xabbuh added a commit to symfony/symfony-docs that referenced this pull requestSep 21, 2016
…(tgalopin)This PR was merged into the master branch.Discussion----------[Filesystem] Add documentation for the readlink methodFollowing the merge ofsymfony/symfony#17498, this PR introduces documentation for the new methd `readlink`.Commits-------3572a3d [Filesystem] Add documentation for the readlink method
@fabpotfabpot mentioned this pull requestOct 27, 2016
@tgalopintgalopin deleted the readlink branchNovember 8, 2019 10:54
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tgalopin@fabpot@xabbuh@nicolas-grekas@javiereguiluz@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp