[Filesystem] Add the Path class #30969

theofidry · 2019-04-07T10:53:31Z

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

The current Filesystem component is great to deal with the actual file system. It is however lacking when it comes to manipulating with file paths, and when it does, it checks the paths existence which might not what you desire in certain cases.

This PR proposes to merge webmozart/path-util (the path related part only) to Symfony. The library has been very stable, well tested, well documented, but is lacking some love as webmozart doesn't have time to check it anymore.

The current state is the original as is to get a first assessment on what to be changed before working more on this if there is an interest.

It is also worth mentioning that this class has been heavily used in Box and Puli with success among others.

webmozart

There are still wrong file headers and wrong @since comments. Otherwise I love it, but I'm obviously biased. 😄

src/Symfony/Component/Filesystem/Path.php

src/Symfony/Component/Filesystem/Tests/PathTest.php

src/Symfony/Component/Filesystem/Path.php

src/Symfony/Component/Filesystem/Tests/PathTest.php

src/Symfony/Component/Filesystem/Path.php

OskarStark

What do you think of making this a standalone component? Or is It to much overhead? Maybe this abstraction could be quite helpful for the php eco system without requiring the whole filesystem component.

src/Symfony/Component/Filesystem/Tests/PathTest.php

src/Symfony/Component/Filesystem/Path.php

kunicmarko20 · 2019-04-08T09:10:21Z

Any reason why this is modelled as a util class instead of a value object, wouldn't that be more useful?

src/Symfony/Component/Filesystem/Path.php

webmozart · 2019-04-08T10:04:39Z

Any reason why this is modelled as a util class instead of a value object, wouldn't that be more useful?

This class is a very low level building block designed to support thousands of calls to its methods. The only notable state is the home directory, which is only relevant if you use ~ in your paths.

Under these aspects, I don't see the point of complicating this to:

$path = new Path();
$relativePath = $path->makeRelative('/foo/bar', '/foo');

