Skip to content
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

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

Open
gggeek opened this issue Nov 6, 2013 · 5 comments

Comments

@gggeek
Copy link
Collaborator

gggeek commented Nov 6, 2013

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

@ghost ghost assigned indeyets Nov 6, 2013
@indeyets
Copy link
Owner

indeyets commented Nov 6, 2013

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
Copy link
Collaborator Author

gggeek commented Nov 6, 2013

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
Copy link
Owner

indeyets commented Nov 6, 2013

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
Copy link
Collaborator Author

gggeek commented Nov 6, 2013

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
Copy link
Owner

indeyets commented Nov 6, 2013

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… :-/

@indeyets indeyets removed their assignment Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants