parser-reflection
parser-reflection copied to clipboard
implementsInterface() isSubclassOf() method inconsistancies in ReflectionClass
I'm finding differences in how Go\ParserReflection\ReflectionClass
and native ReflectionClass
behave for methods implementsInterface()
and isSubclassOf()
. I'm using some fakery to simulate external file loading, so I would be unsurprised if the issue involves how I'm populating these files, but I don't currently see anything I missing in that regard.
This is a couple pages of code, so it's hard to call it a "minimal reproducible test case", but I wrote it to be self-contained, and to demonstrate the issue I'm seeing in my code. I hope my comments are adequate, but I opted for clarity of the output over clarity of the code. This is why I chose to use ANSI color codes to make the issue easier to see.
As far as the issue I'm seeing in my actual codebase, I'm seeing the following behavior:
-
implementsInterface()
is working as expected. -
isSubclassOf()
is always returningFALSE
.
Here is a gist of the example code I'm using: https://gist.github.com/loren-osborn/b3a80c075ebb3ac6d3e2d8749a23bbe7 :
- Lines in green are working as expected.
- Lines in red (marked with
!!
) are returningFALSE
when they should returnTRUE
. - Lines with a yellow (marked with
..
) background are not throwing an exception that the nativeReflectionClass
method throws. This may or may not be the expected behavior. (I suggest thatGo\ParserReflection\ReflectionClass
should throw an exception as well, but that's my personal opinion.)
This is what my output looks like::
I actually have developed a fix for this, but:
- ~~My earliest changes that resolved this issue didn't properly account for classes that had already been loaded and broke the existing unit tests.~~ My updated changes resolved this.
- I still need to convert this mini test suite into unit tests to add to PHPUnit
- I am awaiting permission before I can release any changes made on company time. (I think the odds of me getting permission soon are reasonably high.)
Hi, @loren-osborn!
Thank you for reporting this issue. Of course, sometimes it can be very hard to emulate original reflection behavior, so I suppose that there are issues in my library. Some of them are important and should be fixed, whereas some of them aren't.
So, if you can provide broken PHPUnit test with PR then I'll be happy to review it and merge fix.
From what I can see, there is an issue in the goaop/parser-reflection
with checking interfaces for isSubclassOf()
. Original source code can be very simple: https://3v4l.org/S32TE.
Could you please check the branch fix/is-subclass-of
?
I've added additional checks to make isSubclassOf
method compatible with original reflection.
It looks like there are a few edge cases you missed, including parent interface of an interface. I'll try to update the gist with a more complete set of tests when I get to work. Unfortunately I don't have time now to make it easy to read. 😕 sorry
One thing I saw that I did differently in my tests, was that I collected all my library behavior data before including any of my PHP files containing the code I was analyzing. Then I included the code and ran the same tests with the native \ReflectionClass
class, comparing the results at the end. I realize this is a bit counter to the way PHPUnit typically works, but I think it's a helpful test to run in this case, as you need to see how this implementation works without the code loaded.
Ok... I updated the the public Gist to the more complete one that I ended up using. The code isn't pretty and would need some heavy refactoring if it wasn't going to be discarded, but it should help you get to a clean-room solution, if you want to do it yourself. Please let me know if this is helpful.
I am attaching a screenshot of how the output should look when these two methods are functioning properly.
I'm not sure that this makes for the cleanest code, but I found the easiest way to mimic the native ReflectionExceptions
is to catch the current Exception, verify it's the one we expect, and throw a new Exception altered to match the native one (including the original Exception as the $previous
param). Please let me know if there's anything else I can do to help.