Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[TwigBridge] fix FormExtension::$renderer bc break#20710
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
| /** | ||
| * Make this property private in 4.0. | ||
| */ | ||
| public$renderer; |
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.
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?
fbourigaultDec 1, 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.
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.
I like the idea but how to avoid access to properties other than$renderer in a nice way? Throwing aRuntimeException? And what about__isset and__unset ?
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.
I'd indeed throw an exception. And yeah,__isset and__unset would have to be implemented, so… not sure it's worth the hassle.
| { | ||
| private$renderer; | ||
| /** | ||
| * Make this property private 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.
please mark it as@internal though, so that IDEs warn users
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.
btw, no need to mark it as private in 4.0, as it will be removed.
stof commentedDec 1, 2016
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) |
fbourigault commentedDec 1, 2016
You are right. Is adding re-adding the dependency as lazy to avoid the circular reference problem fixed inb515702, 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? |
stof commentedDec 1, 2016
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 |
fbourigault commentedDec 2, 2016
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). |
stof commentedDec 2, 2016
@fbourigault it would still have been public (it was necessary before) but it would have been an internal one. |
greg0ire commentedDec 2, 2016
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. |
fabpot commentedDec 2, 2016
Does it mean we can close this one? |
greg0ire commentedDec 2, 2016
I think so. |
fbourigault commentedDec 2, 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.
Closing because this is not |
stof commentedDec 2, 2016
@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 |
greg0ire commentedDec 2, 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.
@fbourigault : I second that, everything is a BC-break ;) |
fbourigault commentedDec 2, 2016
fbourigault commentedDec 5, 2016
@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 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
fabpot commentedDec 7, 2016
Looks like we have reenabled the spacebar heating :) |
Beforeb515702, the
FormExtension::$rendererproperty was not declared but initialized inFormExtension::__construct(). This makes the property visibility to public. Inb515702, the property was declared but with a private visibility. This broke the backward compatibility of theFormExtensionclass.