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

Revert "feature #1757 deprecated Twig_Template::getEnvironment()" #2106

Closed
wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Contributor

This reverts commit cfb0593, reversing
changes made to 1eb5fb2.

WDYT? removing getEnvironment led to #1813, and now I'd like to get the filename (when using the Filesystem loader), but I can't.
If OK, I think we should also revert #1813 on master, and maybe also #1807 on 1.x

@stof
Copy link
Member

stof commented Aug 30, 2016

How is ->getSource related to the deprecation of getEnvironment ?

@nicolas-grekas
Copy link
Contributor Author

Because when you have the env, you can do $template->getEnvironment()->getLoader()->getSource($template->getTemplateName()) to get the source.
With this is mind, adding getSource could be seen as a workaround for the removal of getEnvironment.

@ninsuo
Copy link
Contributor

ninsuo commented Sep 1, 2016

👌

…() (fabpot)"

This reverts commit cfb0593, reversing
changes made to 1eb5fb2.
fabpot added a commit that referenced this pull request Sep 19, 2016
This PR was merged into the 1.x branch.

Discussion
----------

deprecated some more methods on Twig_Environment

These methods are sometimes misused (#2020) when called from a template context (Twig_Template::getEnv() has been deprecated as well, but #2106 might re-introduce it).

Anyway, Twig_Environment should not act as a container, so, being able to change the defaults make sense, but retrieving them probably not.

Commits
-------

e548cbe deprecated some more methods on Twig_Environment
@fabpot
Copy link
Contributor

fabpot commented Sep 20, 2016

I'm 👎. This is hackish and I don't want to support this. I've fixed the getSource issue more elegantly in #2129 and we need to expose some more things, let's do it. But getting the environment to be able to get the loader really looks bad to me.

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

Successfully merging this pull request may close these issues.

None yet

4 participants