Injected factory instead of explicit "new X()" ?

[donquixote]

Imagine I wanted to extend this library, and provide an alternative implementation for ReflectionParameter. To do this, I would also have to replace ReflectionFunctionLikeTrait, to replace the "new ReflectionParameter" with something that creates my own type of reflection parameter. To do this, I would also have to replace or extend ReflectionFunction and ReflectionMethod, so they can use the modified trait. And then I would have to replace ReflectionClass, to replace the "new ReflectionMethod" statements.. etc.

This could be solved by injecting a factory into each object. Then I would only have to extend ReflectionParameter, and extend the factory class, and replace the method that creates new parameter objects.

[lisachenko]

I will think about that, but I have a feeling, that you can just override the concrete class with your own implementation, and this way is much more simpler, because you will be able to call parent::method() if needed.

Factory is a bad pattern and should be avoided if possible. Just create your own new Foo\Vendor\ReflectionClass($className) where Foo\Vendor\ReflectionClass extends Go\ParserReflection\ReflectionClass

[donquixote]

The problem is this:

$param = nwe MyReflectionParameter();
$param2 = $param->getClass()->getMethod('foo')->getParameters()[0];

Now $param2 is no longer of the type MyReflectionParameter, but it is just a regular ReflectionParameter. Currently I would have to subclass almost every class in the library, if I want $param2 to use my own custom MyReflectionParameter class.

Factory is a bad pattern and should be avoided if possible.

I don't know what kind of factory you have in mind that is so bad..

With the factory, it could look like this (a bit simplified):

trait ReflectionFunctionLikeTrait {
    private $parameterFactory;
    function __construct(ParameterFactoryInterface $parameterFactory) {
        $this->parameterFactory = $parameterFactory;
    }
    function getParameters() {
        if (!isset($this->parameters)) {
            $parameters = [];
            foreach ($this->functionLikeNode->getParams() as $parameterIndex => $parameterNode) {
                $reflectionParameter = $this->parameterFactory->buildParameter(
                    // We don't need to pass $functionName and $parameterName, these can be obtained from $declaringFunction and $parameterNode.
                    $this,
                    $parameterIndex,
                    $parameterNode
                );
                $parameters[] = $reflectionParameter;
            }
            $this->parameters = $parameters;
        }
        return $this->parameters;
    }
    ...
}

Note that ReflectionFunctionLikeTrait::__construct() only asks for a ParameterFactoryInterface. Others might ask for a MethodFactoryInterface.

In our default implementation, we might have just one factory object that implements all these interfaces. But others could have separate factories for each interface.

---

Now there is some things we can choose. What exactly should be the responsibility of the factory? What parameters should it receive?

In the snippet above, it gets all the data from the parse tree. So the ParameterFactoryInterface now depends on PhpParser. This means I cannot write a factory that uses a different parser engine.

But I think this is ok, because all the classes depend on PhpParser AST. To replace the parser engine, I would have to replace all the classes, anyway.

[donquixote]

In the snippet above, it gets all the data from the parse tree. So the ParameterFactoryInterface now depends on PhpParser. This means I cannot write a factory that uses a different parser engine.

But I think this is ok, because all the classes depend on PhpParser AST. To replace the parser engine, I would have to replace all the classes, anyway.

To make our classes agnostic of the parser and AST, we could do it like this (but I don't think it's a good idea):

trait ReflectionFunctionLikeTrait {
    private $parameterFactory;
    function __construct(
        $docComment,
        $endLine,
        $fileName,
        $name,
        ParameterProviderInterface $parameterProvider,
        $shortName,
        $startLine,
        StaticVariableProviderInterface $staticVariableProvider,
        $isClosure,
        $isGenerator,
        $returnsReference
    ) {
        $this->parameterProvider = $parameterProvider;
        [..]
    }
    function getParameters() {
        return isset($this->parameters)
            ? $this->parameters
            : $this->parameters = $this->parameterProvider->getParameters($this);
    }
    ...
}

So now everything that was previously coming from the AST node now has to be explicitly injected in the constructor (or some other way). I don't think this is desirable..

[lisachenko]

I'm going to add configurable class names into the library to have an ability to extend functionality. But I wonder why do you want to extend reflection classes and for what purposes? Maybe it will be better to put this into the library itself?

[loren-osborn]

I think I mentioned this in the other thread, but I thought I'd point out a reasonable use case for something you would not want to build into this module's classes: ApectMock allows overriding methods in some subset of classes. Knowing if a class is under AspectMock control would be a reasonable addition to a derived class of these classes. This library is a dependency of AspectMock, and should have no knowledge of it. Therefore, if a class is under AspectMock control is a good candidate for a derived class extension that should NOT be integrated into this library.