-
Notifications
You must be signed in to change notification settings - Fork 16
replaced path-util with symfony/filesystem #59
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
replaced path-util with symfony/filesystem #59
Conversation
Ocramius
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Source-wise, this is fine, but in practice I would rather get rid of symfony/* as a dependency, perhaps including the upstream sources and relevant tests, rather than introducing a dependency.
| "require": { | ||
| "php": "^7.3 || ^8.0.0", | ||
| "webmozart/path-util": "^2.2" | ||
| "symfony/filesystem": "^5.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit vary on introducing symfony/* dependencies, since they are inherently unsafe from a dependency PoV:
See also #57
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is at least one other Project that does not want to use the symfony Project for the same reasons, i tried to Fork it and update it in this Project https://github.com/a4blue/path-util
If i require it instead, i think it should be ok with everyone :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ocramius What's the stance of the Symfony team on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea: tbh, including their classes (renamed, with a licence notice here) is probably the safest bet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now merging and releasing this is still better than keeping the abandoned repo in there as dependency.
Along the lines quite a few libraries now generate the warning report here:
Package webmozart/path-util is abandoned, you should avoid using it. Use symfony/filesystem instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in that case a higher support for also Symf 4/5/6 might be good.
Or indeed the classes are just copied - as long as you keep the original copyright (license header), it will be fine.
Thats even easier and will have less dependency issues with symfony and changing their signature/code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now merging and releasing this is still better than keeping the abandoned repo in there as dependency.
Not really a problem until a security issue occurs.
I suggest sending a patch with the necessary upstream dependency code, license headers and tests included.
…bsolute()` This imports sources from `webmozart/path-util`: since the license and authors are the same, no further license additions are necessary. Ref: https://github.com/webmozart/path-util/blob/6099b5238073f87f246863fd58c2e447acfc0d24/tests/PathTest.php Ref: https://github.com/webmozart/path-util/blob/6099b5238073f87f246863fd58c2e447acfc0d24/src/Path.php Fixes #59 Fixes #57
…bsolute()` This imports sources from `webmozart/path-util`: since the license and authors are the same, no further license additions are necessary. Ref: https://github.com/webmozart/path-util/blob/6099b5238073f87f246863fd58c2e447acfc0d24/tests/PathTest.php Ref: https://github.com/webmozart/path-util/blob/6099b5238073f87f246863fd58c2e447acfc0d24/src/Path.php Fixes #59 Fixes #57
|
Removed dependency in #60 |
No description provided.