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 twig_to_array is deprecated #1337

Merged
merged 1 commit into from
Dec 26, 2023

Conversation

WebMamba
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
Issues
License MIT

@smnandre
Copy link
Member

It's the same bug for all the pacakges failing in the CI ?

@WebMamba
Copy link
Contributor Author

With this PR in complement #1330 the CI should be green again! 😁

// since twig/twig 3.9.0: Using the internal "twig_to_array" function is deprecated.
$compiler
->write('$toArray = function ($data) {')
->write('if (method_exists(Twig\Extension\CoreExtension::class, \'toArray\')) {')
Copy link
Member

Choose a reason for hiding this comment

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

I think this test should be done on compile time and not on render time... Or we will call method_exists a billion time per render..

So i'd make something like

if (method_exists(...))  {
$compiler->write('return Twig\Extension\CoreExtension::toArray($data);')
} else {
$compiler->write('return twig_to_array'...)
}

Copy link
Member

Choose a reason for hiding this comment

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

That's a nice idea 👍

@smnandre
Copy link
Member

Could we merge this one @weaverryan @kbond ?

( if it's all good for you @WebMamba )

I'd like to bring the green back on the CI, having a better vision of current PR checks

@kbond kbond force-pushed the fix_twig_to_array_deprecated branch from 16f0c79 to 2a2ab7c Compare December 26, 2023 15:53
@kbond
Copy link
Member

kbond commented Dec 26, 2023

Thanks Matheo.

@kbond kbond merged commit 4cee6ac into symfony:2.x Dec 26, 2023
5 of 6 checks passed
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.

5 participants