Skip to content

[TwigBundle] Fix twig loader registered twice #20712

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

Merged
merged 1 commit into from
Dec 2, 2016
Merged

[TwigBundle] Fix twig loader registered twice #20712

merged 1 commit into from
Dec 2, 2016

Conversation

ogizanagi
Copy link
Contributor

@ogizanagi 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
Copy link
Member

stof commented Dec 1, 2016

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

@ogizanagi
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link

mvrhov commented Dec 2, 2016

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

@xabbuh
Copy link
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
Copy link
Member

stof commented Dec 2, 2016

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

@ogizanagi
Copy link
Contributor Author

Fair enough. Reverted to the private alias solution 😄

@fabpot
Copy link
Member

fabpot commented Dec 2, 2016

Thank you @ogizanagi.

@fabpot fabpot merged commit 2c81819 into symfony:2.7 Dec 2, 2016
fabpot added a commit that referenced this pull request Dec 2, 2016
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 fix/2.7/twig_bundle/fix_loader_reg_twice branch December 2, 2016 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants