Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Bridge\Twig] Trigger deprecation when using FormExtension::$renderer#20769
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
greg0ire commentedDec 5, 2016
Wow thanks for putting that much effort into BC! |
Koc commentedDec 5, 2016
I am afraid that this property is not initialized anymore but haven't tested yet. |
| */ | ||
| publicfunction__get($name) | ||
| { | ||
| if ('renderer' ===$name) { |
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.
Actually it will be removed in 4.0
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.
updated
greg0ire commentedDec 5, 2016
fbourigault commentedDec 5, 2016
Is it safer to throw an Exception when using magic methods with something else than '$renderer'? |
greg0ire commentedDec 5, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Proof Travis job. Not sure it completely works though… maybe something more needs to be done (the initialization@Koc was talking about?) |
nicolas-grekas commentedDec 6, 2016
this is unrelated and wouldn't match the behavior in 4.0 |
xabbuh commentedDec 6, 2016
@nicolas-grekas But it allows us to add private or protected properties in the future without exposing them. Otherwise we would have to remember to update the magic getter and setter methods. |
nicolas-grekas commentedDec 6, 2016 via email• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
But it allows us to add private or protected properties in the future without exposing them. Otherwise we would have to remember to update the magic getter and setter methods. So be it. Throwing would prevent removing the magic methods to preserve thebehavior in 4.0, which is a no go to me. |
fbourigault commentedDec 6, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@nicolas-grekas, you must also re-inject the renderer in the service definition as it wasremoved when this break was introduced. |
f3d766e to295e4c1Comparenicolas-grekas commentedDec 6, 2016
The renderer is now reinjected, but in a lazy way. PR ready. |
| publicfunctioninitRuntime(\Twig_Environment$environment) | ||
| { | ||
| if (null !==$this->renderer) { | ||
| if ($this->rendererinstanceof TwigRendererInterface) { |
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.
Should we initialize renderer here?
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.
done (in a lazy way again)
| * @internal | ||
| */ | ||
| publicfunction__set($name,$value) | ||
| { |
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.
we should throw an exception for any other name
| * @internal | ||
| */ | ||
| publicfunction__get($name) | ||
| { |
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.
we should throw an exception for any other name
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.
no no no, see#20769 (comment)
| * @internal | ||
| */ | ||
| publicfunction__unset($name) | ||
| { |
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.
we should forbid any other name
| "require": { | ||
| "php":">=5.5.9", | ||
| "symfony/twig-bridge":"~3.2", | ||
| "symfony/twig-bridge":"~3.2,>=3.2.1", |
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.
use^3.2.1, which is exactly what you did here with 2 constraints
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.
updated
xabbuh commentedDec 6, 2016
@nicolas-grekas But the current implementation would suffer the same. You would receive |
nicolas-grekas commentedDec 6, 2016
They will get exactly the same behavior as if the property did not exist: a PHP notice. |
xabbuh commentedDec 7, 2016
Oh indeed, I didn't say anything then. :) |
fabpot commentedDec 7, 2016
Thank you@nicolas-grekas. |
…::$renderer (nicolas-grekas)This PR was merged into the 3.2 branch.Discussion----------[Bridge\Twig] Trigger deprecation when using FormExtension::$renderer| Q | A| ------------- | ---| Branch? | 3.2| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | yes (instead of a BC break)| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -As spotted in#20710 andsonata-project/SonataAdminBundle#4216.Note that this simple implementation is fine because neither the class nor its parent have any private/protected properties.Commits-------6f1c59c [Bridge\Twig] Trigger deprecation when using FormExtension::$renderer
As spotted in#20710 andsonata-project/SonataAdminBundle#4216.
Note that this simple implementation is fine because neither the class nor its parent have any private/protected properties.