[Bridge\Twig] Trigger deprecation when using FormExtension::$renderer #20769
Conversation
0337d78 to
ac478c2
Compare
|
Wow thanks for putting that much effort into BC! |
|
I am afraid that this property is not initialized anymore but haven't tested yet. |
| public function __get($name) | ||
| { | ||
| if ('renderer' === $name) { | ||
| @trigger_error(sprintf('Using the "%s::$renderer" property is deprecated since version 3.2 as it will be made private in 4.0.', __CLASS__), E_USER_DEPRECATED); |
There was a problem hiding this comment.
Actually it will be removed in 4.0
|
Is it safer to throw an Exception when using magic methods with something else than '$renderer'? |
|
Proof Travis job. Not sure it completely works though… maybe something more needs to be done (the initialization @Koc was talking about?) |
ac478c2 to
c1ed3ce
Compare
this is unrelated and wouldn't match the behavior in 4.0 |
|
@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. |
|
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 the
behavior in 4.0, which is a no go to me.
|
|
@nicolas-grekas, you must also re-inject the renderer in the service definition as it was removed when this break was introduced. |
f3d766e to
295e4c1
Compare
|
The renderer is now reinjected, but in a lazy way. PR ready. |
| @@ -40,7 +46,7 @@ public function __construct(TwigRendererInterface $renderer = null) | |||
| */ | |||
| public function initRuntime(\Twig_Environment $environment) | |||
| { | |||
| if (null !== $this->renderer) { | |||
| if ($this->renderer instanceof TwigRendererInterface) { | |||
There was a problem hiding this comment.
Should we initialize renderer here?
There was a problem hiding this comment.
done (in a lazy way again)
295e4c1 to
c77e38e
Compare
| */ | ||
| public function __set($name, $value) | ||
| { | ||
| if ('renderer' === $name) { |
There was a problem hiding this comment.
we should throw an exception for any other name
| */ | ||
| public function __get($name) | ||
| { | ||
| if ('renderer' === $name) { |
There was a problem hiding this comment.
we should throw an exception for any other name
There was a problem hiding this comment.
no no no, see #20769 (comment)
| */ | ||
| public function __unset($name) | ||
| { | ||
| if ('renderer' === $name) { |
| @@ -17,7 +17,7 @@ | |||
| ], | |||
| "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.
use ^3.2.1, which is exactly what you did here with 2 constraints
c77e38e to
73d9b1e
Compare
73d9b1e to
6f1c59c
Compare
@nicolas-grekas But the current implementation would suffer the same. You would receive |
|
They will get exactly the same behavior as if the property did not exist: a PHP notice. |
|
Oh indeed, I didn't say anything then. :) |
|
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 and sonata-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 and sonata-project/SonataAdminBundle#4216.
Note that this simple implementation is fine because neither the class nor its parent have any private/protected properties.