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

Autoloadable with a "start" method? #12

Open
adamwathan opened this issue Mar 7, 2014 · 2 comments
Open

Autoloadable with a "start" method? #12

adamwathan opened this issue Mar 7, 2014 · 2 comments

Comments

@adamwathan
Copy link

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

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

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.

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