JMSDiExtraBundle icon indicating copy to clipboard operation
JMSDiExtraBundle copied to clipboard

@Service and class inheritance issue

Open zimmermanj42 opened this issue 12 years ago • 3 comments

Posted an issue earlier, but closed it as I found out more about it and it has changed quite a bit.

Here is my setup:

<?php

namespace FirstBundle\Services;

use JMS\DiExtraBundle\Annotation as DI;

/**
 * @DI\Service("foo", public = true)
 */
class Foo {}


<?php

namespace SecondBundle\Services;

use JMS\DiExtraBundle\Annotation as DI;
use FirstBundle\Services\Foo as BaseFoo;

/**
 * @DI\Service("foo", public = true)
 */
class Foo extends BaseFoo {}

Ultimately, I need to replace a service defined in FirstBundle with a new implementation in SecondBundle (I am using bundle inheritance to extend/replace other things in the FirstBundle).

However, the above configuration doesn't work; some kind of infinite loop is entered in Symfony and I run out of memory (just running the "container:debug" command).

I've discovered that JMSDiExtraBundle is setting the "parent" of the service definition (rather a definition decorator) for the SecondBundle's Foo class to "foo", so in some kind of compiler pass Symfony just keeps looping over the "foo" service as it's parent is itself...

It seems that JMSDiExtraBundle is determining the parent of the SecondBundle's Foo by inspecting the class data (since it extends the FirstBundle's Foo), and even if I explicitly set the "parent" attribute in the Service annotation markup for SecondBundle's Foo to "null" (not the string, but just plain null), it is still setting the parent based on this class metadata.

I don't know if this is intended or not, but if I don't use the JMSDiExtraBundle annotations and just stick with YAML config, this scheme works just fine and as intended.

zimmermanj42 avatar Jul 03 '13 21:07 zimmermanj42

This is were the "issue" is happening (file is "/Metadata/MetadataConverter.php"):

class MetadataConverter
{

    public function convert(ClassHierarchyMetadata $metadata)
    {
        static $count = 0;
        $definitions = array();

        $previous = null;
        foreach ($metadata->classMetadata as $classMetadata) {
            if (null === $previous && null === $classMetadata->parent) {
                $definition = new Definition();
            } else {
                $definition = new DefinitionDecorator(
                    $classMetadata->parent ?: $previous->id
                );
            }

The "&&" in "if (null === $previous && null === $classMetadata->parent)" is what is causing this to happen. Even if the Service annotation is set to null for the parent, the fact that the class extends another class seems to trump that and still create a DefinitionDecorator compared to a Definition that would end up overwriting the current one (assuming the same name is used). If the "&&" is changed to "||", the fact that I set a null parent in the annotation takes affect and this then works.

Like I've said, what is there may be the intended effect, but it would seem that if you explicitly set a null parent in the annotation, then the parent should be null and a new Definition would be made.

zimmermanj42 avatar Jul 04 '13 03:07 zimmermanj42

I can confirm this bug when using standard class Inheritance in Symfony Controllers.

Example:

class AbstractController extends Controller
{
    /**
     * @DI\Inject("my_software.config")
     */
    public $config;
}
class FooController extends AbstractController
{
    public function fooAction()
    {
        return new Response('hello');
    }
}

FooController is loaded as an Instance of AbstractController. As soon as I Inject another Property in FooController as well, the problem disappears. The problem only appears when there are >1 controllers extending AbstractController... Just a quick guess but the Bug seems to be somewhere around MetadataFactory:67 (getMetadataForClass)..

toothman avatar Sep 22 '14 15:09 toothman

Ran into this myself. @zimmermanj42's comment fixes the issue. Can we get dev feedback?

jtreminio avatar Dec 15 '16 00:12 jtreminio