8000 [TwigBridge] fix BC break caused by using FormRenderer instead of TwigRenderer by dmaicher · Pull Request #26553 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[TwigBridge] fix BC break caused by using FormRenderer instead of TwigRenderer #26553

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[TwigBridge] fix BC break caused by using FormRenderer instead of TwigRenderer #26553

wants to merge 1 commit into from

Conversation

dmaicher
Copy link
Contributor
@dmaicher dmaicher commented Mar 15, 2018 •
edited
Loading
Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #24616, #25659
License MIT
Doc PR -

In 6ea71cb#diff-5eb0532c96734f6e23428e921225b2f0R21 the TwigRenderer service was replaced with FormRenderer.

This caused 2 BC breaks:

  • people that were injecting twig.form.renderer and coding against the interface TwigRendererInterface suddenly received an error because the new FormRenderer does not implement this interface.
  • also retrieving the twig runtime for the old FQCN is not working anymore and causes a Twig_Error_Runtime exception

This PR reverts the class change for the service twig.form.renderer. This should be safe to revert because the old TwigRendererInterface extends FormRendererInterface and TwigRenderer extends FormRenderer. So all people that adapted to the BC break and changed the expected interface/class should still be fine.

Additionally because some people already adjusted to the second BC break when retrieving the runtime (see for example sonata-project/SonataAdminBundle#4758) we add the new non-deprecated FormRenderer as a runtime as well.

So both of these work on 3.4 then:

$twig->getRuntime(TwigRenderer::class)
$twig->getRuntime(FormRenderer::class)

WDYT? Did I forget something?

@dmaicher dmaicher mentioned this pull request Mar 15, 2018
Closed
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Mar 16, 2018
@nicolas-grekas
Copy link
Member

people that were injecting twig.form.renderer

when moving on 4.0, what will these people inject?

@dmaicher
Copy link
Contributor Author
dmaicher commented Mar 16, 2018 •
edited
Loading

when moving on 4.0, what will these people inject?

maybe a bit misleading what I wrote. That service twig.form.renderer still exists on 4.0 and there its the non-deprecated FormRenderer class (as TwigRenderer was removed).

So when upgrading to 4.0 its documented here as a BC break: https://github.com/symfony/symfony/blob/master/UPGRADE-4.0.md#twigbridge

EDIT: So actually this change here should be dropped completely when merging into 4.0

@nicolas-grekas
Copy link
Member

From what I understand, ppl using twig.form.renderer in 3.4 will get a deprecation for the class, so they will seek for removing this deprecation in 3.4, before moving to 4.0. I feel like there is a missing upgrade path, don't you ?

@dmaicher
Copy link
Contributor Author
dmaicher commented Mar 16, 2018 •
edited
Loading

From what I understand, ppl using twig.form.renderer in 3.4 will get a deprecation for the class, so they will seek for removing this deprecation in 3.4, before moving to 4.0

This is not correct for the current 3.4 as the service class was changed to FormRenderer together with deprecating TwigRenderer in one step (which is a BC break).

With my change people will indeed see a deprecation notice when using twig.form.renderer on 3.4.

EDIT:

I feel like there is a missing upgrade path, don't you ?

@nicolas-grekas ah you mean with the changes in this PR there is no upgrade path because of the deprecation notice? But this was the same for some other deprecated Symfony service classes or not? Like ContainerAwareEventDispatcher etc

@nicolas-grekas
Copy link
Member

you mean with the changes in this PR there is no upgrade path because of the deprecation notice

yes, that's what I meant

this was the same for some other deprecated Symfony service classes or not?

if yes, we should do stricter reviews, this should not be the case.

In the end, shouldn't we live with this BC break and document it in UPGRADE files?
I don't have a better idea to fix the BC break :(

@dmaicher
8000
Copy link
Contributor Author
dmaicher commented Mar 16, 2018 •
edited
Loading

In the end I'm also fine with just allowing this BC break and documenting it. What do you think @Tobion ?

@Tobion
Copy link
Contributor
Tobion commented Mar 16, 2018 •
edited
Loading

If we change it back, all people will get deprecations without upgrade path. So to me it's better document the small bc break.

@dmaicher dmaicher mentioned this pull request Mar 16, 2018
Merged
@dmaicher
Copy link
Contributor Author

See #26570

@dmaicher dmaicher closed this Mar 16, 2018
fabpot added a commit that referenced this pull request Mar 18, 2018
@fabpot
minor #26570 [TwigBundle] document TwigRenderer BC break in UPGRADE-3…
ae16d77 
….4 (dmaicher)

This PR was merged into the 3.4 branch.

Discussion
----------

[TwigBundle] document TwigRenderer BC break in UPGRADE-3.4

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

This is a follow up for #26553 as it seems impossible to fix the BC break in a clean way for everyone.

ping @nicolas-grekas @Tobion

Commits
-------

47a3d7a [TwigBundle] document TwigRenderer BC break in UPGRADE-3.4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Status: Needs Review TwigBridge
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
4 participants
@dmaicher @nicolas-grekas @Tobion @carsonbot
0