# Reflection to remove final - Thread: https://externals.io/message/57381 - Original archive: https://news-web.php.net/php.internals/57381 - Started by Jan Dolecek on 2012-01-16T18:06:31+00:00 - Messages: 20 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #57381 — Jan Dolecek — 2012-01-16T18:06:31+00:00 URL: https://externals.io/message/57381#57381 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 --- ## #57384 — Sebastian Bergmann — 2012-01-17T10:22:06+00:00 URL: https://externals.io/message/57381#57384 · In reply to: #57381 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/ --- ## #57385 — Pierre Joye — 2012-01-17T10:34:16+00:00 URL: https://externals.io/message/57381#57385 · In reply to: #57381 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? > Thanks > > Jan Dolecek > juzna.cz@gmail.com > > -- > > > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #57386 — Sebastian Bergmann — 2012-01-17T10:54:09+00:00 URL: https://externals.io/message/57381#57386 · In reply to: #57385 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/ --- ## #57387 — jpauli — 2012-01-17T11:00:21+00:00 URL: https://externals.io/message/57381#57387 · In reply to: #57386 +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: > > […] > > 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/ --- ## #57388 — jpauli — 2012-01-17T11:03:33+00:00 URL: https://externals.io/message/57381#57388 · In reply to: #57387 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 wrote: > > > […] --- ## #57389 — Sebastian Bergmann — 2012-01-17T11:27:43+00:00 URL: https://externals.io/message/57381#57389 · In reply to: #57387 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/ --- ## #57390 — Sebastian Bergmann — 2012-01-17T11:28:51+00:00 URL: https://externals.io/message/57381#57390 · In reply to: #57388 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/ --- ## #57391 — Jan Dolecek — 2012-01-17T12:40:57+00:00 URL: https://externals.io/message/57381#57391 · In reply to: #57388 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 > > > > > […] --- ## #57392 — Laruence — 2012-01-17T12:54:51+00:00 URL: https://externals.io/message/57381#57392 · In reply to: #57384 > Am 16.01.2012 19:06, schrieb Jan Dolecek: > > […] > >  +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/ --- ## #57393 — Ferenc Kovacs — 2012-01-17T13:01:26+00:00 URL: https://externals.io/message/57381#57393 · In reply to: #57392 > > 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 --- ## #57394 — Jan Dolecek — 2012-01-17T13:07:47+00:00 URL: https://externals.io/message/57381#57394 · In reply to: #57393 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 > > […] > reflection should always be used with care. > > -- > Ferenc Kovács > @Tyr43l - http://tyrael.hu --- ## #57395 — Nikita Popov — 2012-01-17T13:09:07+00:00 URL: https://externals.io/message/57381#57395 · In reply to: #57381 > 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 --- ## #57396 — Jan Dolecek — 2012-01-17T13:11:54+00:00 URL: https://externals.io/message/57381#57396 · In reply to: #57395 > 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 --- ## #57397 — Jan Dolecek — 2012-01-17T13:15:18+00:00 URL: https://externals.io/message/57381#57397 · In reply to: #57390 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: > > […] > >  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/ --- ## #57398 — Pierre Joye — 2012-01-17T13:31:39+00:00 URL: https://externals.io/message/57381#57398 · In reply to: #57397 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.com > > > > > > […] > > -- > > > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #57402 — Gustavo Lopes — 2012-01-17T21:27:07+00:00 URL: https://externals.io/message/57381#57402 · In reply to: #57381 > 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 --- ## #57403 — Stas Malyshev — 2012-01-17T22:21:50+00:00 URL: https://externals.io/message/57381#57403 · In reply to: #57402 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 --- ## #57404 — Jan Dolecek — 2012-01-17T23:00:09+00:00 URL: https://externals.io/message/57381#57404 · In reply to: #57403 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! > > > > […] > > > 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 --- ## #57406 — Gustavo Lopes — 2012-01-18T10:35:37+00:00 URL: https://externals.io/message/57381#57406 · In reply to: #57404 > 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