Skip to content
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

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

Merged

Conversation

@Rebolon
Copy link

Rebolon commented Jan 8, 2020

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 force-pushed the Rebolon:rebolon/template-controller-add-context branch from 8011da4 to e8922fe Jan 8, 2020
@Rebolon Rebolon changed the base branch from master to 4.4 Jan 8, 2020
@Rebolon Rebolon force-pushed the Rebolon:rebolon/template-controller-add-context branch from e8922fe to dbf8500 Jan 8, 2020
Copy link
Member

yceruto left a comment

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.

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md Outdated Show resolved Hide resolved
src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md Outdated Show resolved Hide resolved
@yceruto yceruto added this to the next milestone Jan 8, 2020
@Rebolon

This comment has been minimized.

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

This comment has been minimized.

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!

@Rebolon Rebolon force-pushed the Rebolon:rebolon/template-controller-add-context branch from dbf8500 to 6c3ba1c Jan 8, 2020
@Rebolon Rebolon requested review from dunglas, lyrixx, sroze and xabbuh as code owners Jan 8, 2020
@yceruto yceruto changed the base branch from 4.4 to master Jan 8, 2020
@Rebolon Rebolon force-pushed the Rebolon:rebolon/template-controller-add-context branch from 2773cd3 to 3d671a9 Jan 8, 2020
@Rebolon

This comment has been minimized.

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 Jan 8, 2020
@yceruto

This comment has been minimized.

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
Copy link
Member

yceruto left a comment

Look good to me after minor comment, thanks!

@Rebolon Rebolon force-pushed the Rebolon:rebolon/template-controller-add-context branch from c5d2729 to 6357c39 Jan 9, 2020
@Rebolon

This comment has been minimized.

Copy link
Author

Rebolon commented Jan 9, 2020

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

@chalasr
chalasr approved these changes Jan 9, 2020
Copy link
Member

chalasr left a comment

with minor comment

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 10, 2020

(please fix fabbot's report)

@fabpot
fabpot approved these changes Jan 10, 2020
… be sent to the template
@fabpot fabpot force-pushed the Rebolon:rebolon/template-controller-add-context branch from 7519b1f to e27b417 Jan 10, 2020
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 10, 2020

Thank you @Rebolon.

@fabpot fabpot closed this in 22bebd8 Jan 10, 2020
@fabpot fabpot merged commit e27b417 into symfony:master Jan 10, 2020
0 of 2 checks passed
0 of 2 checks passed
fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.