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

[TwigBridge] fix BC for FormExtension if renderer is FormRenderer #24535

Closed
wants to merge 8 commits into
base: 3.4
from

Conversation

Projects
None yet
8 participants
@dmaicher
Contributor

dmaicher commented Oct 12, 2017

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #24533
License MIT
Doc PR -

This fixes some issues within FormExtension since the renderer is now a FormRenderer.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Oct 12, 2017

Member

Can't we create a test case that would fail without these changes?

Member

xabbuh commented Oct 12, 2017

Can't we create a test case that would fail without these changes?

dmaicher added some commits Oct 12, 2017

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 12, 2017

@dmaicher

This comment has been minimized.

Show comment
Hide comment
@dmaicher

dmaicher Oct 12, 2017

Contributor

I don't quite understand this test fail:

1) Warning
The data provider specified for Symfony\Bridge\Twig\Tests\Extension\FormExtensionTest::testInitRuntimeAndAccessRenderer is invalid.
Trying to configure method "get" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Warning
The data provider specified for Symfony\Bridge\Twig\Tests\Extension\FormExtensionTest::testAccessRendererAndInitRuntime is invalid.
Trying to configure method "get" which cannot be configured because it does not exist, has not been specified, is final, or is static

I'm mocking Symfony\Component\DependencyInjection\ContainerInterface.

Contributor

dmaicher commented Oct 12, 2017

I don't quite understand this test fail:

1) Warning
The data provider specified for Symfony\Bridge\Twig\Tests\Extension\FormExtensionTest::testInitRuntimeAndAccessRenderer is invalid.
Trying to configure method "get" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Warning
The data provider specified for Symfony\Bridge\Twig\Tests\Extension\FormExtensionTest::testAccessRendererAndInitRuntime is invalid.
Trying to configure method "get" which cannot be configured because it does not exist, has not been specified, is final, or is static

I'm mocking Symfony\Component\DependencyInjection\ContainerInterface.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Oct 12, 2017

Member

Looks like you need to add your tests to the legacy group.

Member

xabbuh commented Oct 12, 2017

Looks like you need to add your tests to the legacy group.

@dmaicher

This comment has been minimized.

Show comment
Hide comment
@dmaicher

dmaicher Oct 12, 2017

Contributor

@xabbuh thanks looks better now. One test fail left: https://travis-ci.org/symfony/symfony/jobs/287174522

Contributor

dmaicher commented Oct 12, 2017

@xabbuh thanks looks better now. One test fail left: https://travis-ci.org/symfony/symfony/jobs/287174522

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Oct 13, 2017

Member

You need to add symfony/dependency-injection to the require-dev section of TwigBridge's composer.json file.

Member

xabbuh commented Oct 13, 2017

You need to add symfony/dependency-injection to the require-dev section of TwigBridge's composer.json file.

@dmaicher

This comment has been minimized.

Show comment
Hide comment
@dmaicher

dmaicher Oct 13, 2017

Contributor

thanks again @xabbuh 👍 Can be reviewed then

Contributor

dmaicher commented Oct 13, 2017

thanks again @xabbuh 👍 Can be reviewed then

@garf

garf approved these changes Oct 13, 2017

@@ -54,7 +54,7 @@ public function initRuntime(Environment $environment)
{
if ($this->renderer instanceof TwigRendererInterface) {
$this->renderer->setEnvironment($environment);
} elseif (null !== $this->renderer) {
} elseif (is_array($this->renderer)) {

This comment has been minimized.

@xabbuh

xabbuh Oct 13, 2017

Member

This was not needed to fix the bug, right?

@xabbuh

xabbuh Oct 13, 2017

Member

This was not needed to fix the bug, right?

This comment has been minimized.

@dmaicher

dmaicher Oct 13, 2017

Contributor

It is needed:

Testing Symfony\Bridge\Twig\Tests\Extension\FormExtensionTest
.....E                                                              6 / 6 (100%)

Time: 132 ms, Memory: 4.00MB

There was 1 error:

1) Symfony\Bridge\Twig\Tests\Extension\FormExtensionTest::testAccessRendererAndInitRuntime with data set #2 (array(Mock_ContainerInterface_3ca94d5e Object (...), 'service_id'), Mock_FormRendererInterface_d5edefef Object (...))
Error: Cannot use object of type Mock_FormRendererInterface_d5edefef as array

/var/www/symfony/src/Symfony/Bridge/Twig/Extension/FormExtension.php:58
/var/www/symfony/src/Symfony/Bridge/Twig/Tests/Extension/FormExtensionTest.php:43

ERRORS!
Tests: 6, Assertions: 15, Errors: 1.
@dmaicher

dmaicher Oct 13, 2017

Contributor

It is needed:

Testing Symfony\Bridge\Twig\Tests\Extension\FormExtensionTest
.....E                                                              6 / 6 (100%)

Time: 132 ms, Memory: 4.00MB

There was 1 error:

1) Symfony\Bridge\Twig\Tests\Extension\FormExtensionTest::testAccessRendererAndInitRuntime with data set #2 (array(Mock_ContainerInterface_3ca94d5e Object (...), 'service_id'), Mock_FormRendererInterface_d5edefef Object (...))
Error: Cannot use object of type Mock_FormRendererInterface_d5edefef as array

/var/www/symfony/src/Symfony/Bridge/Twig/Extension/FormExtension.php:58
/var/www/symfony/src/Symfony/Bridge/Twig/Tests/Extension/FormExtensionTest.php:43

