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

Fix deprecation in ForbiddenFilterRule.php #310

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Aug 30, 2024

Deprecate the filter node of FilterExpression

Needed for Twig 3.12.0

See twigphp/Twig#4184

Do we need to bump the minimum required Twig version?

@VincentLanglet
Copy link
Owner

Deprecate the filter node of FilterExpression

Needed for Twig 3.12.0

Shouldn't the same be done for FunctionExpression too ?
https://github.com/VincentLanglet/Twig-CS-Fixer/blob/main/src/Rules/Node/ForbiddenFunctionRule.php#L38
https://github.com/VincentLanglet/Twig-CS-Fixer/blob/main/src/Rules/Node/ValidConstantFunctionRule.php#L23

Do we need to bump the minimum required Twig version?

No, I add to extends the lower bound support of twig/twig for drupal and some others CMS so i'll keep it.
#306

We need to support both, with something like

$node->hasAttribute('twig_callable')
  ? $node->getAttribute('twig_callable')->getName() // twig/twig >= 3.12
  : $node->getNode('filter')->getAttribute('value');

@ruudk
Copy link
Contributor Author

ruudk commented Aug 30, 2024

Thanks. It turns out it can be done much simpler 😁

@VincentLanglet
Copy link
Owner

We need to support both, with something like

$node->hasAttribute('twig_callable')
  ? $node->getAttribute('twig_callable')->getName() // twig/twig >= 3.12
  : $node->getNode('filter')->getAttribute('value');

For the phpstan issue it will be

if ($node->hasAttribute('twig_callable')) {
    $twigCallable = $node->getAttribute('twig_callable');
    Assert::instanceof($twigCallable, TwigFilter::class);
    $filterName = $twigCallable->getName();
} else {
    // BC for twig/twig < 3.12
    $filterName = $node->getNode('filter')->getAttribute('value');
}

@ruudk
Copy link
Contributor Author

ruudk commented Aug 30, 2024

I guess not, the name attribute was only added in the latest version. Force pushed for another try. This looks better?

@VincentLanglet
Copy link
Owner

You need #311 and to rebase after

@VincentLanglet
Copy link
Owner

I guess not, the name attribute was only added in the latest version. Force pushed for another try. This looks better?

What about FunctionExpression ? Should it be fixed too ?

Also @ruudk , I don't have any deprecation when running my tests, I would be interested if you know how to have them displayed by PHPUnit to confirm

  • I have deprecation before
  • And I have no deprecation after your PR

@ruudk
Copy link
Contributor Author

ruudk commented Aug 30, 2024

Good idea. Let me try to enable that first, so you are aware of these deprecations automatically.

@ruudk
Copy link
Contributor Author

ruudk commented Aug 30, 2024

We need to install the Symfony PHPUnit Bridge in order to get the deprecations to fail the test.

But this also immediately will give you a problem for the future: the Symfony PHPUnit Bridge is not compatible with PHPUnit 10 / 11. See symfony/symfony#49069.

But luckily, PHPUnit 11.2 also has this functionality now. But for Symfony (not related to you I guess) there is another issue: symfony/symfony#53812

So what to do? Use the PHPUnit Bridge for now and enable the deprecations helper?

@VincentLanglet
Copy link
Owner

I will wait Phpunit 11.2 then

@ruudk
Copy link
Contributor Author

ruudk commented Aug 30, 2024

What about FunctionExpression ? Should it be fixed too ?

That already uses ->getAttribute('name')

@VincentLanglet
Copy link
Owner

What about FunctionExpression ? Should it be fixed too ?

That already uses ->getAttribute('name')

Ok, can you just rebase main @ruudk

> Deprecate the `filter` node of `FilterExpression`

Needed for Twig 3.12.0

See twigphp/Twig#4184
@VincentLanglet VincentLanglet merged commit a0e552f into VincentLanglet:main Aug 30, 2024
17 checks passed
@VincentLanglet
Copy link
Owner

Thanks @ruudk, Do you have other findings related to the 3.12 version or should i release soon ?

@ruudk
Copy link
Contributor Author

ruudk commented Aug 30, 2024

Not that I know. It worked when we upgraded this morning. But since I fixed a few deprecations in our project I went to see if there were any deprecations here. So I guess all just works fine ☺️

@ruudk ruudk deleted the patch-1 branch August 30, 2024 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants