New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TwigBundle] Fix twig loader registered twice #20712

Merged
merged 1 commit into from Dec 2, 2016

Conversation

Projects
None yet
6 participants
@ogizanagi
Member

ogizanagi commented Dec 1, 2016

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #20665
License MIT
Doc PR N/A

Generated code:

Before

    protected function getTwig_LoaderService()
    {
        $a = new \Twig_Loader_Filesystem(array(), $this->targetDirs[3]);
        
        $a->addPath(...);
        // ...

        $this->services['twig.loader'] = $instance = new \Twig_Loader_Chain();

        $instance->addLoader($a);
        $instance->addLoader($a);

        return $instance;
    }

After

    protected function getTwig_LoaderService()
    {
        $this->services['twig.loader'] = $instance = new \Twig_Loader_Filesystem(array(), $this->targetDirs[3]);

        $instance->addPath(...);
        // ...

        return $instance;
    }

Another solution is to simply create a private alias. But I don't know if we should care or not about the case people may rely on the fact both services exist as definition, and not as an alias, in a compiler pass. (Has been preferred over of using a child definition)

For reference, this issue was introduced in #13354.

@stof

This comment has been minimized.

Member

stof commented Dec 1, 2016

a better solution would be to use an alias rather than a child service

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Dec 1, 2016

@stof : That's what I meant by (and the first solution I implemented):

Another solution is to simply create a private alias. But I don't know if we should care or not about the case people may rely on the fact both services exist as definition, and not as an alias, in a compiler pass.

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Dec 1, 2016

(By this I mean someone may use $container->getDefinition('twig.loader.filesystem') in a compiler pass, which won't work if twig.loader.filesystem becomes an alias)

@mvrhov

This comment has been minimized.

Contributor

mvrhov commented Dec 2, 2016

I'm using $container->getDefinition('twig.loader.filesystem') in a compiler pass.

@xabbuh

This comment has been minimized.

Member

xabbuh commented Dec 2, 2016

I am not sure if this is something that is part of our BC promise. Using hasDefinitiin() and getDefinition() in fact isn't the best idea (at least when working with definitions from other bundles that you do not control). You should rather use has() and findDefinition().

@stof

This comment has been minimized.

Member

stof commented Dec 2, 2016

@xabbuh is true. Definitions can be replaced by aliases in many cases (for instance when using decorates)

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Dec 2, 2016

Fair enough. Reverted to the private alias solution 😄

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 2, 2016

Thank you @ogizanagi.

@fabpot fabpot merged commit 2c81819 into symfony:2.7 Dec 2, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Dec 2, 2016

bug #20712 [TwigBundle] Fix twig loader registered twice (ogizanagi)
This PR was merged into the 2.7 branch.

Discussion
----------

[TwigBundle] Fix twig loader registered twice

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20665
| License       | MIT
| Doc PR        | N/A

Generated code:

### Before

```php
    protected function getTwig_LoaderService()
    {
        $a = new \Twig_Loader_Filesystem(array(), $this->targetDirs[3]);

        $a->addPath(...);
        // ...

        $this->services['twig.loader'] = $instance = new \Twig_Loader_Chain();

        $instance->addLoader($a);
        $instance->addLoader($a);

        return $instance;
    }
```

### After

```php
    protected function getTwig_LoaderService()
    {
        $this->services['twig.loader'] = $instance = new \Twig_Loader_Filesystem(array(), $this->targetDirs[3]);

        $instance->addPath(...);
        // ...

        return $instance;
    }
```

~~Another solution is to simply create a private alias. But I don't know if we should care or not about the case people may rely on the fact both services exist as definition, and not as an alias, in a compiler pass.~~ (Has been preferred over of using a child definition)

For reference, this issue was introduced in #13354.

Commits
-------

2c81819 [TwigBundle] Fix twig loader registered twice

@ogizanagi ogizanagi deleted the ogizanagi:fix/2.7/twig_bundle/fix_loader_reg_twice branch Dec 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment