8000 [FrameworkBundle] Deprecate renderView() in favor of renderTemplate() by javiereguiluz · Pull Request #36184 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle] Deprecate renderView() in favor of renderTemplate() #36184

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
Merged
merged 1 commit into from
May 5, 2020
Merged

[FrameworkBundle] Deprecate renderView() in favor of renderTemplate() #36184

merged 1 commit into from
May 5, 2020

Conversation

javiereguiluz
Copy link
Member
Q A
Branch? master
Bug fix? no
New feature? no
Deprecations? yes
Tickets -
License MIT
Doc PR -

@javiereguiluz javiereguiluz added this to the next milestone Mar 24, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.0 Mar 24, 2020
@stof stof modified the milestones: 5.0, next Mar 24, 2020
@stof
Copy link
Member
stof commented Mar 24, 2020

@nicolas-grekas I reverted the milestone. We cannot introduce a new deprecation in 5.0.x

nicolas-grekas
nicolas-grekas reviewed Mar 24, 2020
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we consider this is worth the migration cost?

src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php Outdated
@@ -241,20 +241,30 @@ protected function denyAccessUnlessGranted($attributes, $subject = null, string
* Returns a rendered view.
*/
protected function renderView(string $view, array $parameters = []): string
{
@trigger_error('Since symfony/framework-bundle 5.1: "renderView()" method is deprecated. Use "renderTemplate()" instead.', E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trigger_deprecation() + missing annotation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh also: changelog+upgrade files should be updated

@javiereguiluz
Copy link
Member Author

About the migration cost ... I would never change the render() method because everybody uses it ... but renderView() is much less popular (in my experience and from what I've seen out there: render() is used more than 100 times more often that renderView() ... but this is only my experience).

@dmaicher
Copy link
Contributor

About the migration cost ... I would never change the render() method because everybody uses it ... but renderView() is much less popular (in my experience and from what I've seen out there: render() is used more than 100 times more often that renderView() ... but this is only my experience).

Seems about right. Not exactly a factor of 100 but here some numbers from my biggest Symfony 4.4 work project:

$ grep -r --include *Controller.php  'this->render(' src/ vendor/ | wc -l
180

$ grep -r --include *Controller.php  'this->renderView(' src/ vendor/ | wc -l
14

@michaljusiega
Copy link
Contributor

Another point. MVC division: Model, view, controller. Most programmers associate with such an approach that the controller generates a view, not a template.

nicolas-grekas
nicolas-grekas reviewed Mar 25, 2020
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php Outdated
@@ -242,7 +242,7 @@ protected function denyAccessUnlessGranted($attributes, $subject = null, string
*/
protected function renderView(string $view, array $parameters = []): string
{
@trigger_error('Since symfony/framework-bundle 5.1: "renderView()" method is deprecated. Use "renderTemplate()" instead.', E_USER_DEPRECATED);
@trigger_deprecation('Since symfony/framework-bundle 5.1: "renderView()" method is deprecated. Use "renderTemplate()" instead.', E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/@//

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well actually that's broken, you'll figure out :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(don't miss the annotation also)

xabbuh
xabbuh reviewed Mar 27, 2020
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php Outdated
@@ -241,20 +241,30 @@ protected function denyAccessUnlessGranted($attributes, $subject = null, string
* Returns a rendered view.
*/
protected function renderView(string $view, array $parameters = []): string
{
trigger_deprecation('Since symfony/framework-bundle 5.1: "renderView()" method is deprecated. Use "renderTemplate()" instead.', E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "renderView()" method [...]?

nicolas-grekas
nicolas-grekas reviewed Mar 27, 2020
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php Outdated
@@ -241,20 +241,30 @@ protected function denyAccessUnlessGranted($attributes, $subject = null, string
* Returns a rendered view.
*/
protected function renderView(string $view, array $parameters = []): string
{
trigger_deprecation('Since symfony/framework-bundle 5.1: "renderView()" method is deprecated. Use "renderTemplate()" instead.', E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's not the signature of the function, and the annotation is missing

nicolas-grekas
nicolas-grekas requested changes Mar 31, 2020
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(just to confirm this needs a few more hops)

@xabbuh xabbuh modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot force-pushed the deprecate_view branch from 114b04a to 7b9ff2a Compare May 5, 2020 06:26
@fabpot
Copy link
Member
fabpot commented May 5, 2020

Thank you @javiereguiluz.

@fabpot fabpot mentioned this pull request May 5, 2020
Closed
@fabpot fabpot merged commit b494beb into symfony:master May 5, 2020
@fabpot
Copy link
Member
fabpot commented May 5, 2020

Actually, that's a hard BC break. What if one already has a renderTemplate() method in their controllers? ... well, it happens that EasyAdmin does this, and it is broken now. Shall we revert?

@nicolas-grekas
Copy link
Member

Sure, let's revert if it needs more thoughts.

@javiereguiluz
Copy link
Member Author

@fabpot I hadn't thought about that ... but then, we'll never be able to add any new method to AbstractController ... because someone may have already defined it 🤔

@fabpot
Copy link
Member
fabpot commented May 5, 2020

@javiereguiluz That's correct :(

@fabpot
Copy link
Member
fabpot commented May 5, 2020

reverted

fabpot added a commit that referenced this pull request May 5, 2020
@fabpot
Revert "feature #36184 [FrameworkBundle] Deprecate renderView() in fa…
ad01068 
…vor of renderTemplate() (javiereguiluz)"

This reverts commit b494beb, reversing
changes made to b9d4149.
@javiereguiluz
Copy link
Member Author
javiereguiluz commented May 5, 2020 •
edited
Loading

A possible solution would be to deprecate defining custom renderTemplate() methods and adding it in Symfony 6.0:

public function renderView(...)
{
    if (method_exists($this, 'renderTemplate')) {
        @trigger_deprecation('
            You define a custom renderTemplate() method in your controllers,
            but Symfony will add that method in Symfony 6.0, so you should
            rename your method.
        ');
    }

    // ...
}

But then we could only deprecate renderView() in Symfony 6 and remove it in Symfony 7 (to be released in November 2023). Maybe the simplest solution is to keep the renderView() and that's it.

@fabpot fabpot mentioned this pull request May 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@ro0NL ro0NL ro0NL left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas requested changes

@fabpot fabpot fabpot approved these changes

@xabbuh xabbuh xabbuh approved these changes

Assignees
No one assigned
Labels
Deprecation FrameworkBundle Status: Reviewed
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
9 participants
@javiereguiluz @stof @dmaicher @michaljusiega @fabpot @nicolas-grekas @ro0NL @xabbuh @carsonbot
0