cpan-authors / IPC-Run

https://metacpan.org/pod/IPC::Run
Other
21 stars 38 forks source link

Handle reference to undefined array in IPC::Run::run() #164

Open szmate1618 opened 2 years ago

szmate1618 commented 2 years ago

Fixes #162.

This pull request currently contains 2 changes:

Any of the two would be sufficient on its own to solve this specific problem, but I suggest to go with both.

I think the first change is important because regardless of this issue, it is semantically incorrect to search for empty file names on a path. And the second change is important because if we know we are going to fail, it's better to fail fast. Of course, failing really fast would mean the very first line of run, but after studying the code a little, it felt like the correct place to handle such special cases is in harness.

Instead of failing silently I'm using croak in both cases, but I'm unsure if this is the correct decision, and would like to hear your input. I'm unsure, because this library has many users, and it is quite possible that someone has accidentally (or not) written code that depends on run not failing on undefined values. This fix would break such code. On the other hand, if your code accidentally depends on a bug, maybe you want it to break, so you notice the problem.

I would add some tests, but to be perfectly honest, I have no idea how the test framework works.

szmate1618 commented 2 years ago

Ok, I did not expect those test failures. I did run the tests on my machine, at least on Windows, and they passed (except for the ones that are skipped on that platform). edit: I accidentally ran the tests on an earlier version, without my changes.

I will look into this tomorrow.