-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[MailerBundle] Don't use deprecated Twig functionality #2530
Conversation
stloyd
commented
Mar 2, 2015
Q | A |
---|---|
Bug fix? | yes |
New feature? | no |
BC breaks? | no |
Deprecations? | no |
License | MIT |
if (null !== $email->getTemplate()) { | ||
$data = $this->twig->mergeGlobals($data); | ||
|
||
$template = $this->twig->loadTemplate($email->getTemplate()); | ||
|
||
$subject = $template->renderBlock('subject', $data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The renderBlock method cannot be called because it is not in the API of Twig_TemplateInterface: http://twig.sensiolabs.org/api/master/Twig_TemplateInterface.html
Docblock says: This method is for internal use only and should never be directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not case for this PR IMO...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but you replace deprecated features, it seems like the same ... not ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem with this is more complicated, that API is not deprecated, and yes it's internal, but unfortunately there is no real alternative to use (interface you pointed out is deprecated) so I guess it's acceptable to use it in current way till there will be official alternative.
Should I integrate this change in the following PR: #2482 ? |
if (null !== $email->getTemplate()) { | ||
$data = $this->twig->mergeGlobals($data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$data is used in the else, so it is needed before !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not used in else as it's not same env...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes my bad !
i've done all your changes in my PR #2482 |
@pjedrzejewski @Arn0d ping ;) |
[MailerBundle] Don't use deprecated Twig functionality
Thank you Joseph! 👍 |