implementsInterface() isSubclassOf() method inconsistancies in ReflectionClass

[loren-osborn]

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 returning FALSE.

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 returning FALSE when they should return TRUE.
• Lines with a yellow (marked with ..) background are not throwing an exception that the native ReflectionClass method throws. This may or may not be the expected behavior. (I suggest that Go\ParserReflection\ReflectionClass should throw an exception as well, but that's my personal opinion.)

This is what my output looks like::

[loren-osborn]

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.)

[lisachenko]

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.

[lisachenko]

Could you please check the branch fix/is-subclass-of?

I've added additional checks to make isSubclassOf method compatible with original reflection.

[loren-osborn]

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

[loren-osborn]

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.

[loren-osborn]

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.

[loren-osborn]

I am attaching a screenshot of how the output should look when these two methods are functioning properly.

[loren-osborn]

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.