Grokbase › Groups › PHP › php-internals › January 2012
FAQ
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

Search Discussions

  • Sebastian Bergmann at Jan 17, 2012 at 10:22 am ⇧

    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/
  • Laruence at Jan 17, 2012 at 12:55 pm ⇧

    On Tue, Jan 17, 2012 at 6:22 PM, Sebastian Bergmann wrote:
    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/

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Laruence  Xinchen Hui
    http://www.laruence.com/
  • Ferenc Kovacs at Jan 17, 2012 at 1:01 pm ⇧

    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
  • Jan Dolecek at Jan 17, 2012 at 1:08 pm ⇧
    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


    On Tue, Jan 17, 2012 at 2:01 PM, Ferenc Kovacs wrote:

    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
  • Pierre Joye at Jan 17, 2012 at 10:34 am ⇧
    hi Jan!

    Patch looks good, but coding standard.

    Please always use brackets, even for one line condition (if () foo else bar).

    Cheers,
    On Mon, Jan 16, 2012 at 7:06 PM, Jan Dolecek wrote:
    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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Pierre

    @pierrejoye | http://blog.thepimp.net | http://www.libgd.org
  • Sebastian Bergmann at Jan 17, 2012 at 10:54 am ⇧

    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/
  • Jpauli at Jan 17, 2012 at 11:01 am ⇧
    +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 wrote:

    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/

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
  • Jpauli at Jan 17, 2012 at 11:04 am ⇧
    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
    On Tue, Jan 17, 2012 at 12:00 PM, jpauli wrote:

    +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 wrote:

    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/

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
  • Sebastian Bergmann at Jan 17, 2012 at 11:29 am ⇧

    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/
  • Jan Dolecek at Jan 17, 2012 at 1:15 pm ⇧
    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


    On Tue, Jan 17, 2012 at 12:28 PM, Sebastian Bergmann wrote:
    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/

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
  • Pierre Joye at Jan 17, 2012 at 1:31 pm ⇧
    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?
    On Tue, Jan 17, 2012 at 2:15 PM, Jan Dolecek wrote:
    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


    On Tue, Jan 17, 2012 at 12:28 PM, Sebastian Bergmann wrote:
    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/

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Pierre

    @pierrejoye | http://blog.thepimp.net | http://www.libgd.org
  • Jan Dolecek at Jan 17, 2012 at 12:41 pm ⇧
    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


    On Tue, Jan 17, 2012 at 12:03 PM, jpauli wrote:
    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
    On Tue, Jan 17, 2012 at 12:00 PM, jpauli wrote:

    +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 wrote:

    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/

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
  • Sebastian Bergmann at Jan 17, 2012 at 11:27 am ⇧

    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/
  • Nikita Popov at Jan 17, 2012 at 1:09 pm ⇧

    On Mon, Jan 16, 2012 at 7:06 PM, Jan Dolecek wrote:
    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
  • Jan Dolecek at Jan 17, 2012 at 1:12 pm ⇧

    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
  • Gustavo Lopes at Jan 17, 2012 at 9:27 pm ⇧

    On Mon, 16 Jan 2012 19:06:31 +0100, Jan Dolecek wrote:

    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
  • Stas Malyshev at Jan 17, 2012 at 10:21 pm ⇧
    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
  • Jan Dolecek at Jan 17, 2012 at 11:00 pm ⇧
    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


    On Tue, Jan 17, 2012 at 11:21 PM, Stas Malyshev wrote:
    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


    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
  • Gustavo Lopes at Jan 18, 2012 at 10:35 am ⇧

    On Wed, 18 Jan 2012 00:00:09 +0100, Jan Dolecek wrote:

    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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedJan 16, '12 at 6:06p
activeJan 18, '12 at 10:35a
posts20
users9
websitephp.net

9 users in discussion

Jan Dolecek: 6 posts Sebastian Bergmann: 4 posts Pierre Joye: 2 posts Gustavo Lopes: 2 posts Jpauli: 2 posts Nikita Popov: 1 post Ferenc Kovacs: 1 post Laruence: 1 post Stas Malyshev: 1 post

Content

People

Support

Translate

site design / logo © 2022 Grokbase