The methods are pure (except if you use ~, which you shouldn't in tests), hence testability is not an issue.

kunicmarko20 · 2019-04-08T10:08:35Z

I was thinking more like:

$path = new Path('/foo/bar');
$path->canonicalize();

I guess the issue is that I have no idea where this class is gonna be used as this PR just adds it and doesn't actually show where it is going to be used.

theofidry · 2019-04-08T10:11:55Z

@kunicmarko20 I would instantiate Path. It's mostly stateless functions and when it's not it's for performance reasons (and having it in a class avoids to have to use the global state as an alternative).

It's also not something you really want to abstract/mock, so there is really no reason to have to instantiate it.

I guess the issue is that I have no idea where this class is gonna be used as this PR just adds it and doesn't actually show where it is going to be used.

I don't know if there is a lot of cases within the Symfony codebase to be honest. I however pointed two projects using it a lot.

src/Symfony/Component/Filesystem/Path.php

apfelbox · 2019-04-11T08:28:13Z

If we make it a value object, practically all of these functions should return a new Path and not a string, and the object should implement __toString().

Might be quite convenient to have it as a VO, but it will work as static methods as well (that's how node.js does it).

theofidry · 2019-04-11T09:59:01Z

If we make it a value object, practically all of these functions should return a new Path and not a string, and the object should implement __toString().

@apfelbox that would be quite an overhead; It's fine if you approach paths as a VO in your codebase and performances are not an issue, but in projects like Box or Puli in which it's being used, it's unreasonable.

theofidry · 2019-04-11T10:02:33Z

What do you think of making this a standalone component? Or is It to much overhead?

@OskarStark This would be quite an overhead IMO, also the FS component is relatively light so I don't think it's an issue.

apfelbox · 2019-04-11T10:05:15Z

@theofidry ok, in this case I would stick to static methods. 👍

theofidry · 2019-04-11T10:14:02Z

Fabbot fixed, the Travis & AppVeyor failures seem unrelated

theofidry · 2019-05-16T17:28:46Z

status: needs review

src/Symfony/Component/Filesystem/Path.php

theofidry · 2019-09-26T19:52:19Z

Rebased against master. The Travis failure looks unrelated (it's about the Cache component)

OskarStark · 2019-09-26T20:38:21Z

Would be happy to get this merged 🥰🥳

BTW, Shouldnt this go into 4.4 as a new feature?

src/Symfony/Component/Filesystem/Path.php

theofidry · 2019-12-10T21:47:24Z

Can a decision be taken about this class?

Just to put things in context: this class can be seen as complementary to the Filesystem as the last has a lot of similar path-related methods, the main difference being that the Filesystem will check their existences. Path is more meant as a path manipulator without doing any existence check whatsoever which is very useful to manipulate/construct/change paths without side-effects.

The main question IMO is whether or not this class is a good fit for the current Filesystem component, which brings the question of the main purpose of the Filesystem component. To put things in perspective, the Filesystem component is 8 years old (originally created by willdurand) and has been (regardless of the original intent) ever since then a thin layer on top of some file system related PHP native functions to patch/enrich them.

mvorisek · 2019-12-11T15:58:17Z

src/Symfony/Component/Filesystem/Path.php

+        // Windows root
+        if (\strlen($path) > 1 && ctype_alpha($path[0]) && ':' === $path[1]) {
+            // Special case: "C:"
+            if (2 === \strlen($path)) {


See php/php-src#5001 , C: is oficially not an absolute path.

chalasr · 2019-12-11T18:27:04Z

Can a decision be taken about this class?

It looks useful to me. And I think Filesystem is a good fit for it.
Making core uses it internally may help moving forward, I suggest looking at places where this could be used.

src/Symfony/Component/Filesystem/Path.php

fabpot · 2020-10-07T15:56:24Z

We've just moved away from master as the main branch to use 5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to 5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

tomasfejfar · 2020-12-19T13:07:04Z

@theofidry do you plan to reopen the PR? :)

This PR was merged into the 5.4 branch. Discussion ---------- [Filesystem] Add the Path class | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix #26523 | License | MIT | Doc PR | TODO Re-opening #30969 as requested by \`@fabpot`. This PR proposes a low-level, performant and pure API to manipulate file paths. Two methods `Path::isAbsolute()` and `Path::isRelative()` are similar to `Filesystem::isAbsolutePath()` and `Filesystem::makePathRelative()`. They are however different as the methods from `Path` do not check the existence of the path which is very helpful in some scenarios (for example when manipulating virtual paths such as "regular" paths of a PHAR). The original code comes from [webmozart/path-util](https://github.com/webmozart/path-util) from which the path part was extracted. The library is very stable, well tested and documented. It has been heavily used in [Puli](https://github.com/puli), which although is no longer active has been a good performance test and is used in several other popular projects such as: - vimeo/psalm - drush/drush - infection/infection - humbug/box - humbug/php-scoper - phpbench/phpbench It however suffers from lack of maintener from `@webmozart` which no longer has time to maintain it. Commits ------- 512b476 Add the Path class

carsonbot added Status: Needs Review Feature labels Apr 7, 2019

javiereguiluz added the ⭐️ EUFOSSA Hackathon label Apr 7, 2019

webmozart suggested changes Apr 7, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Path.php Outdated Show resolved Hide resolved

src/Symfony/Component/Filesystem/Path.php Outdated Show resolved Hide resolved

src/Symfony/Component/Filesystem/Path.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 7, 2019

nicolas-grekas changed the title ~~[WIP] Add the Path class~~ [Filesystem] Add the Path class Apr 7, 2019

nicolas-grekas reviewed Apr 7, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Path.php Outdated Show resolved Hide resolved

derrabus requested changes Apr 7, 2019

View reviewed changes

OskarStark reviewed Apr 7, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Tests/PathTest.php Outdated Show resolved Hide resolved

OskarStark reviewed Apr 7, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Path.php Outdated Show resolved Hide resolved

nicolas-grekas added this to the next milestone Apr 7, 2019

stof reviewed Apr 8, 2019

View reviewed changes

kunicmarko20 reviewed Apr 8, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Path.php Show resolved Hide resolved

kunicmarko20 reviewed Apr 8, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Path.php Show resolved Hide resolved

chalasr added the Filesystem label Apr 8, 2019

ro0NL reviewed Apr 11, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Path.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Review and removed Status: Needs Work labels May 16, 2019

OskarStark reviewed May 16, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Path.php Outdated Show resolved Hide resolved

theofidry added 9 commits September 26, 2019 21:23

Use the Filesystem exceptions instead of the PHP root ones

329cf39

Add myself as an author

928a0df

Remove getFilename()

e8f857c

Use long lines

838727b

Mark Path as experimental

e9b7035

Remove non-useful phpdoc

42c6d72

Fix PathTest

8113090

Add myself to PathTest

4e38d18

Apply path

2688745

theofidry force-pushed the feature/path-component branch from a7cb394 to 2688745 Compare September 26, 2019 19:24

theofidry added 2 commits September 26, 2019 21:25

Fix the experimental tag

96774e9

Restore PHPUnit binary

b28cf08

theofidry mentioned this pull request Nov 29, 2019

Introduce a FileSystemTestCase to simplify some I/O dependent test cases infection/infection#869

Merged

chalasr reviewed Dec 10, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Path.php Show resolved Hide resolved

src/Symfony/Component/Filesystem/Path.php Show resolved Hide resolved

fabpot removed the ⭐️ EUFOSSA Hackathon label Dec 11, 2019

OskarStark mentioned this pull request Dec 11, 2019

[FrameworkBundle] Fix invalid Windows path normalization in TemplateNameParser #34915

Merged

mvorisek reviewed Dec 11, 2019

View reviewed changes

webmozart reviewed Dec 11, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Path.php Show resolved Hide resolved

nicolas-grekas mentioned this pull request Feb 7, 2020

Filesystem::joinPaths() #33375

Closed

fabpot closed this Oct 7, 2020

nicolas-grekas added the for 5.x label Oct 7, 2020

tomasfejfar mentioned this pull request Jul 2, 2021

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

Closed

theofidry mentioned this pull request Jul 2, 2021

[Filesystem] Add the Path class #41954

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Filesystem] Add the Path class #30969

[Filesystem] Add the Path class #30969

theofidry commented Apr 7, 2019 •

edited

Loading

webmozart left a comment

OskarStark left a comment •

edited

Loading

kunicmarko20 commented Apr 8, 2019

webmozart commented Apr 8, 2019

kunicmarko20 commented Apr 8, 2019

theofidry commented Apr 8, 2019

apfelbox commented Apr 11, 2019

theofidry commented Apr 11, 2019

theofidry commented Apr 11, 2019 •

edited

Loading

apfelbox commented Apr 11, 2019

theofidry commented Apr 11, 2019 •

edited

Loading

theofidry commented May 16, 2019

theofidry commented Sep 26, 2019

OskarStark commented Sep 26, 2019 •

edited

Loading

theofidry commented Dec 10, 2019 •

edited

Loading

mvorisek Dec 11, 2019

chalasr commented Dec 11, 2019

fabpot commented Oct 7, 2020

tomasfejfar commented Dec 19, 2020

[Filesystem] Add the Path class #30969

[Filesystem] Add the Path class #30969

Conversation

theofidry commented Apr 7, 2019 • edited Loading

webmozart left a comment

Choose a reason for hiding this comment

OskarStark left a comment • edited Loading

Choose a reason for hiding this comment

kunicmarko20 commented Apr 8, 2019

webmozart commented Apr 8, 2019

kunicmarko20 commented Apr 8, 2019

theofidry commented Apr 8, 2019

apfelbox commented Apr 11, 2019

theofidry commented Apr 11, 2019

theofidry commented Apr 11, 2019 • edited Loading

apfelbox commented Apr 11, 2019

theofidry commented Apr 11, 2019 • edited Loading

theofidry commented May 16, 2019

theofidry commented Sep 26, 2019

OskarStark commented Sep 26, 2019 • edited Loading

theofidry commented Dec 10, 2019 • edited Loading

mvorisek Dec 11, 2019

Choose a reason for hiding this comment

chalasr commented Dec 11, 2019

fabpot commented Oct 7, 2020

tomasfejfar commented Dec 19, 2020

theofidry commented Apr 7, 2019 •

edited

Loading

OskarStark left a comment •

edited

Loading

theofidry commented Apr 11, 2019 •

edited

Loading

theofidry commented Apr 11, 2019 •

edited

Loading

OskarStark commented Sep 26, 2019 •

edited

Loading

theofidry commented Dec 10, 2019 •

edited

Loading