Skip to content
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] fixed Twig options (removed the parameter as it cannot contain service references) #13476

Merged
merged 1 commit into from Jan 30, 2015

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Jan 21, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets #13467
License MIT
Doc PR n/a

The new autoescape_service option is used to set the autoescape Twig option, which can be a callable. The Twig options are stored in a parameter (twig.options), but as parameters cannot have references, that does not work well.

So, this PR removed the parameter as it is not needed.

@fabpot
Copy link
Member Author

fabpot commented Jan 21, 2015

I've also made an additional commit to remove two other unneeded parameters.

@stof
Copy link
Member

stof commented Jan 21, 2015

isn't it a BC break to remove these parameters (for edge cases though) ?


$container->setParameter('twig.form.resources', $config['form_themes']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, actually, I'm quite sure I know about bundles relying on this parameter to prepend form themes defining the rendering of their special blocks, so this is an actual BC break

@fabpot
Copy link
Member Author

fabpot commented Jan 25, 2015

I've removed the second commit to keep BC

@stof
Copy link
Member

stof commented Jan 30, 2015

👍

@fabpot fabpot merged commit 69748a1 into symfony:2.7 Jan 30, 2015
fabpot added a commit that referenced this pull request Jan 30, 2015
…it cannot contain service references) (fabpot)

This PR was merged into the 2.7 branch.

Discussion
----------

[TwigBundle] fixed Twig options (removed the parameter as it cannot contain service references)

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no
| Fixed tickets | #13467
| License       | MIT
| Doc PR        | n/a

The new `autoescape_service` option is used to set the `autoescape` Twig option, which can be a callable. The Twig options are stored in a parameter (`twig.options`), but as parameters cannot have references, that does not work well.

So, this PR removed the parameter as it is not needed.

Commits
-------

69748a1 [TwigBundle] fixed Twig options (removed the parameter as it cannot contain service references)
@fabpot fabpot deleted the autoescape-fix branch February 12, 2015 09:32
@richardfullmer
Copy link

SonataCacheBundle was relying on this parameter. See sonata-project/SonataCacheBundle#72

@xabbuh
Copy link
Member

xabbuh commented Jun 1, 2015

@richardfullmer Can you please open a new issue if you think that this is a bug?

@richardfullmer
Copy link

I don't necessarily think it's a bug. How does one tell which parameters will have guaranteed BC verses those that don't?

@fabpot
Copy link
Member Author

fabpot commented Jun 2, 2015

They are no guarantees on parameters except for the one defined in the kernel (like debug, ...). Almost all parameters are private and should not be relied on.

@richardfullmer
Copy link

thanks

@peterrehm
Copy link
Contributor

I stumbled around that issue as well as I used that to have a second twig environment with just a string loader for special renderings. I did not want to use the loader chain aas I wanted still errors in case of a wrong template name.

A sample implementation can be found here:
https://techpunch.co.uk/development/render-string-twig-template-symfony2

What is the easiest way to work around that now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants