goaop
The "\Go\ParserReflection\ReflectionEngine::locateClassFile" doesn't work with built-in classes
When attempting to use \Go\ParserReflection\ReflectionEngine::locateClassFile on a built-in class (for them \ReflectionClass::getFileName() returns false), then exception about not found class is thrown.
I'm proposing to show Locating of built-in classes is not supported. exception message, when attempting to find built-in class.
I can agree with this issue, but I have another vision: enable support for existing classes in all Reflection classes, so if class or function is already exists, then we can create a proxy for original reflection and forward all method calls to the internal reflection instances.
What do your think?
Not sure really how such proxy can't be implemented without overriding each method in each reflection class once again.
@lisachenko, I hope this isn't starting to sound like a tired refrain, but I do now have a working implementation of this "pass through mode" you described here. I'm sure if you had some code to look at, this would be a lot more welcome comment. While I am still waiting on permission to share my changes, I wanted to reassure you as to what I anticipate the chain of events will look like.
I've been working for a couple months on modifying AspectMock to allow effective unit testing of my client's Drupal 7 based custom modules. I am very close to having a first pass working solution. Neither my boss nor I are in a good situation to request permission to contribute my changes back upstream until I can demonstrate a cost benefit to the organization. Given my changes are reasonably extensive, I think the cost advantage of not supporting a custom fork are pretty clear; I really just need to demonstrate that my changes are an effective tool for testing our code, which management already sees the value in. I'm hoping to have management endorsed code to contribute back in the next 2-4 weeks.
That said, that brings me to why I'm writing today:
So, the main advantage that I see of having this pass through working, is to allow a consistent interface for reflections of builtin, user defined, and not-yet-loaded classes/functions/etc. Once these custom Reflection class implentations work with builtin's it's natural to want to extend them. To this end, I have implemented a ReflectionFactory to allow any parsed reflection class to be easily be replaced with a derived class. This, unfortunately adds a bit more complexity, but I believe the benefit is worth it. This allows, for instance, AspectMock to easily add a isUnderAspectMockControl() method to it's custom ReflectionClass without modifying this package at all. To do this cleanly I changed all ReflectionEngine methods from static to instance methods (with a compatibility shim for public methods), and made a ReflectionEngine instance a property of ReflectionFactory.
I look forward to hearing your feedback on these design changes.
Hi, @loren-osborn! I understand this situation with private source code modification. Unfortunately, if your heads requested you to keep this code - it's ok. But there is no any price and valuable changes that cost a lot, especially for standalone component, like this.
So, you could try to convenience your boss that publishing these patches won't harm your business at all.
According to described changes, I think that several requests for the factory should be satisfied. So ReflectionFactory is ok. Configuration of this factory should be performed by default in the src/bootstrap.php or configured statically in the class and can be changed in runtime with ReflectionEngine::setClassMap(array $mappings) method. Then anyone will be able to create children classes and use them for own needs.
I also like the whole idea to make this library working with original reflection classes, however this will flood a code with a lot of typical lines if ($this->isInitialized) { parent::xxx() }. But it's a price for making library working with system classes too.
I can't see any value in changing ReflectionEngine to dynamic instance. It will be better to have only one single instance and ability to change it/mock it. In this case ReflectionEngine will be the single source of truth which is good.
@lisachenko, yes, I will, of course honor the wishes of my employer, in contributing to this project (and others) on company time; I was trying to give you my best guess what management will decide and when. I expect they want us to do some code grooming before posting any code for comment, and the prime priority now is to demonstrate a company benefit from testing.
I currently don't have a unified ReflectionFactory::setClassMap(), as I suspect the typical use case would only involve extending very few classes, but it would be an easy addition to make. The alternate classes are easily set in a module's initialization code (be it bootstrap.php or some other initializer). One benefit of multiple factory instances is that different modules, with their own incompatible derived parsed reflection classes, can both coexist, as long as each module's own ReflectionFactory instance is used to instantiate parsed reflection objects.
I was relatively surprised how few methods needed to defer to parent method implementations, given how complete your wrapper implementation is, although to minimize changes in behavior, instead of checking $this->isInitialized, I used a wrapper that evaluates to if ($this->getFileName() === FALSE) { ... } as my condition.
As far as making ReflectionEngine into a ReflectionFactory owned instance, I have mixed feelings. I feel the functionality is too complex to not allow 3rd parties to override it, which is certainly inelegant with static methods. On the other hand I agree with your desire for a "single source of truth" with the parsed result cache (and the construct → filename cache that I've added and hope to share) but want to limit constraints on derived classes. I'm not clear yet on the best solution yet, but (mostly thinking out loud) I'm imagining splitting ReflectionEngine into 2 or 3 classes:
-
ReflectionEngine, as an instance, would retain all functionality. -
ReflectionCache, would get all the static data fromReflectionEngine, and would be sharable between factory instances, but can determine what is/isn't cached in a way a derived class could override. -
ReflectionCachedItem, would allow parent and derived classes to share cached data items without agreeing on what is or isn't cached.
This is just a rough idea, and I welcome suggestions.
I just realized that most of my recent discussion on this thread is really related to issue #2