Add restore method for aliasing functions

[dingo-d]

This PR is the result of me writing an issue seeing if this is solved or not, then realizing I should dig a bit into the codebase, and after digging realizing I should just try to give it a go, to save the maintainer some time.

Background

In my tests, for some specific instances, I needed to return false on the file_exists function, so I just did

when('file_exists')->alias(function($argument) {
  // Some logic that returns false based on the argument. 
});

This, of course, 'polluted' the rest of the tests, so I needed to add

when('file_exists')->justReturn(true);

This way I won't get false returned, but it's still not ideal, as now the rest of my tests that may depend on the file_exists function will always return true.

What I tried to do was:

when('file_exists')->reset();

This is not working. After checking the codebase, this happens because what is returned isn't a container, but a FunctionStub instance, which doesn't have the reset method on it.

Problem

Patchwork has the restore function, but that function expects the instance of Patchwork\CallRerouting\Handle and not the string of the patched function. So I cannot just do Patchwork\restore('file_exists') in my code (tried it and it errored out).

Solution

This PR 😄

I've added the restore method to the FunctionStub class. For it to work correctly I need to keep track of all the handles created when using Patchworks' redefine function. That's why I created a $handles property that will store all the patched functions so that we can easily restore them during testing.

So now if you have in your test

when('file_exists')->alias(function($argument) {
  // Some logic that returns false based on the argument. 
});

You can just do

when('file_exists')->restore();

And the original functionality of the patched function will be back. Note that for this example (and the test added in the PR) you need to add the patchwork.json because we are redefining PHP internal function.

The changes are covered with a test (not ideal, I know).

I'm open for adding more tests, so if you can think of any additional test cases I'm all for adding them 🎉

[gmazzap]

Thanks @dingo-d!

At first glance it looks very fine. I'm just back from a 2 weeks vacation so I have piles of staff on my desk at work. Will try to properly review as soon as I can.

[gmazzap]

@dingo-d First feedback without looking at the actual code, but just reading you description.

---

Do you teardown?

Brain Monkey teardown() function calls Patchwork\restoreAll(), so if you have that function called at the end of every test (as recommended) then you don't need to restore each single redefinition.

So the workflow I always followed is:

1. alias a function in a test
2. run teardown() at the end of the test (in PHPUnit this is usually done via tearDown() method on a base test class)
3. run next test

At step 3 the redefinition done at step 1 is not in effect anymore.

What happen in the next test?

Defined functions

At step 3 above, if the function is defined during tests execution (like file_exists), it will work as usual.

Undefined functions

If the function is not defined during tests execution (e.g. a WordPress function when WordPress is not loaded), Brain Monkey had defined it before replacing it via Patchwork.

In fact, Brain Monkey defines a function that triggers an error, and then replaces it with whatever alias you define.

That means at step 3 above the function will be back being the function that triggers an error.

There are two scenarios:

• the function is not called by the executed by the next test (after teardown())
• the function is called by the code executed by next test (after teardown())

In the case number 1, that is unrelevant: the function triggers an error but nothing calls it, so that's fine.

In the case number 2, because we are talking of a function that is not defined during tests execution, but the code under test calls it, you are required to provide an alias, and if you don't do it the function will trigger an error, making the test fail, and that is expected and correct behavior.

In short...

if you call teardown() at the end of every test as recommended, the need to "restore" a single replacement is just not there.

Edge cases

The only edge case would be that you want to replace an exiting function, and then in the same test you want it to behave back as the original. For example:

/** @test */
function it_test_something()
{
   when('file_exist')->justReturn(false);

   this_is_a_function_that_calls_replaced_file_exist();

   when('file_exists')->restore();

   this_is_a_function_that_calls_original_file_exist();
}

To be honest, I think this is not common at all, and in all these years using Brain Monkey I never had the need for it.

And I think (I never tried) you could accomplish the same by doing:

function it_test_something()
{
   when('file_exist')->justReturn(false);

   this_is_a_function_that_calls_replaced_file_exist();

   when('file_exists')->alias(fn() => \Patchwork\relay());

   this_is_a_function_that_calls_original_file_exist();
}

Did I miss anything?

Is there anything obvious I'm missing? Did I get the issue completely wrong?

[dingo-d]

Hi @gmazzap! Thanks for the extensive answer. I know that I could teardown the internal redefinition after each test, but I think why I made this because I needed to alias the internal function and then restore it inside one test file. I didn't think of using the when('file_exists')->alias(fn() => \Patchwork\relay()); trick, will need to check this out.

The PR was written some time ago and I'm not 100% sure what was the reason I wrote this 😅, but at the time I think my tests were failing because of it (or I couldn't properly test some logic in my code). I think I was redefining the function to mock the behavior of file_exists based on what the argument was, but in the same method that is using it, I needed it to be restored to the previous state. Or something to that effect.

I'll have to dig a bit deeper into what I was doing 26 days ago, and then I'll probably see if I can just fix this by either using a teardown function in the tearDown (or in my case the pest's afterEach) method.

[mbmjertan]

I've encountered similar issues when aliasing functions to what @dingo-d described, with function aliases or justReturn redefinitions didn't clear after a test (and teardown), in turn causing unexpected function definitions and behaviour in tests 👀

[gmazzap]

@mbmjertan

Redefinition is clearead after teardown (if not that's a bug on Patchwork).

What I think you're referring to is that the functions stay defined. And that's true, and this PR won't change that.

Patchwork redefines functions that are defined. So Brain Monkey takes care of defining a dummy function before allowing the re-definition via Patchwork. When you restore Patchwork re-definition, the dummy function stay there.

If you use that function again in another test, you have to re-stub it via Patchwork, or what you'll end up calling is the Brain Monkey's dummy function that will let your test fail.