ERRORS!
Tests: 6, Assertions: 15, Errors: 1.

This comment has been minimized.

@dmaicher

dmaicher Oct 13, 2017

Contributor

Which is the problem reported here: sonata-project/SonataAdminBundle#4652

@dmaicher

dmaicher Oct 13, 2017

Contributor

Which is the problem reported here: sonata-project/SonataAdminBundle#4652

This comment has been minimized.

@xabbuh

xabbuh Oct 13, 2017

Member

For anyone else wondering: The array stored in $this->renderer is replaced by the FormRenderInterface instance that is fetched from the container in __get(): https://github.com/dmaicher/symfony/blob/6813a9f1f1251412bf6161dd732e65e853680736/src/Symfony/Bridge/Twig/Extension/FormExtension.php#L124

@xabbuh

xabbuh Oct 13, 2017

Member

For anyone else wondering: The array stored in $this->renderer is replaced by the FormRenderInterface instance that is fetched from the container in __get(): https://github.com/dmaicher/symfony/blob/6813a9f1f1251412bf6161dd732e65e853680736/src/Symfony/Bridge/Twig/Extension/FormExtension.php#L124

@xabbuh

xabbuh approved these changes Oct 13, 2017

@@ -22,6 +22,7 @@
"require-dev": {
"fig/link-util": "^1.0",
"symfony/asset": "~2.8|~3.0|~4.0",
"symfony/dependency-injection": "~2.8|~3.0|~4.0",

This comment has been minimized.

@chalasr

chalasr Oct 13, 2017

Member

this targets 3.4, should this be ~3.4|~4.0?

@chalasr

chalasr Oct 13, 2017

Member

this targets 3.4, should this be ~3.4|~4.0?

This comment has been minimized.

@dmaicher

dmaicher Oct 13, 2017

Contributor

I just need the ContainerInterface for the tests. So I guess 2.8 is also fine? Not sure what to do here tbh 😊

@dmaicher

dmaicher Oct 13, 2017

Contributor

I just need the ContainerInterface for the tests. So I guess 2.8 is also fine? Not sure what to do here tbh 😊

@fabpot

fabpot approved these changes Oct 13, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 13, 2017

Member

Thank you @dmaicher.

Member

fabpot commented Oct 13, 2017

Thank you @dmaicher.

fabpot added a commit that referenced this pull request Oct 13, 2017

bug #24535 [TwigBridge] fix BC for FormExtension if renderer is FormR…
…enderer (dmaicher)

This PR was squashed before being merged into the 3.4 branch (closes #24535).

Discussion
----------

[TwigBridge] fix BC for FormExtension if renderer is FormRenderer

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24533
| License       | MIT
| Doc PR        | -

This fixes some issues within FormExtension since the renderer is now a `FormRenderer`.

Commits
-------

4a2f608 [TwigBridge] fix BC for FormExtension if renderer is FormRenderer

@fabpot fabpot closed this Oct 13, 2017

@dmaicher dmaicher deleted the dmaicher:fix-24533 branch Oct 13, 2017

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Oct 13, 2017

Member

Just noticed: Shouldn't we have merged this into the 3.3 branch?

Member

xabbuh commented Oct 13, 2017

Just noticed: Shouldn't we have merged this into the 3.3 branch?

@dmaicher

This comment has been minimized.

Show comment
Hide comment
@dmaicher

dmaicher Oct 13, 2017

Contributor

@xabbuh this bug did not exist on 3.3 though as the rendering service there was an instance of TwigRendererInterface and no FormRendererInterface yet as on 3.4

Contributor

dmaicher commented Oct 13, 2017

@xabbuh this bug did not exist on 3.3 though as the rendering service there was an instance of TwigRendererInterface and no FormRendererInterface yet as on 3.4

chalasr added a commit that referenced this pull request Oct 18, 2017

minor #24603 [TwigBridge] fix merge of 3.4 into master (dmaicher)
This PR was merged into the 4.0-dev branch.

Discussion
----------

[TwigBridge] fix merge of 3.4 into master

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes/no
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Within #24535 I added `FormExtensionTest` which is completely useless now on master 😊

So this PR just removes the test after it has been merged from 3.4 into master.

Commits
-------

5c36609 [TwigBridge] fix merge of 3.4 into master
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 15, 2017

Member

@dmaicher could you please check sonata-project/SonataAdminBundle#4758, is it somehow related to this one?

Member

nicolas-grekas commented Nov 15, 2017

@dmaicher could you please check sonata-project/SonataAdminBundle#4758, is it somehow related to this one?

@dmaicher

This comment has been minimized.

Show comment
Hide comment
@dmaicher

dmaicher Nov 15, 2017

Contributor

@nicolas-grekas seems not related as my fix was for 3.4 but the problem there actually happens on 3.3

Contributor

dmaicher commented Nov 15, 2017

@nicolas-grekas seems not related as my fix was for 3.4 but the problem there actually happens on 3.3

@dmaicher

This comment has been minimized.

Show comment
Hide comment
@dmaicher

dmaicher Nov 15, 2017

Contributor

But I have the feeling 3.4 broke BC by switching from Symfony\Bridge\Twig\Form\TwigRenderer to Symfony\Component\Form\FormRenderer?

This PR only fixes some symptoms of that BC break imho.

Contributor

dmaicher commented Nov 15, 2017

But I have the feeling 3.4 broke BC by switching from Symfony\Bridge\Twig\Form\TwigRenderer to Symfony\Component\Form\FormRenderer?

This PR only fixes some symptoms of that BC break imho.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment