Skip to content

[FrameworkBundle] TemplateController should accept extra arguments to be sent to the template #35257

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
Jan 10, 2020
Merged

[FrameworkBundle] TemplateController should accept extra arguments to be sent to the template #35257

merged 1 commit into from
Jan 10, 2020

Conversation

Rebolon
Copy link

@Rebolon Rebolon commented Jan 8, 2020 •
edited by yceruto
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #...
License MIT
Doc PR symfony/symfony-docs#...

In the official documentation (symfony.com/doc/master/templates.html#rendering-a-template-directly-from-a-route) it says that TemplateController should accept extra arguments. In fact it's not available for instance.
So i added the context argument as an array. Because of deprecation of templating, only the twig instance will apply the context argument.

It will need to be implemented in branch 5.

The following issue has been created in documentation project : symfony/symfony-docs#12897

@Rebolon Rebolon changed the base branch from master to 4.4 January 8, 2020 11:25
yceruto
yceruto requested changes Jan 8, 2020
View reviewed changes
Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

Hi @Rebolon, thanks for taking care of this. I left some comments to make your "first contribution" happy :)

  1. The target branch must be master for new features (future 5.1), 4.4 is already released and must be used for bugfix only. Please, rebase it to master.
  2. A functional test case is required for new features like this one.

Don't hesite to ask any question, thanks again.

@yceruto yceruto added this to the next milestone Jan 8, 2020
@Rebolon
Copy link
Author

Rebolon commented Jan 8, 2020

@yceruto thx for help.
Not sure to get time for the Functional Test. I understand the goal, but not sure how to achieve it throught this project in short time (i'm looking for it).

@yceruto
Copy link
Member

yceruto commented Jan 8, 2020

Not sure to get time for the Functional Test. I understand the goal, but not sure how to achieve it throught this project in short time (i'm looking for it).

Don't worry about it, I can add it at the end, just let me know when the other comments have been fixed. Thanks!

@yceruto yceruto changed the base branch from 4.4 to master January 8, 2020 15:47
@nicolas-grekas nicolas-grekas mentioned this pull request Jan 8, 2020
Merged
4 tasks
@Rebolon
Copy link
Author

Rebolon commented Jan 8, 2020

@yceruto it should be ok right now
I added an Integration Test as i needed Twig. If you consider you need a Functional Test, it's up to you, or just give me tips on the wished usage (WebTestCase or Panther, anything else)

@Rebolon Rebolon requested a review from yceruto January 8, 2020 17:06
@yceruto
Copy link
Member

yceruto commented Jan 8, 2020

Wow! you were fast ⚡ :)

An unit test is enough for me, but can you confirm that this is the new route config for this feature?

# config/routes.yaml
acme_privacy:
    path:          /privacy
    controller:    Symfony\Bundle\FrameworkBundle\Controller\TemplateController
    defaults:
        template:  'static/privacy.html.twig'
        maxAge:    86400
        sharedAge: 86400

        # optionally you can define some arguments passed to the template
        context:
            site_name: 'ACME'
            theme: 'dark'

yceruto
yceruto approved these changes Jan 8, 2020
View reviewed changes
Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

Look good to me after minor comment, thanks!

@Rebolon
Copy link
Author

Rebolon commented Jan 9, 2020 •
edited
Loading

@yceruto That's it for the yaml config.
I pushed the last changes following your review.

chalasr
chalasr approved these changes Jan 9, 2020
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

with minor comment

@nicolas-grekas
Copy link
Member

(please fix fabbot's report)

@fabpot
Copy link
Member

fabpot commented Jan 10, 2020

Thank you @Rebolon.

@fabpot fabpot mentioned this pull request Jan 10, 2020
Closed
@fabpot fabpot closed this in 22bebd8 Jan 10, 2020
@fabpot fabpot merged commit e27b417 into symfony:master Jan 10, 2020
@HeahDude HeahDude mentioned this pull request Feb 9, 2020
Closed
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@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

@yceruto yceruto yceruto approved these changes

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@chalasr chalasr chalasr approved these changes

@dunglas dunglas Awaiting requested review from dunglas

@lyrixx lyrixx Awaiting requested review from lyrixx

@sroze sroze Awaiting requested review from sroze

@xabbuh xabbuh Awaiting requested review from xabbuh

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

Successfully merging this pull request may close these issues.
6 participants
@Rebolon @yceruto @nicolas-grekas @fabpot @chalasr @carsonbot