Skip to content

[TwigBridge] Support for Twig 3 #33039

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

Merged
merged 2 commits into from
Aug 21, 2019
Merged

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Aug 8, 2019

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

I occasionally test my projects against dev branches of the libraries I use. It's probably a bit early for this PR, but since I had to patch Symfony anyway in order to test Twig 3 in my projects, I though I might as well share the branch with you.

BC break: I had to add return types to several non-final classes of the Twig bridge. This will break applications that override these classes without adding the return types.

This PR is marked as a draft because Twig 3 is still a moving target.

@fabpot
Copy link
Member

fabpot commented Aug 21, 2019

See #33269

@fabpot
Copy link
Member

fabpot commented Aug 21, 2019

Now that #33269 has been merged, I think we can finish this one @derrabus. Twig 3.0 will be released before Symfony 5, so this is safe and will allow us to spot issues sooner than later. Thank you.

fabpot added a commit that referenced this pull request Aug 21, 2019
…(fabpot)

This PR was merged into the 4.4 branch.

Discussion
----------

[TwigBridge] Mark all classes extending twig as @Final

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes-ish
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | refs #33039
| License       | MIT
| Doc PR        | n/a

Classes defining extensions/nodes/node visitors/token parsers should not be changed. They should be final.

That would also help with Twig 3.0 which introduces type hints (including return types).

Commits
-------

d657459 [TwigBridge] Mark all classes extending twig as @Final
@derrabus derrabus force-pushed the improvement/twig-3 branch from af7d80b to 6545600 Compare August 21, 2019 08:48
@derrabus derrabus marked this pull request as ready for review August 21, 2019 08:48
@derrabus derrabus requested review from lyrixx and xabbuh as code owners August 21, 2019 08:48
@derrabus derrabus force-pushed the improvement/twig-3 branch from 6545600 to 6e108b1 Compare August 21, 2019 08:50
@derrabus derrabus force-pushed the improvement/twig-3 branch from 6e108b1 to 9447ccf Compare August 21, 2019 08:57
@derrabus
Copy link
Member Author

Ready.

Also, I'm wondering if we should backport the composer.json changes (except for the bridge, of course) to 4.4. An Symfony 4.4 application with TwigBridge 5.0 should technically be able to run Twig 3.

@fabpot
Copy link
Member

fabpot commented Aug 21, 2019

Symfony 4.4 still supports 1.x, so it cannot support 1.x/2.x and 3.0 at the same time.

@derrabus derrabus force-pushed the improvement/twig-3 branch 2 times, most recently from 81b4cf1 to b3b7570 Compare August 21, 2019 11:46
fabpot added a commit that referenced this pull request Aug 21, 2019
This PR was merged into the 4.4 branch.

Discussion
----------

Removed calls to Twig\Environment::loadTemplate()

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

This PR prepares #33039. Twig 3 does not have the `loadTemplate()` anymore, so this PR replaces calls to that method.

Commits
-------

ea9e375 Removed calls to Twig\Environment::loadTemplate().
@derrabus derrabus force-pushed the improvement/twig-3 branch from b3b7570 to 57f1451 Compare August 21, 2019 11:52
@fabpot
Copy link
Member

fabpot commented Aug 21, 2019

I've just merged 4.4 into master so that this one can be rebased.

@derrabus derrabus force-pushed the improvement/twig-3 branch from 57f1451 to b050ccc Compare August 21, 2019 12:52
@nicolas-grekas
Copy link
Member

We should also replace all @final since Symfony 4.4 by @final here.

@derrabus
Copy link
Member Author

Rebased.

@derrabus derrabus force-pushed the improvement/twig-3 branch from b050ccc to f30edca Compare August 21, 2019 12:56
@derrabus
Copy link
Member Author

We should also replace all @final since Symfony 4.4 by @final here.

Done.

@derrabus
Copy link
Member Author

Should I also remove the getName() implementations from the Twig extensions? IIRC, we only need them for Twig 1 which has been dropped on master. Do we use them for anything else?

@derrabus
Copy link
Member Author

Also, I will add return type declarations to all @finalclasses where possible.

@fabpot
Copy link
Member

fabpot commented Aug 21, 2019

getName() can be removed

@derrabus
Copy link
Member Author

Ready.

@fabpot
Copy link
Member

fabpot commented Aug 21, 2019

Thank you @derrabus.

fabpot added a commit that referenced this pull request Aug 21, 2019
This PR was squashed before being merged into the 5.0-dev branch (closes #33039).

Discussion
----------

[TwigBridge] Support for Twig 3

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

I occasionally test my projects against dev branches of the libraries I use. It's probably a bit early for this PR, but since I had to patch Symfony anyway in order to test Twig 3 in my projects, I though I might as well share the branch with you.

BC break: I had to add return types to several non-final classes of the Twig bridge. This will break applications that override these classes without adding the return types.

This PR is marked as a draft because Twig 3 is still a moving target.

Commits
-------

44ed90c Finalized all `@final` classes.
f30edca Support for Twig 3.
@fabpot fabpot merged commit 44ed90c into symfony:master Aug 21, 2019
@derrabus derrabus deleted the improvement/twig-3 branch August 21, 2019 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants