[TwigBridge] fix FormExtension::$renderer bc break #20710
Conversation
Before b515702, the FormExtension::$renderer property was not declared but initialized in FormExtension::__construct(). This makes the property visibility to public. In b515702, the property was declared but with a private visibility. This broke the backward compatibility of the FormExtension class.
| /** | ||
| * Make this property private in 4.0. | ||
| */ | ||
| public $renderer; |
There was a problem hiding this comment.
Maybe a better way might be to keep it private and use a magic getter to throw a deprecation notice? I know, magic is bad, but deprecation notices might outweight this?
There was a problem hiding this comment.
I like the idea but how to avoid access to properties other than $renderer in a nice way? Throwing a RuntimeException? And what about __isset and __unset ?
There was a problem hiding this comment.
I'd indeed throw an exception. And yeah, __isset and __unset would have to be implemented, so… not sure it's worth the hassle.
| @@ -23,7 +23,10 @@ | |||
| */ | |||
| class FormExtension extends \Twig_Extension implements \Twig_Extension_InitRuntimeInterface | |||
| { | |||
| private $renderer; | |||
| /** | |||
| * Make this property private in 4.0. | |||
There was a problem hiding this comment.
please mark it as @internal though, so that IDEs warn users
There was a problem hiding this comment.
btw, no need to mark it as private in 4.0, as it will be removed.
|
I'm not even sure this will solve your issue, as I don't think the property is initialized anymore in core (due to deprecation) |
|
You are right. Is adding re-adding the dependency as lazy to avoid the circular reference problem fixed in b515702, removing the deprecated notice from the constructor (I think Symfony should not trigger deprecated when using own objects) and using magic methods with deprecation notice will solve the issue? |
|
Note however that I'm not sure this should be actually fixed. A property not defined in the code cannot really be considered as being part of our public API covered by the BC promise (and if it had been declared explicitly previously, it would have been tagged as |
|
I understand that the missing property was a mistake and if it existed, it will never been with a public visibility, but at least one bundle (sonata-project/SonataAdminBundle#4216) was using it. As sonata-project has a BC release process, it may be ok to not fix it in Symfony, but it may have other libraries relying on this public property (I don't have any example). |
|
@fbourigault it would still have been public (it was necessary before) but it would have been an internal one. |
|
I agree that we can't really expect an undeclared property to be part of the BC promise. Relying on it in the first place was a mistake. We must fix this on the Sonata side. |
|
Does it mean we can close this one? |
|
I think so. |
|
Closing because this is not |
|
@fbourigault it is not that we don't consider it as a BC break (it is one indeed). But we consider that it is not covered by our BC promise, as being part of an internal implementation |
|
@fbourigault : I second that, everything is a BC-break ;) |
|
@nicolas-grekas opened a PR using magic methods: #20769 |
…::$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
|
Looks like we have reenabled the spacebar heating :) |
Before b515702, the
FormExtension::$rendererproperty was not declared but initialized inFormExtension::__construct(). This makes the property visibility to public. In b515702, the property was declared but with a private visibility. This broke the backward compatibility of theFormExtensionclass.