Skip to content

[Filesystem] Add $fs->join([$pathSegment]) #26523

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

Closed
tomasfejfar opened this issue Mar 14, 2018 · 14 comments · Fixed by #41954
Closed

[Filesystem] Add $fs->join([$pathSegment]) #26523

tomasfejfar opened this issue Mar 14, 2018 · 14 comments · Fixed by #41954
Labels
Feature Filesystem RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Milestone

Comments

@tomasfejfar
Copy link
Contributor

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Symfony version latest

When working with filesystem it's always very cumbersome to create path from different segments. For example, create full path from base directory, subdirectory and filename. Especially if you need to compare paths later.

What I propose is to add $filesystem->join() method. It's purpose would be to merge different path segment with path separator in a predictable way.

$filesystem->join(['some', 'path', 'segment']); // 'some/path/segment'

$filesystem->join(['/some/path/', '/subdir/', 'filename.tmp']); // '/some/path/subdir/filename.tmp'
// notice how it behaves better than implode('/', $array) in regards to doubleslashes

// following OS depended behavior is up to discussion
$filesystem->join(['c:\', '/subdir/', 'filename.tmp']); // 'c:/subdir/filename.tmp'
// that this way one can be sure that slash is separator and it works fine due to PHP transforming it on the fly when accessing the file

// also this could be useful
$filesystem->toOsSpecific('some/path\to\some/folder'); 
// 'some\path\to\some\folder' on windows
// 'some/path/to/some/folder' on linux
$filesystem->toPathWithSlashes('some\path/to\some\folder');
// always 'some/path/to/some/folder'

What do you think? I'd be happy to send a PR after it's disussed.

@javiereguiluz
Copy link
Member

javiereguiluz commented Jul 31, 2018

For reference, in Go there's a core package called Path that contains a Join() method. Instead of passing an array, it's a variadic function, and it ignores empty strings too:

Join joins any number of path elements into a single path, adding a separating slash if necessary. The result is Cleaned; in particular, all empty strings are ignored.

func Join(elem ...string) string

Example:

package main

import (
	"fmt"
	"path"
)

func main() {
	fmt.Println(path.Join("a", "b", "c"))
	fmt.Println(path.Join("a", "b/c"))
	fmt.Println(path.Join("a/b", "c"))
	fmt.Println(path.Join("", ""))
	fmt.Println(path.Join("a", ""))
	fmt.Println(path.Join("", "a"))
}

a/b/c
a/b/c
a/b/c

a
a

If we decide to do the same, I propose this:

  • Rename this function to Filesystem::joinPaths() because Filesystem::join() looks like you are joining files.
  • Define this function as joinPaths(string ...$paths): string
  • Clean up the result removing double slashes and empty segments
  • Don't make any difference based on OS because PHP doesn't need that (as pointed by the original reporter) Update: Go defines a different package called filepath to take into account the differences between OS.

@javiereguiluz javiereguiluz added this to the next milestone Jul 31, 2018
@tomasfejfar
Copy link
Contributor Author

Let me know when you feel like it's ready and discussed enough for a PR.

@apfelbox
Copy link
Contributor

This exists in node as well and is called path.join().
Would this function merge ../ (without using realpath)?

@yceruto
Copy link
Member

yceruto commented Jul 31, 2018

This also exists in python os.path.join('foo', 'bar') and would be useful to call this function statically too Filesystem::join()

@nicolas-grekas nicolas-grekas added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Jul 31, 2018
@Simperfit
Copy link
Contributor

Can we move forward on this ?
I feel that could be really useful.

@tomasfejfar
Copy link
Contributor Author

I'll be happy to implement what @javiereguiluz proposed. I think that (at least in the first iteration) it shouldn't be too clever and merge ../ as @apfelbox suggested (would break SRP). Maybe there could be a extra function like resolveRelativePathSegments or even $fs->realpath() for things like this. But that's for further discussion. I noticed that there is already a SO question for this!

@Simperfit
Copy link
Contributor

Simperfit commented Apr 19, 2019

@tomasfejfar go for it (when you have time), if you need any help you can ask me !
If you want to initiate the thing and that someone help with tests or implem, same I'm ok to help you ;).

@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@lud
Copy link

lud commented Dec 29, 2020

+1 to this feature 👍

@carsonbot carsonbot removed the Stalled label Dec 29, 2020
@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@lud
Copy link

lud commented Jul 2, 2021

Yes.

@carsonbot carsonbot removed the Stalled label Jul 2, 2021
@tomasfejfar
Copy link
Contributor Author

tomasfejfar commented Jul 2, 2021

#30969 this one is much further...

@wouterj
Copy link
Member

wouterj commented Jul 2, 2021

Is there anyone up to take over that PR or create a new PR for this issue?
It doesn't make sense to keep feature requests stalled for more than 2 years.

If you need help with the contribution process, feel free to hop into the #contribs channel on Symfony Slack.

@wouterj
Copy link
Member

wouterj commented Jul 2, 2021

Ah, I see @theofidry restarted the PR himself. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Filesystem RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants