ext/reflection #1083
ext/reflection #1083
Conversation
|
I'll tidy the commits up once it's ready for merging. |
|
Thanks for working on this! Just let us know when it's in shape and I'll take a look. |
|
55 commits was starting to look messy, so I rebased and squashed most of them. I'm about of a third of the way through the Zend ext/reflection tests. There's some that fail because of expected format for a var_dump or reliance on the way properties are returned internally, so I'll be copying those into tests outside of the zend folder. b56e6ab has been cherry-picked into #1127 |
|
There's also tests that rely on the standard extension, and the information that you can usually get out of that. I'm unsure as to where to implement that, since it doesn't look like there's any one place that defines a "standard" extension to add in the |
|
Those look like real failing tests... |
|
And in general, it would be better to split this up into multiple pulls since Github isn't very good at cherry picking from these (and even if you do it's hard to track what was done) -- we shy away from monolithic commits because it is much easier to revert clean and small ones quickly when something breaks and easier to bisect. |
|
Looking at the failing tests, and I'm not entirely sure what to do about them. The first one's from what appears to be a lazy evaluation of string concatenation with echo (echo METHOD . "($arg)\n"; with $arg undefined is echoing everything up to the ( then throwing the notice) The second one's expected: can't change accessibility of private members in repo authoritative mode. The third one looks like f_hphp_set_static_property doesn't work consistently in repo authoritative mode. I'll need to look into that. |
|
For tests for functionality that won't work in repo mode, you can (and should) add an empty .php.norepo file of the same name to skip it in repo mode. |
|
For the first one, feel free to just not move that test over too. |
|
I was thinking that it'd make sense to not allow the mutating reflection methods to work in repo authoritative mode, given that it looks like there's some optimisations done that don't play nice with it. It might just be that these optimisations only effect static variables, in which case it'll make sense to only disallow mutating those. |
|
I think that is a reasonable restriction; even if you did allow them, if it changes properties of functions that we made assumptions about, something is going to break. (Certainly with RepoAuthoritative and WholeProgram optimizations enabled something is likely to break here.) |
This function allows you to call a method declared in an ancestor of a class that has been potentially overriden in the current class.
|
I'm going to close for now since this has been idle for months. @simonwelsh, if you want to continue with this, feel free to reopen it when it is ready or create a new PR. |
|
Any progress with reflection? It's currently blocking usage of nette/nette. |
|
I can't speak for @simonwelsh on this pull request but some fixes to reflection were made in trunk over the lockdown so it's possible things have improved for you. |
|
Ok, compiling master right now, I will try again and let you know. |
| if($this->info['name'] == '__construct') { | ||
| return true; | ||
| } else { | ||
| return strcasecmp($this->info['class'], $this->_class) == 0; |
There was a problem hiding this comment.
shouldn't this include a check for the class being in the global namespace ?
Work in progress, do not merge
This is my progress so far of getting ext/reflection reaching parity with Zend.
The main change I'm currently looking for review on is the addition of the invoke_ancestor_method function in 070875b.