Hi,
reflection is a great tool for testing, but it still misses support
for mocking classes/methods marked as final. I created a small patch
https://gist.github.com/1621839#file_php_finals.patch, examples how to
use it https://gist.github.com/1621839#file_final_test.php and wrote a
short article explaining it:
http://blog.juzna.cz/2012/01/mock-vs-final-fights-in-testing/.
Can you include it into svn pls?
Thanks
Jan Dolecek
juzna.cz@gmail.com
Am 16.01.2012 19:06, schrieb Jan Dolecek:
reflection is a great tool for testing, but it still misses support
for mocking classes/methods marked as final. I created a small patch
https://gist.github.com/1621839#file_php_finals.patch, examples how to
use it https://gist.github.com/1621839#file_final_test.php and wrote a
short article explaining it:
http://blog.juzna.cz/2012/01/mock-vs-final-fights-in-testing/.
+1 from me for the idea. Implementation looks good at first glance as
well, but I let more knowledgable C developers be the judge of that.
--
Sebastian Bergmann Co-Founder and Principal Consultant
http://sebastian-bergmann.de/ http://thePHP.cc/
Am 16.01.2012 19:06, schrieb Jan Dolecek:
reflection is a great tool for testing, but it still misses support
for mocking classes/methods marked as final. I created a small patch
https://gist.github.com/1621839#file_php_finals.patch, examples how to
use it https://gist.github.com/1621839#file_final_test.php and wrote a
short article explaining it:
http://blog.juzna.cz/2012/01/mock-vs-final-fights-in-testing/.+1 from me for the idea. Implementation looks good at first glance as
well, but I let more knowledgable C developers be the judge of that.
if a class is marked as final, there must be some reason for it to be a final.
if you remove the final and extends it, may lead to bad error.
thanks
--
Sebastian Bergmann Co-Founder and Principal Consultant
http://sebastian-bergmann.de/ http://thePHP.cc/--
--
Laruence Xinchen Hui
http://www.laruence.com/
if a class is marked as final, there must be some reason for it to be a
final.if you remove the final and extends it, may lead to bad error.
reflection should always be used with care.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Yes, there always is a reason. But when we test and create mocks, this
reasons can go aside. This is the same with
ReflectionProperty::setAccessible() etc.
They shouldn't be used normally, but in special cases like testing
it's necessary.
Jan Dolecek
juzna.cz@gmail.com
if a class is marked as final, there must be some reason for it to be a
final.if you remove the final and extends it, may lead to bad error.
reflection should always be used with care.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
hi Jan!
Patch looks good, but coding standard.
Please always use brackets, even for one line condition (if () foo else bar).
Cheers,
Hi,
reflection is a great tool for testing, but it still misses support
for mocking classes/methods marked as final. I created a small patch
https://gist.github.com/1621839#file_php_finals.patch, examples how to
use it https://gist.github.com/1621839#file_final_test.php and wrote a
short article explaining it:
http://blog.juzna.cz/2012/01/mock-vs-final-fights-in-testing/.Can you include it into svn pls?
ThanksJan Dolecek
juzna.cz@gmail.com--
--
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Am 17.01.2012 11:34, schrieb Pierre Joye:
Patch looks good, but coding standard.
If there are no objections, I can fix the CS issue and commit the patch
to trunk.
--
Sebastian Bergmann Co-Founder and Principal Consultant
http://sebastian-bergmann.de/ http://thePHP.cc/
+1, I like the idea as well.
How didn't we think about that earlier... ?
Julien
On Tue, Jan 17, 2012 at 11:54 AM, Sebastian Bergmann sebastian@php.netwrote:
Am 17.01.2012 11:34, schrieb Pierre Joye:
Patch looks good, but coding standard.
If there are no objections, I can fix the CS issue and commit the patch
to trunk.--
Sebastian Bergmann Co-Founder and Principal Consultant
http://sebastian-bergmann.de/ http://thePHP.cc/
Just one last word : The patch just adds setFinal() to ReflectionClass.
What about ReflectionMethod ? Not very usefull for the mocking purpose, but
that would be good for the Reflection API.
Julien
+1, I like the idea as well.
How didn't we think about that earlier... ?Julien
On Tue, Jan 17, 2012 at 11:54 AM, Sebastian Bergmann sebastian@php.netwrote:
Am 17.01.2012 11:34, schrieb Pierre Joye:
Patch looks good, but coding standard.
If there are no objections, I can fix the CS issue and commit the patch
to trunk.--
Sebastian Bergmann Co-Founder and Principal Consultant
http://sebastian-bergmann.de/ http://thePHP.cc/
Am 17.01.2012 12:03, schrieb jpauli:
The patch just adds setFinal() to ReflectionClass.
What about ReflectionMethod?
From the patch
+/* {{{ proto public void ReflectionMethod::setFinal([bool isFinal = true])
- Sets/unsets class as final */
+ZEND_METHOD(reflection_method, setFinal)
+/* {{{ proto public void ReflectionClass::setFinal([bool isFinal = true])
- Sets/unsets class as final */
+ZEND_METHOD(reflection_class, setFinal)
TL;DR: All is well :)
--
Sebastian Bergmann Co-Founder and Principal Consultant
http://sebastian-bergmann.de/ http://thePHP.cc/
Sebastian, thanks for the commit. But actually as I see the tests, I
may have not specified it enough:
ReflectionClass::setFinal(false) removes the final from the class, not
it's methods.
This test case
http://svn.php.net/viewvc/php/php-src/trunk/ext/reflection/tests/ReflectionClass_setFinal.phpt?view=markup&pathrev=322390
should have:
final class a { // class is final
public function b() { // method is not
...
}
}
Thanks
Jan Dolecek
juzna.cz@gmail.com
Am 17.01.2012 12:03, schrieb jpauli:
The patch just adds setFinal() to ReflectionClass.
What about ReflectionMethod?From the patch
+/* {{{ proto public void ReflectionMethod::setFinal([bool isFinal = true])
- Sets/unsets class as final */
+ZEND_METHOD(reflection_method, setFinal)+/* {{{ proto public void ReflectionClass::setFinal([bool isFinal = true])
- Sets/unsets class as final */
+ZEND_METHOD(reflection_class, setFinal)TL;DR: All is well :)
--
Sebastian Bergmann Co-Founder and Principal Consultant
http://sebastian-bergmann.de/ http://thePHP.cc/
As it is not that bad for trunk, I think we should wait a bit before
commiting this patch, there are still need for discussions and we
should give some time to other people to comment the feature (the
implementation is trivial as this stage). Sebastian, mind to revert it
until we are done with the discussions?
Sebastian, thanks for the commit. But actually as I see the tests, I
may have not specified it enough:ReflectionClass::setFinal(false) removes the final from the class, not
it's methods.This test case
http://svn.php.net/viewvc/php/php-src/trunk/ext/reflection/tests/ReflectionClass_setFinal.phpt?view=markup&pathrev=322390
should have:final class a { // class is final
public function b() { // method is not
...
}
}Thanks
Jan Dolecek
juzna.cz@gmail.comAm 17.01.2012 12:03, schrieb jpauli:
The patch just adds setFinal() to ReflectionClass.
What about ReflectionMethod?From the patch
+/* {{{ proto public void ReflectionMethod::setFinal([bool isFinal = true])
- Sets/unsets class as final */
+ZEND_METHOD(reflection_method, setFinal)+/* {{{ proto public void ReflectionClass::setFinal([bool isFinal = true])
- Sets/unsets class as final */
+ZEND_METHOD(reflection_class, setFinal)TL;DR: All is well :)
--
Sebastian Bergmann Co-Founder and Principal Consultant
http://sebastian-bergmann.de/ http://thePHP.cc/--
--
--
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
setFinal is available for both classes and methods, as shown in
example in my gist
(https://gist.github.com/1621839#file_final_test.php). This should be
enough for all possible mocks, as I discussed it with my friends.
It is also possible to remove final, create a mock class and then put
it back, if someone wants to keep it after mocks have been created.
Sorry about my CS, I'll be more careful next time. I wanted to make a
proof of concept quickly to try it in PHP.
Jan Dolecek
juzna.cz@gmail.com
Just one last word : The patch just adds setFinal() to ReflectionClass.
What about ReflectionMethod ? Not very usefull for the mocking purpose, but
that would be good for the Reflection API.Julien
+1, I like the idea as well.
How didn't we think about that earlier... ?Julien
On Tue, Jan 17, 2012 at 11:54 AM, Sebastian Bergmann sebastian@php.netwrote:
Am 17.01.2012 11:34, schrieb Pierre Joye:
Patch looks good, but coding standard.
If there are no objections, I can fix the CS issue and commit the patch
to trunk.--
Sebastian Bergmann Co-Founder and Principal Consultant
http://sebastian-bergmann.de/ http://thePHP.cc/
Am 17.01.2012 12:00, schrieb jpauli:
How didn't we think about that earlier... ?
I have had this on a TODO list at the back of my head for a long time;
at least since we added setAccessible().
--
Sebastian Bergmann Co-Founder and Principal Consultant
http://sebastian-bergmann.de/ http://thePHP.cc/
Hi,
reflection is a great tool for testing, but it still misses support
for mocking classes/methods marked as final. I created a small patch
https://gist.github.com/1621839#file_php_finals.patch, examples how to
use it https://gist.github.com/1621839#file_final_test.php and wrote a
short article explaining it:
http://blog.juzna.cz/2012/01/mock-vs-final-fights-in-testing/.Can you include it into svn pls?
Thanks
Why did you choose to make the argument to setFinal() optional?
setAccessible() doesn't do this.
Nikita
Why did you choose to make the argument to setFinal() optional?
setAccessible() doesn't do this.
To be honest I didn't check the setAccessible method first. It just
seemed natural to me, that $refl->setFinal() makes it final even
without true as an argument.
Jan Dolecek
juzna.cz@gmail.com
Hi,
reflection is a great tool for testing, but it still misses support
for mocking classes/methods marked as final. I created a small patch
https://gist.github.com/1621839#file_php_finals.patch, examples how to
use it https://gist.github.com/1621839#file_final_test.php and wrote a
short article explaining it:
http://blog.juzna.cz/2012/01/mock-vs-final-fights-in-testing/.
This doesn't seem right. Correct me if I'm wrong: for internal classes,
their data structures are allocated permanently, so I'd say the effects of
removing the flag would be permanent (i.e., would affect subsequent
requests). And for threaded builds this could cause races between the
threads.
--
Gustavo Lopes
Hi!
This doesn't seem right. Correct me if I'm wrong: for internal classes,
their data structures are allocated permanently, so I'd say the effects of
removing the flag would be permanent (i.e., would affect subsequent
requests). And for threaded builds this could cause races between the
threads.
It'd probably also mess up bytecode caches, since they permanently store
class definitions.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
This issue seems much more complicated than I thought. We'll need to
consider all cases which could cause troubles and have a solution for
them.
Namely:
- rewriting permanent structures for internal classes, which are
being kept between requests, must be avoided - races in threading models
- rewriting in bytecode caches must be avoided
- anything else in mind?
I'll try to think about it, but I'm not that experienced with php
internals, so any suggestions welcome.
Jan Dolecek
juzna.cz@gmail.com
Hi!
This doesn't seem right. Correct me if I'm wrong: for internal classes,
their data structures are allocated permanently, so I'd say the effects of
removing the flag would be permanent (i.e., would affect subsequent
requests). And for threaded builds this could cause races between the
threads.It'd probably also mess up bytecode caches, since they permanently store
class definitions.Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
This issue seems much more complicated than I thought. We'll need to
consider all cases which could cause troubles and have a solution for
them.Namely:
- rewriting permanent structures for internal classes, which are
being kept between requests, must be avoided- races in threading models
- rewriting in bytecode caches must be avoided
- anything else in mind?
I'll try to think about it, but I'm not that experienced with php
internals, so any suggestions welcome.
I think you should approach it in a different way. Don't remove the final
modifier, just ignore it in some circumstances (e.g. interface,
annotation). PHP doesn't support annotations (at the engine level) and
interface processing is currently done after checking the final class
flag; additionally, there may be some code that relies final classes not
having subclasses to this is probably still not going to be trivial.
--
Gustavo Lopes