Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Removed support for PHP templating everywhere#31800

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromyceruto:rm_templating_integration
Jun 4, 2019

Conversation

@yceruto
Copy link
Member

@ycerutoyceruto commentedJun 3, 2019•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

Ref:#21035 and7169f4d

TODO:
Missing deprecations in 4.4 (#31800):

  • Symfony\Bundle\FrameworkBundle\Controller\TemplateController 2nd argument.
  • Symfony\Bundle\TwigBundle\CacheWarmer\TemplateCacheCacheWarmer class and service.

Labels:FrameworkBundle,TwigBundle,TwigBridge,SecurityBundle,Form,HttpKernel

Friendly ping@fabpot and@dunglas

Koc reacted with hooray emoji
@fabpot
Copy link
Member

@yceruto Some tests are broken with this PR.

@ycerutoycerutoforce-pushed therm_templating_integration branch from5f88db9 to07020d0CompareJune 3, 2019 11:51
@yceruto
Copy link
MemberAuthor

Tests are green now.

private$templating;

publicfunction__construct(Environment$twig =null,EngineInterface$templating =null)
publicfunction__construct(Environment$twig =null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like twig should not be optional in this class. And when twig is not installed, the TemplateController should be removed from DI.

Copy link
MemberAuthor

@ycerutoycerutoJun 3, 2019•
edited
Loading

Choose a reason for hiding this comment

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

I agree, but for DX point of view it's better as is, because you'll get a good error message:

You can not use the TemplateController if the Twig Bundle is not available.

vs

"Too few arguments to function Symfony\Bundle\FrameworkBundle\Controller\TemplateController::__construct(), 0 passed in ...

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it's a little better for DX. But IMO it's not the responsiblity of the Controller do provide that.
We could just catch the instantiation error in

return$this->configureController(parent::instantiateController($class),$class);
and throw a better exception. It's even in the same namespace and the correct class to enhance DX. Then it's clear it's just for DX and we don't need to hack around it in the actual implementations.

@yceruto
Copy link
MemberAuthor

Requires#31821

Status: Needs Work

@nicolas-grekas
Copy link
Member

Rebase unlocked.

@ycerutoycerutoforce-pushed therm_templating_integration branch from9686ff3 to116be1fCompareJune 3, 2019 21:03
@ycerutoycerutoforce-pushed therm_templating_integration branch from116be1f tod5be373CompareJune 3, 2019 21:17
@yceruto
Copy link
MemberAuthor

(Rebased)

Status: Needs Review

Copy link
Contributor

@TobionTobion left a comment

Choose a reason for hiding this comment

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

Fine for me.https://github.com/symfony/symfony/pull/31800/files#r290086885 can be done in a separate PR.


if (!$this->container->has('twig')) {
thrownew \LogicException('You can not use the "renderView" method if theTemplating Component or theTwig Bundle are not available. Try running "composer require symfony/twig-bundle".');
thrownew \LogicException('You can not use the "renderView" method if the Twig Bundle are not available. Try running "composer require symfony/twig-bundle".');
Copy link
Member

Choose a reason for hiding this comment

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

is not available

Copy link
Member

Choose a reason for hiding this comment

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

@fabpot
Copy link
Member

Thank you@yceruto.

@fabpotfabpot merged commitd5be373 intosymfony:masterJun 4, 2019
fabpot added a commit that referenced this pull requestJun 4, 2019
This PR was merged into the 5.0-dev branch.Discussion----------Removed support for PHP templating everywhere| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -Ref:#21035 and7169f4d**TODO:**Missing deprecations in 4.4 (#31800): - [x] `Symfony\Bundle\FrameworkBundle\Controller\TemplateController` 2nd argument. - [x] `Symfony\Bundle\TwigBundle\CacheWarmer\TemplateCacheCacheWarmer` class and service.**Labels:** `FrameworkBundle`, `TwigBundle`, `TwigBridge`, `SecurityBundle`, `Form`, `HttpKernel`Friendly ping@fabpot and@dunglasCommits-------d5be373 Removed support for PHP templating everywhere
@ycerutoyceruto deleted the rm_templating_integration branchJune 5, 2019 17:41
fabpot added a commit that referenced this pull requestJul 3, 2019
…ion (xabbuh)This PR was merged into the 4.3 branch.Discussion----------[FrameworkBundle] deprecate the framework.templating option| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets |#32120| License       | MIT| Doc PR        |The config node has already been removed in the `master` branch in#31800. For DX it would have been better to have this deprecation in 4.3 (see e.g.#32120), but it's probably too late to ship this as a bugfix.Commits-------ba241ce deprecate the framework.templating option
@fabpotfabpot mentioned this pull requestNov 12, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@xabbuhxabbuhAwaiting requested review from xabbuh

+1 more reviewer

@TobionTobionTobion approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

6 participants

@yceruto@fabpot@nicolas-grekas@Tobion@javiereguiluz@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp