[FrameworkBundle] Use Bundle#getPath() to load config files #18579
Conversation
| $reflection = new \ReflectionClass($fqcn); | ||
| $getPathReflection = new \ReflectionMethod($fqcn, 'getPath'); | ||
|
|
||
| return $getPathReflection->invoke(new $fqcn()) ?: dirname($reflection->getFilename()); |
There was a problem hiding this comment.
this is broken in case the class has some mandatory controller arguments (a few bundles do)
| { | ||
| $reflection = new \ReflectionClass($fqcn); | ||
| $getPathReflection = new \ReflectionMethod($fqcn, 'getPath'); | ||
| $reflectionInstance = $reflection->newInstanceWithoutConstructor(); |
There was a problem hiding this comment.
@stof I replaced the instance by this one, but I'm not sure if it can be a solution?
There was a problem hiding this comment.
I don't think it's a solution, the bundle could use on of it's constructor arguments to determine it's path.
There was a problem hiding this comment.
@jvasseur Yes this is what I mean. Any suggestion?
There was a problem hiding this comment.
I don't think we can do anything without the actual bundle instances, maybe we can update the Kernel class to set the kernel service into the ContainerBuilder ?
There was a problem hiding this comment.
ATM the only working way I found is to pass the Kernel instance as argument to the FrameworkBundle when register it (e.g. new FrameworkBundle($this) in the AppKernel), then pass the kernel as argument to a compiler pass (from the FrameworkBundle class) and use it to get the bundle instances. It works but I think that adding an argument to the FrameworkBundle is really not an alternative :/
|
Is there an actual use-case to define the bundle path based on constructor arguments? I don't see any reason to do that. What about making the method static? Won't that be an easy solution? |
|
@wouterj I don't see any reason to use the constructor args inside the method, but in this case it leads to a fatal error. EDIT @wouterj Making the method static gives unexpected side effects as the method uses |
|
@wouterj the use case is see is to have a "virtual" bundle class that you can add multiple times, specifying the path in the constructor. |
|
@jvasseur In case of setting the path in In case of ATM I see a real gain into use the bundle#getPath if available (it will for most bundles), and a real loss if we don't try to use it at all. |
|
@chalasr the example i had in mind is https://github.com/WouterJ/Bundleless/blob/master/VirtualBundle.php The problem with creating a bundle with |
|
@jvasseur As the properties are declared but not set (constructor not called), calling As I understand, you're right, this fixes the issue for most cases but not for this particular (where it would behave as actually), but, while members are properly declared, I don't see how it could lead to errors? |
|
@chalasr It could lead to errors if you do something like this class Bundle
{
public function __construct($someProperty)
{
$this->someProperty = $someProperty;
}
public function getPath()
{
return $this->someProperty->someMethod();
}
} |
|
As said, the proposed fix would cover most of cases (all bundles classes that don't use any constructor argument into the |
Replace breaking new instance by reflection Prevent unsupported method call for php < 5.4
|
I close this simply because it doesn't solve the problem entirely. Cover it partially would not be a good thing. Hope we soon find a real solution for this blocking issue. |
This is the better workaround I found for now.
As proposed by @wouterj, it would be cleaner to move the whole logic in a compiler pass and use the bundle classes instance directly, but
$container->get('kernel')->getBundles()is not accessible because the service is synthetic (tried within the SerializerPass for instance), same problem in the extension.I would like to have your suggestions if you think that would be even better to use a compiler pass.
I've kept the reflection and used it to invoke the
getPath()method of each bundle, rather than callingdirname($reflection->getFilename())and so totally ignore thegetPath()method.