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

[TwigBundle] Add default templates directory and option to configure it #24179

Merged
merged 1 commit into from Sep 13, 2017

Conversation

Projects
None yet
10 participants
@yceruto
Contributor

yceruto commented Sep 12, 2017

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

Feature freeze is coming so this one should be important for the new structure. Moving forward and alternative of #23339 but I'm proposing templates/bundles/<BundleName> instead of templates/bundles/<BundleTwigNamespace> to override bundles templates and easy migration from current app/Resources/<BundleName>/views convention. Also this fix the pending comments.

Summary:

  • Added new option to configure default path for templates directory:
twig:
    default_path: '%kernel.project_dir%/templates' # default
  • Added new path convention to override bundle templates <default_path>/bundles/<BundleName>:
# Examples:
templates/bundles/TwigBundle/Exception/error.html.twig - @Twig/Exception/error.html.twig
templates/bundles/FOSUserBundle/layout.html.twig - @FOSUser/layout.html.twig

Current templates in <kernel.root_dir>/Resources/<BundleName>/views have priority over the new one, and both have priority over the bundle views path.

@yceruto yceruto changed the base branch from master to 3.4 Sep 12, 2017

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Sep 13, 2017

@mvrhov

This comment has been minimized.

Show comment
Hide comment
@mvrhov

mvrhov Sep 13, 2017

Contributor

Please no overrides dir. just keep it inside a bundles dir. Does it really matter. The one working on code knows.

Contributor

mvrhov commented Sep 13, 2017

Please no overrides dir. just keep it inside a bundles dir. Does it really matter. The one working on code knows.

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 13, 2017

Contributor

Changed to bundles directory.

Contributor

yceruto commented Sep 13, 2017

Changed to bundles directory.

@chalasr

We should have a test case for this.

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 13, 2017

Contributor

Tests added.

Contributor

yceruto commented Sep 13, 2017

Tests added.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 13, 2017

Contributor

Just a question; should we deprecate the old paths (hardcoded in src).

Ie trigger in case of if (file_exists($dir = $container->getParameter('kernel.root_dir').'/Resources/views')) { etc. Wouldnt that me more concise?

Only if not configured as path already of course.

Contributor

ro0NL commented Sep 13, 2017

Just a question; should we deprecate the old paths (hardcoded in src).

Ie trigger in case of if (file_exists($dir = $container->getParameter('kernel.root_dir').'/Resources/views')) { etc. Wouldnt that me more concise?

Only if not configured as path already of course.

@fabpot

fabpot approved these changes Sep 13, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 13, 2017

Member

Thank you @yceruto.

Member

fabpot commented Sep 13, 2017

Thank you @yceruto.

@fabpot fabpot merged commit a1b391f into symfony:3.4 Sep 13, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

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

feature #24179 [TwigBundle] Add default templates directory and optio…
…n to configure it (yceruto)

This PR was merged into the 3.4 branch.

Discussion
----------

[TwigBundle] Add default templates directory and option to configure it

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

Feature freeze is coming so this one should be important for the new structure. Moving forward and alternative of #23339 but I'm proposing `templates/bundles/<BundleName>` instead of `templates/bundles/<BundleTwigNamespace>` to override bundles templates and easy migration from current `app/Resources/<BundleName>/views` convention. Also this fix the pending comments.

Summary:
 * Added new option to configure default path for templates directory:
```yaml
twig:
    default_path: '%kernel.project_dir%/templates' # default
```
 * Added new path convention to override bundle templates  `<default_path>/bundles/<BundleName>`:
```
# Examples:
templates/bundles/TwigBundle/Exception/error.html.twig - @Twig/Exception/error.html.twig
templates/bundles/FOSUserBundle/layout.html.twig - @FOSUser/layout.html.twig
```

Current templates in `<kernel.root_dir>/Resources/<BundleName>/views` have priority over the new one, and both have priority over the bundle `views` path.

Commits
-------

a1b391f Add default templates directory and option to configure it

@yceruto yceruto deleted the yceruto:templates branch Sep 13, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 13, 2017

Member

Just tested on 2 of my projects, and it works perfectly.

Member

fabpot commented Sep 13, 2017

Just tested on 2 of my projects, and it works perfectly.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Sep 17, 2017

Member

I agree with @ro0NL that we should deprecate the old /Resources/<BundleName>/views.

  1. it's not good to have different ways for the same thing as makes things inconsistent
  2. there should be a configuration for the new path instead of hardcoding it as it makes things self-explaining, esp. if you add that to the default config file. for the same reason we removed auto-registration of commands in favor of having an explicit import (psr-4 loading and autoconfiguration or not).
Member

Tobion commented Sep 17, 2017

I agree with @ro0NL that we should deprecate the old /Resources/<BundleName>/views.

  1. it's not good to have different ways for the same thing as makes things inconsistent
  2. there should be a configuration for the new path instead of hardcoding it as it makes things self-explaining, esp. if you add that to the default config file. for the same reason we removed auto-registration of commands in favor of having an explicit import (psr-4 loading and autoconfiguration or not).
@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 17, 2017

Contributor

@Tobion at time of posting i didnt realize we need to support both variants; legacy and new projects.

#24227 (comment)

I think also related is we dont want to force users switching to a new structure for no real benefit. But the fact you can mix&match can be confusing for sure! So yes, i think the current approach is a bit too pragmatic.

Technically we're fine :)

Contributor

ro0NL commented Sep 17, 2017

@Tobion at time of posting i didnt realize we need to support both variants; legacy and new projects.

#24227 (comment)

I think also related is we dont want to force users switching to a new structure for no real benefit. But the fact you can mix&match can be confusing for sure! So yes, i think the current approach is a bit too pragmatic.

Technically we're fine :)

This was referenced Oct 18, 2017

xabbuh added a commit to symfony/symfony-docs that referenced this pull request Oct 27, 2017

minor #8549 Add default_path option reference (yceruto, javiereguiluz)
This PR was merged into the 3.4 branch.

Discussion
----------

Add default_path option reference

symfony/symfony#24179

Commits
-------

66fd357 Minor tweaks
af245fc Add default_path option reference
@nesl247

This comment has been minimized.

Show comment
Hide comment
@nesl247

nesl247 Dec 28, 2017

Can anyone tell me if this was tested / supposed to work with Form templates? Doesn't seem to be working.

Specifically templates in templates/bundles/BundleName/Form/something.html.twig does not override the original. In my case this is for https://github.com/excelwebzone/EWZRecaptchaBundle but I'm guessing this would apply to any Form templates.

nesl247 commented Dec 28, 2017

Can anyone tell me if this was tested / supposed to work with Form templates? Doesn't seem to be working.

Specifically templates in templates/bundles/BundleName/Form/something.html.twig does not override the original. In my case this is for https://github.com/excelwebzone/EWZRecaptchaBundle but I'm guessing this would apply to any Form templates.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Jan 2, 2018

Member

@nesl247 please open a new issue if you think you discovered a bug

Member

xabbuh commented Jan 2, 2018

@nesl247 please open a new issue if you think you discovered a bug

@nesl247

This comment has been minimized.

Show comment
Hide comment
@nesl247

nesl247 Jan 3, 2018

Will do. Wasn't sure if I should or not as I'm not sure if it's working as intended or a bug.

nesl247 commented Jan 3, 2018

Will do. Wasn't sure if I should or not as I'm not sure if it's working as intended or a bug.

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