PerlDancer
Move to Path::Tiny
Path::Tiny provides a clean, correct, and consistent interface to various path-related modules. We have our own module, which could be improved.
I want to at least replace the core of it with Path::Tiny and maybe remove it at some point in favor.
This is a work in progress. I have more work to commit once the tests pass.
- [ ] Clean up
FileUtilsinternals. - [ ] Clean up usages of
File::Spec. - [ ] Clean up usages of other
File::*modules. - [ ]
File::Basename->path($path)->basename. (basediris not available.) - [ ] Clean up usages of
-d->path($path)->is_dir. - [ ] Clean up usages of
-f->path($path)->is_file. - [ ] Clean up usages of
-e->path($path)->exists.
I have rewritten and rebased this branch. It's basically waiting on me to finish merge_path from the File Handler, but I need to first figure out what it's doing. :/
Last commit fixes that.
My next part is cleaning up the explicit calls to Path::Tiny::path() and then I'll start removing the stringify that exists everywhere.
That merge_paths commit is no good in that form, i'm afraid.
All of these should throw errors instead of "succeeding", otherwise they present huge security problems and/or break tests on systems with more than one file system tree. @haarg probably has other ideas of how this could break as well.
C:\Users\Mithaldu>perl -e "use Path::Tiny; print Path::Tiny::path( 'c:/meep', 'c:/marp' )->stringify"
C:/meep/c:/marp
C:\Users\Mithaldu>perl -e "use Path::Tiny; print Path::Tiny::path( 'c:/meep', 'd:/marp' )->stringify"
C:/meep/d:/marp
C:\Users\Mithaldu>perl -e "use Path::Tiny; print Path::Tiny::path( 'c:/meep', '../marp' )->stringify"
C:/marp
@wchristian, Thank you for testing them out. I'm not sure what these should have been doing to begin with. What does it mean to ask for "c:/meep", "d:/marp"? These are different volumes.
The first two are both straight-up non-sense in the vein of merge_path( "~/.ssh", "ftp://some.server/whatever" ) so they should error out, nothing else.
The third is "merely" dangerous and should be prevented.
I just rebased this branch (had a few conflicts) and I wrote a solution to the issues raised by @mithaldu.
The short answer is: 2 out of 3 cases get it wrong with both merge_paths (our code that I'm removing) and with Path::Tiny, but it's okay because our code will catch it.
The problem with the third case is a security issue, but not because of what Path::Tiny does. It does the right thing - but we assume the user is correct in doing this, which is a possible security issue.
I've resolved the third issue but I want to write some tests before I commit and push these too.
I added the fix and the tests. Seems be be good. I originally also patched Dancer2::Handler::File but coudn't find any test for it. Then I couldn't find anything using it. I made a syntax error in i and ran the entire testing suite - successful. Are we even using this module anymore?
You replaced the one place we were using Dancer2::Handler::File, so I think at this point we can effectively retire it.
This is good stuff. :+1: from me.
@xsawyerx I just rebased this against current master as I'd really like to see it merged. I'm going to go back over all of the comments to check that all issues are addressed, and then will add a further comment.
Some local test failures, but Travis is busy with maintenance atm. Lots of interesting appveyor failures too. I'll start looking tomorrow.