Namespaced code cannot be patchworked. #4

Closed
Spudley opened this Issue Dec 10, 2012 · 13 comments

Comments

Projects
None yet
2 participants
@Spudley
Contributor

Spudley commented Dec 10, 2012

I'm having trouble using patchwork with namespaced code. It seems that it doesn't support it. None of the documentation mentions namespaces at all.

(This is problematic for me, since we're moving all our code to use namespaces).

@ghost ghost assigned antecedent Dec 10, 2012

@antecedent

This comment has been minimized.

Show comment
Hide comment
@antecedent

antecedent Dec 10, 2012

Owner

Hi Spudley,

While it's true that there's no mention of namespaces in the documentation (and that's a shame), I wasn't able to get any code that uses Patchwork with namespaced functions to misbehave, so it would be of great help to see a particular snippet that doesn't work as expected.

Also, just as a passing thought, are you sure that you're not trying to patch code that is in the same file from which Patchwork.php is included? I've stated in the Getting Started page that this won't work, though I could probably have done a better job at making that clear.

Owner

antecedent commented Dec 10, 2012

Hi Spudley,

While it's true that there's no mention of namespaces in the documentation (and that's a shame), I wasn't able to get any code that uses Patchwork with namespaced functions to misbehave, so it would be of great help to see a particular snippet that doesn't work as expected.

Also, just as a passing thought, are you sure that you're not trying to patch code that is in the same file from which Patchwork.php is included? I've stated in the Getting Started page that this won't work, though I could probably have done a better job at making that clear.

@Spudley

This comment has been minimized.

Show comment
Hide comment
@Spudley

Spudley Dec 14, 2012

Contributor

Thanks for the reply. I used basically the same code as I already had working for non-namespaced code, but just added the namespaces, and as far as I can tell it no longer replaces the functions. No errors, but the original code is still called.

If it works for you, then it's quite likely I've done something silly so I'll have another look at it and let you know. (might not be straight away though)

Contributor

Spudley commented Dec 14, 2012

Thanks for the reply. I used basically the same code as I already had working for non-namespaced code, but just added the namespaces, and as far as I can tell it no longer replaces the functions. No errors, but the original code is still called.

If it works for you, then it's quite likely I've done something silly so I'll have another look at it and let you know. (might not be straight away though)

@antecedent

This comment has been minimized.

Show comment
Hide comment
@antecedent

antecedent Dec 14, 2012

Owner

Just caught it — leading backslashes are the problem. Patchwork\replace doesn't complain if the name of the function to be patched begins with a backslash, yet this makes the patch never run. The quick fix would, of course, be to remove all leading backslashes in calls to Patchwork\replace manually. As for the actual fix to make it ignore the backslashes, I'll probably push it later today. Sorry for any inconvenience caused by this bug!

Owner

antecedent commented Dec 14, 2012

Just caught it — leading backslashes are the problem. Patchwork\replace doesn't complain if the name of the function to be patched begins with a backslash, yet this makes the patch never run. The quick fix would, of course, be to remove all leading backslashes in calls to Patchwork\replace manually. As for the actual fix to make it ignore the backslashes, I'll probably push it later today. Sorry for any inconvenience caused by this bug!

@antecedent

This comment has been minimized.

Show comment
Hide comment
@antecedent

antecedent Dec 14, 2012

Owner

I've pushed the fix now and tagged it as version 1.1.3. Please try it and tell if it resolves your issue.

Owner

antecedent commented Dec 14, 2012

I've pushed the fix now and tagged it as version 1.1.3. Please try it and tell if it resolves your issue.

@Spudley

This comment has been minimized.

Show comment
Hide comment
@Spudley

Spudley Dec 19, 2012

Contributor

Hia. I've had a chance to test it, and I have good news and bad news.

The good news is that straightforward patching of a namespaced method does now work -- creating an object of that type and calling the relevant method results in the patchworked method being called.

This is great, as it resolves the immediate issue I was having.

However, that bad news is that digging a bit deeper gave me additional related problems:

If I have a class that extends the class being patchworked, calling the patchworked method from an object of the extending class gives me the original non-patchworked method. This doesn't seem to work even without namespacing, though I could have sworn it used to work.

class boobar {
public function flub() { print "no patchworking here";}
}
class foobar extends boobar {}

\Patchwork\replace("boobar::flub", function() { print 'patchwork is working'; });
$testme = new boobar();
$testme->flub(); //prints 'patchwork is working'
$testme = new foobar();
$testme->flub(); //prints no patchworking here'

Similar results if namespaces are introduced (I also tried it with different namespaces for the two classes, but of course if the simple case isn't working there's no reason why it should work for the more complex case)

This may be better off being split into a separate ticket, as I don't think it's directly related to the original issue, but as I say, I thought it was working previously.

Thanks for everything!

Contributor

Spudley commented Dec 19, 2012

Hia. I've had a chance to test it, and I have good news and bad news.

The good news is that straightforward patching of a namespaced method does now work -- creating an object of that type and calling the relevant method results in the patchworked method being called.

This is great, as it resolves the immediate issue I was having.

However, that bad news is that digging a bit deeper gave me additional related problems:

If I have a class that extends the class being patchworked, calling the patchworked method from an object of the extending class gives me the original non-patchworked method. This doesn't seem to work even without namespacing, though I could have sworn it used to work.

class boobar {
public function flub() { print "no patchworking here";}
}
class foobar extends boobar {}

\Patchwork\replace("boobar::flub", function() { print 'patchwork is working'; });
$testme = new boobar();
$testme->flub(); //prints 'patchwork is working'
$testme = new foobar();
$testme->flub(); //prints no patchworking here'

Similar results if namespaces are introduced (I also tried it with different namespaces for the two classes, but of course if the simple case isn't working there's no reason why it should work for the more complex case)

This may be better off being split into a separate ticket, as I don't think it's directly related to the original issue, but as I say, I thought it was working previously.

Thanks for everything!

@antecedent

This comment has been minimized.

Show comment
Hide comment
@antecedent

antecedent Dec 19, 2012

Owner

I'm profoundly sorry, but I'll have to keep my reply short and not as informative as I'd like. This is due to the unfortunate fact that I'm having major problems with my health, which quite heavily involve the brain, and have been having them for an alarmingly long time — almost a year now. Writing is very difficult for me now, and even more so is coding.

I'll just say that the behavior of Patchwork you described is the intended behavior, as counter-intuitive as it is. It's just that to the best of my knowledge, there's no reasonably simple way to efficiently make it work the way one would intuitively expect it to work. It's definitely possible, but it would require some really involved coding that I'm really not capable of now, and it's uncertain if I'll ever be. Also, while I'm by no means blaming you for not reading deep enough into the documentation, it actually includes a mention of this:

Patchwork ignores class inheritance when redefining methods. This means that if A::foo is redefined, and B extends A, then B::foo is not redefined, even when B preserves the original implementation of foo.

Currently, I can only hope that you aren't dealing with a very large number of subclasses and that you'll be able to apply your patches to each of them separately without your code turning into pasta.

I appreciate your understanding, and thank you for your interest in Patchwork.

Owner

antecedent commented Dec 19, 2012

I'm profoundly sorry, but I'll have to keep my reply short and not as informative as I'd like. This is due to the unfortunate fact that I'm having major problems with my health, which quite heavily involve the brain, and have been having them for an alarmingly long time — almost a year now. Writing is very difficult for me now, and even more so is coding.

I'll just say that the behavior of Patchwork you described is the intended behavior, as counter-intuitive as it is. It's just that to the best of my knowledge, there's no reasonably simple way to efficiently make it work the way one would intuitively expect it to work. It's definitely possible, but it would require some really involved coding that I'm really not capable of now, and it's uncertain if I'll ever be. Also, while I'm by no means blaming you for not reading deep enough into the documentation, it actually includes a mention of this:

Patchwork ignores class inheritance when redefining methods. This means that if A::foo is redefined, and B extends A, then B::foo is not redefined, even when B preserves the original implementation of foo.

Currently, I can only hope that you aren't dealing with a very large number of subclasses and that you'll be able to apply your patches to each of them separately without your code turning into pasta.

I appreciate your understanding, and thank you for your interest in Patchwork.

@antecedent antecedent closed this Dec 19, 2012

@Spudley

This comment has been minimized.

Show comment
Hide comment
@Spudley

Spudley Dec 20, 2012

Contributor

Hi. I'm really sorry to hear about your health issues. :-(

You're right; I hadn't remembered that it was in the docs. It's not a big deal; as you say, one can just override the same method in each class that one's going to use. And no, I don't have so many to worry about at any one time.

I only mentioned it because I stumbled on it while trying out test the previous fix (I was trying to be thorough). I didn't think to check the docs.

I want to say again that I really appreciate Patchwork -- it's a seriously impressive bit of software, and fills a big gap. It makes it possible to write useful tests for otherwise untestable code, without having to rewrite. It's saved me a lot of time. Thank you for writing it.

Contributor

Spudley commented Dec 20, 2012

Hi. I'm really sorry to hear about your health issues. :-(

You're right; I hadn't remembered that it was in the docs. It's not a big deal; as you say, one can just override the same method in each class that one's going to use. And no, I don't have so many to worry about at any one time.

I only mentioned it because I stumbled on it while trying out test the previous fix (I was trying to be thorough). I didn't think to check the docs.

I want to say again that I really appreciate Patchwork -- it's a seriously impressive bit of software, and fills a big gap. It makes it possible to write useful tests for otherwise untestable code, without having to rewrite. It's saved me a lot of time. Thank you for writing it.

@antecedent

This comment has been minimized.

Show comment
Hide comment
@antecedent

antecedent Dec 26, 2012

Owner

Well, as a stranger from the internet, you don't really have to be sorry, but thanks — that probably makes your words mean even more to me. Also thanks for letting me know that the existence of Patchwork actually made someone's life better :)

Meanwhile, I've tried to make some fixes to Patchwork in the last few days... Finally tagged them as version 1.2.0 today. Given the condition that I'm in, it's quite likely that I've messed something up in there, which is why I'd like to ask you, or anyone reading this, to test the new release on actual, real life code.

The most important change is, of course, that method patches are now heritable. I've updated the docs to reflect that:

As of version 1.2.0, Patchwork follows inheritance when redefining methods. This means that if Y extends X, then redefining X::foo also redefines Y::foo, unless the foo method is overridden in Y.

And I guess that's it for now. Merry Christmas!

Owner

antecedent commented Dec 26, 2012

Well, as a stranger from the internet, you don't really have to be sorry, but thanks — that probably makes your words mean even more to me. Also thanks for letting me know that the existence of Patchwork actually made someone's life better :)

Meanwhile, I've tried to make some fixes to Patchwork in the last few days... Finally tagged them as version 1.2.0 today. Given the condition that I'm in, it's quite likely that I've messed something up in there, which is why I'd like to ask you, or anyone reading this, to test the new release on actual, real life code.

The most important change is, of course, that method patches are now heritable. I've updated the docs to reflect that:

As of version 1.2.0, Patchwork follows inheritance when redefining methods. This means that if Y extends X, then redefining X::foo also redefines Y::foo, unless the foo method is overridden in Y.

And I guess that's it for now. Merry Christmas!

@Spudley

This comment has been minimized.

Show comment
Hide comment
@Spudley

Spudley Jan 10, 2013

Contributor

Hi there.
Firstly, sorry it's taken me a couple of weeks to respond -- things got a bit busy!

I've done a bit of testing of your changes at this end, and Patchwork is looking good; all seems to be working as expected. Looking at the current version from github, which appears to be 1.2.1.

The only thing that's a bit odd is that it's outputting the string "\Patchwork\Interceptor\applyScheduledPatches();" to the console. (This isn't causing me any problems as where I'm using it the console output isn't important, but I thought I should mention it)

Contributor

Spudley commented Jan 10, 2013

Hi there.
Firstly, sorry it's taken me a couple of weeks to respond -- things got a bit busy!

I've done a bit of testing of your changes at this end, and Patchwork is looking good; all seems to be working as expected. Looking at the current version from github, which appears to be 1.2.1.

The only thing that's a bit odd is that it's outputting the string "\Patchwork\Interceptor\applyScheduledPatches();" to the console. (This isn't causing me any problems as where I'm using it the console output isn't important, but I thought I should mention it)

@antecedent

This comment has been minimized.

Show comment
Hide comment
@antecedent

antecedent Jan 10, 2013

Owner

Whoa. The console output thing seems incredibly odd. Does this happen on PHP CLI or with a web server?

Owner

antecedent commented Jan 10, 2013

Whoa. The console output thing seems incredibly odd. Does this happen on PHP CLI or with a web server?

@Spudley

This comment has been minimized.

Show comment
Hide comment
@Spudley

Spudley Jan 10, 2013

Contributor

It's happening at the command-line. The I'm running phpUnit tests.

Contributor

Spudley commented Jan 10, 2013

It's happening at the command-line. The I'm running phpUnit tests.

@antecedent

This comment has been minimized.

Show comment
Hide comment
@antecedent

antecedent Jan 11, 2013

Owner

Successfully reproduced and fixed. Oh, and shame on me for not testing the last few releases with PHPUnit.

Owner

antecedent commented Jan 11, 2013

Successfully reproduced and fixed. Oh, and shame on me for not testing the last few releases with PHPUnit.

@Spudley

This comment has been minimized.

Show comment
Hide comment
@Spudley

Spudley Jan 11, 2013

Contributor

Fabulous. Tested this end; all okay. 👍

Contributor

Spudley commented Jan 11, 2013

Fabulous. Tested this end; all okay. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment