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

[TwigBundle] Use the apply tag instead of the filter tag #31290

Merged
merged 1 commit into from Apr 28, 2019

Conversation

greg0ire
Copy link
Contributor

@greg0ire greg0ire commented Apr 27, 2019

The filter has been deprecated in favor of the apply tag since Twig 2.9,
see https://twig.symfony.com/doc/2.x/tags/filter.html (apply does not
seem to have its own documentation page yet).

Q A
Branch? 3.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

⚠ Note: don't merge until Twig 2.9 and 1.40 are released, or this will block symfony releases ⚠

@greg0ire
Copy link
Contributor Author

The bug can be observed like this:

./phpunit src/Symfony/Bundle/TwigBundle/
#!/usr/bin/env php
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing src/Symfony/Bundle/TwigBundle/
.......................................................           55 / 55 (100%)

Time: 733 ms, Memory: 26.00MB

OK (55 tests, 190 assertions)

Remaining direct deprecation notices (2)

  2x: The "filter" tag is deprecated since Twig 2.9, use the "apply" tag instead.
    1x in CacheWarmingTest::testCacheIsProperlyWarmedWhenTemplatingIsAvailable from Symfony\Bundle\TwigBundle\Tests\Functional
    1x in CacheWarmingTest::testCacheIsProperlyWarmedWhenTemplatingIsDisabled from Symfony\Bundle\TwigBundle\Tests\Functional

@chalasr
Copy link
Member

chalasr commented Apr 27, 2019

This deprecation is breaking our test suite on 3.4 https://travis-ci.org/symfony/symfony/jobs/525267589#L5408 (not master because the test is marked a legacy there). Bumping the twig requirement to 2.9 is not possible on lower branches (even for master, twig 2.9 is not released yet).
We need to use the filter tag when the apply one is not available

@greg0ire
Copy link
Contributor Author

I contributed it on that branch because I did not observe the deprecation on 3.4, but I must have forgotten to update my vendors. The fix will be harder on 3.4 indeed: is there a way to detect if a tag is available from Twig?

@chalasr
Copy link
Member

chalasr commented Apr 27, 2019

There is no AFAIK :/
I thought about registering an internal twig function on 3.4 which would check the existence of a twig 2.9 class e.g. ApplyTokenParser, that could work. But @xabbuh told me that he will be able to look into this bug tonight so I suggest to wait here :)

@greg0ire
Copy link
Contributor Author

In case he does find a solution, what should we do with my PR? Close it and let his solution apply 🥁 ? Or is it best to bump the dependency?

@chalasr
Copy link
Member

chalasr commented Apr 27, 2019

I think we would better not bump it (in case Twig 2.9 is not ready in time for the Symfony 4.3 releases) but I don't know tbh :) Let's keep this open until we have more inputs

@fabpot
Copy link
Member

fabpot commented Apr 27, 2019

You can bump the minimum version. I will release the new versions of Twig 1.x and 2.x in the next few days.

@fabpot
Copy link
Member

fabpot commented Apr 27, 2019

On 3.4 :)

@greg0ire greg0ire force-pushed the use-apply-instead-of-filter branch from cc68892 to 56fe300 Compare April 27, 2019 17:58
@greg0ire greg0ire changed the base branch from master to 3.4 April 27, 2019 17:58
The filter has been deprecated in favor of the apply tag since Twig 2.9,
see https://twig.symfony.com/doc/2.x/tags/filter.html (apply does not
seem to have its own documentation page yet).
@greg0ire greg0ire force-pushed the use-apply-instead-of-filter branch from 56fe300 to 9c11b98 Compare April 27, 2019 18:55
@greg0ire
Copy link
Contributor Author

cc @xabbuh

@greg0ire greg0ire changed the title Use the apply tag instead of the filter tag [TwigBundle] Use the apply tag instead of the filter tag Apr 27, 2019
@greg0ire
Copy link
Contributor Author

I added a warning above, cause I think this should not be merged yet, right?

⚠ Note: don't merge until Twig 2.9 and 1.40 are released, or this will block symfony releases ⚠

@chalasr chalasr added this to the 3.4 milestone Apr 27, 2019
@fabpot
Copy link
Member

fabpot commented Apr 28, 2019

Thank you @greg0ire.

@fabpot fabpot merged commit 9c11b98 into symfony:3.4 Apr 28, 2019
fabpot added a commit that referenced this pull request Apr 28, 2019
…greg0ire)

This PR was merged into the 3.4 branch.

Discussion
----------

[TwigBundle] Use the apply tag instead of the filter tag

The filter has been deprecated in favor of the apply tag since Twig 2.9,
see https://twig.symfony.com/doc/2.x/tags/filter.html (apply does not
seem to have its own documentation page yet).

| Q             | A
| ------------- | ---
| Branch?       | 3.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

⚠ Note: don't merge until Twig 2.9 and 1.40 are released, or this will block symfony releases ⚠

Commits
-------

9c11b98 Use the apply tag instead of the filter tag
This was referenced May 1, 2019
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.

None yet

4 participants