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

[Bridge\Twig] Trigger deprecation when using FormExtension::$renderer #20769

Merged
merged 1 commit into from Dec 7, 2016

Conversation

Projects
None yet
10 participants
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 5, 2016

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.

@greg0ire

This comment has been minimized.

Copy link
Contributor

greg0ire commented Dec 5, 2016

Wow thanks for putting that much effort into BC!

@Koc

This comment has been minimized.

Copy link
Contributor

Koc commented Dec 5, 2016

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);

This comment has been minimized.

@Koc

Koc Dec 5, 2016

Contributor

Actually it will be removed in 4.0

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 6, 2016

Author Member

updated

@greg0ire

This comment has been minimized.

Copy link
Contributor

greg0ire commented Dec 5, 2016

@fbourigault

This comment has been minimized.

Copy link
Contributor

fbourigault commented Dec 5, 2016

Is it safer to throw an Exception when using magic methods with something else than '$renderer'?

@greg0ire

This comment has been minimized.

Copy link
Contributor

greg0ire commented Dec 5, 2016

Proof Travis job. Not sure it completely works though… maybe something more needs to be done (the initialization @Koc was talking about?)

Here is the output without your changes

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:fix-bc branch from ac478c2 to c1ed3ce Dec 6, 2016

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Dec 6, 2016

Is it safer to throw an Exception when using magic methods with something else than '$renderer'?

this is unrelated and wouldn't match the behavior in 4.0

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Dec 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

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Dec 6, 2016

@fbourigault

This comment has been minimized.

Copy link
Contributor

fbourigault commented Dec 6, 2016

@nicolas-grekas, you must also re-inject the renderer in the service definition as it was removed when this break was introduced.

@nicolas-grekas nicolas-grekas added this to the 3.2 milestone Dec 6, 2016

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:fix-bc branch 2 times, most recently from f3d766e to 295e4c1 Dec 6, 2016

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Dec 6, 2016

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) {

This comment has been minimized.

@Koc

Koc Dec 6, 2016

Contributor

Should we initialize renderer here?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 6, 2016

Author Member

done (in a lazy way again)

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:fix-bc branch from 295e4c1 to c77e38e Dec 6, 2016

*/
public function __set($name, $value)
{
if ('renderer' === $name) {

This comment has been minimized.

@stof

stof Dec 6, 2016

Member

we should throw an exception for any other name

*/
public function __get($name)
{
if ('renderer' === $name) {

This comment has been minimized.

@stof

stof Dec 6, 2016

Member

we should throw an exception for any other name

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 6, 2016

Author Member

no no no, see #20769 (comment)

*/
public function __unset($name)
{
if ('renderer' === $name) {

This comment has been minimized.

@stof

stof Dec 6, 2016

Member

we should forbid any other name

@@ -17,7 +17,7 @@
],
"require": {
"php": ">=5.5.9",
"symfony/twig-bridge": "~3.2",
"symfony/twig-bridge": "~3.2,>=3.2.1",

This comment has been minimized.

@stof

stof Dec 6, 2016

Member

use ^3.2.1, which is exactly what you did here with 2 constraints

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 6, 2016

Author Member

updated

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:fix-bc branch from c77e38e to 73d9b1e Dec 6, 2016

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Dec 6, 2016

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 But the current implementation would suffer the same. You would receive null now, but you will get a PHP error with Symfony 4.0. Throwing an exception now would at least warn users to be aware that they do something wrong.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Dec 6, 2016

They will get exactly the same behavior as if the property did not exist: a PHP notice.
See https://3v4l.org/YBvLe

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Dec 7, 2016

Oh indeed, I didn't say anything then. :)

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Dec 7, 2016

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 6f1c59c into symfony:3.2 Dec 7, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Dec 7, 2016

bug #20769 [Bridge\Twig] Trigger deprecation when using FormExtension…
…::$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

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:fix-bc branch Dec 7, 2016

@fbourigault fbourigault referenced this pull request Dec 7, 2016

Closed

Release 2.0 #235

@fabpot fabpot referenced this pull request Dec 13, 2016

Merged

Release v3.2.1 #20897

@fbourigault fbourigault referenced this pull request Jan 30, 2017

Closed

[WIP] Deprecate AbstractAdmin::isGranted #4287

3 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment