antecedent / patchwork

Method redefinition (monkey-patching) functionality for PHP.
https://antecedent.github.io/patchwork/
MIT License
437 stars 39 forks source link

Autoloadable with a "start" method? #12

Open adamwathan opened 10 years ago

adamwathan commented 10 years ago

Not sure if this could be done without being a breaking change, but I'm working with my own fork at the moment where I've basically wrapped the bootstrapping in a function call like this:

function start()
{
    Preprocessor\Stream::wrap();

    Preprocessor\attach(array(
        Preprocessor\Callbacks\Preprocessor\propagateThroughEval(),
        Preprocessor\Callbacks\Interceptor\markPreprocessedFiles(),
        Preprocessor\Callbacks\Interceptor\injectCallInterceptionCode(),
        Preprocessor\Callbacks\Interceptor\injectScheduledPatchApplicationCode(),
    ));
}

...and then autoloaded the Patchwork.php file in my composer.json. Allows me to just do a Patchwork\start() in my PHPUnit setUp call for any tests that need monkey patching.

Would be nice to make it possible for the main repo to work in a similar way, if not I can continue to play around with the fork.

Great library, just found it this morning but really made my life a lot easier :) Thanks!

antecedent commented 10 years ago

I'm afraid my fantasy fails me to see the benefits of this – could you elaborate?

Does it have something to do with reducing the preprocessing overhead, specifically for files where nothing will ever need to be monkey-patched? That's a genuine concern, of course. The only thing that the current API has to offer with respect to this is Patchwork\Preprocessor\exclude(). It can be used to mark files or folders to be skipped by the preprocessor. However, it's not elaborate at all – there's no support for patterns (regexes or the like) or whitelisting instead of blacklisting.

But of course, I might have guessed wrong.

Anyway, I'd be leaning more towards something like Patchwork\suspend() and Patchwork\resume(), in order not to diverge from the default documented behavior. The default would still be to start the preprocessor on inclusion. Would that be OK too?

adamwathan commented 10 years ago

Yeah I mean I wouldn't want it to always be automatically autoloaded with the stream wrapper setup on every request (even requests that aren't just me running a test).

I guess my general feeling about the whole thing is I like all my code to be autoloaded so it's available to me, but I don't necessarily wish for any code to actually be executed just because I autoloaded the library.

When I first tried out the library, I was manually requiring Patchwork.php at the beginning of any tests where I needed to monkey patch, and not including it anywhere else. This was getting a bit messy, so I thought it would be nicer if everything was just available via the autoloader.

Basically the difference between having to do this on certain select tests:

public function setUp()
{
    require __DIR__ . "/../../vendor/antecedent/patchwork/Patchwork.php":
}

...vs this:

public function setUp()
{
    Patchwork\start();
}

Especially kind of annoying if I have some tests that are nested in 2 subdirectories and some that are nested in 1, so the require line is different for each one. Manually requiring things in general just doesn't feel right to me anyways when trying to use composer for everything.

It sounds like I might be worried about nothing though and that it would be fine to automatically load Patchwork on every single request even if it's only needed for a few specific tests, so if that's the case no problem.