indeyets / pake

pake is a PHP automation tool with capabilities similar to make
http://wiki.github.com/indeyets/pake
MIT License
182 stars 16 forks source link

pake_replace_regexp_to_dir fails if passed absolute file name and empty path #42

Open gggeek opened 10 years ago

gggeek commented 10 years ago

in line 293 of pake_function.php:

$content = pake_read_file($src_dir.'/'.$file);

we should prepend $src_dir . DIRECTORY_SPERARATOR only when $src_dir is not empty. Otherwise we get an error on windows: /C:/a/file/path

indeyets commented 10 years ago

hm… and what is the case for calling pake_replace_regexp_to_dir without $src_dir? that's a required parameter.

if there is a legitimate usecase — I'll figure out solution. otherwise, I can apply stricter error-checks

gggeek commented 10 years ago

To be honest, I was just testing a workaround to a problem I have :-)

Here's the deal. Code which used to work, up to pake 1.7.1

$files = pakeFinder::...->in( $dir );
pake_replace_regexp( $files, $dir, ... );

Since Pake 1.7.2, this does not work any more, and I have to use instead:

$files = pakeFinder::...->relative()->in( $dir );
pake_replace_regexp( $files, $dir, ... );

But that does not seem to work either, at least when on Windows and using my code (which at the top-level uses all paths/filenames with forwards slashes, trying to be compatible with both win and unix).

I'm gonna send 2 pull requests for that btw

indeyets commented 10 years ago

so… pake_replace_regexp() doesn't work if it gets array of absolute filenames. right? would it be sufficient if I solve this issue?

strictly speaking, that is a bit of an edge-case from the pake-style approach point of view. relative paths are better, but best option is to pass pakeFinder object to pake_replace_regexp, not array of paths. but I understand that it might be problematic sometimes.

that's one of the things which has to be implemented cleanly in pake2 :)

gggeek commented 10 years ago

I agree that this should be cleaned up a bit, but especially documented.

As of now, there are various pake_xxx functions which take a "filelist", "path" as arguments, and nowhere it was stated what is supported for input.

  1. passing a pakeFinder for "filelist" is good, but so far I mostly coded this way:
$files = pakeFinder::...->in( $dir );
... some other work ...
pake_replace_regexp( $files, $dir, ... );
which means I throw away the pakeFinder object immediately. To keep it around is a bit of a chore (lot of    refactoring)

Also if I want to pass in a list of files instead of an object, using relative paths is a bit of a chore as well. Main use case is that I also have to do some other manipulations on those files (and other manipulations would be done using absolute path, so I'd need to execute the PakeFinder twice w. different options)
  1. in an ideal world, we should support all cases :-) But I'm not sure how complex the code in pakeFinder::get_files_from_argument would end up being.

    Trying to enumerate all cases: 1 - pakefinder obj + non empty dir path 2 - array (or string) using relative paths + non empty dir path 3 - array (or string) using absolute paths + non empty dir path 4 - pakefinder obj + empty dir path 5 - array (or string) using relative paths + empty dir path 6 - array (or string) using absolute paths + empty dir path

    4 and 5 seem to make no sense. What about 6? Case 3 is currently broken, whereas it would be practical to have it working as long as the files path is a subdir of the dir path

indeyets commented 10 years ago

it's a bit more complicated, as we also have pake_replace_regexp_to_dir() which takes target dir as additional parameter.

absolute-paths will work for in-place replacement ($src_dir === $target_dir), but probably won't if target differs. I can't think of proper behaviour in this case.

also, there is a weird use-case when some of paths in array are relative and some are absolute… :-/