Closed odoucet closed 7 years ago
@jpauli what is your plan for the PHP 7 migration of the extension ?
Other extensions I saw prefered creating a specific branch for PHP7 because it needs many more changes than previous versions.
There will be another branch I think, with a new code ; but not before PHP7 is released
the issue with a different branch is that the Twig C extension lives inside the Twig repository. So creating a different branch for PHP 7 would mean creating a different version of Twig for the PHP 7 support, which would be a maintenance nightmare for the library (especially given that the PHP code is already compatible with PHP 7 in the same codebase)
Maybe the C extension should get its own repo? If not the only option is to use a big #ifdef and have both sources in the same file.
You could also easily have 2 files, and just #include the right version.
Started the PHP7 port of Twig extension as an ext7/ directory like for Pimple. You shoudl ping @fabpot for files organization, not me.
Any word on this?
I see this error on PHP7 when trying to compile:
/Twig/ext/twig/twig.c:23:40: fatal error: ext/standard/php_smart_str.h: No such file or directory
#include "ext/standard/php_smart_str.h"
^
compilation terminated.
make: *** [twig.lo] Error 1
I'd love to see this too, any update @jpauli ?
This extension is not PHP 7 ready.
thanks for all your work, looking forward to enjoying the performance boost when it's ready :)
I see that part of the PHP 7 tests are still failing so how far are we from having working version for PHP 7? Really looking forward for having this for PHP 7 too :)
+1
@iler the C extension is not compatible with PHP 7 (it cannot even compile). Twig itself it fully compatible with PHP 7 (and I don't see any failing test there). Regarding the C extension, the first thing to investigate for PHP 7 would be whether we still need the performance improvements it brings on PHP 5 (given the improvements done in PHP 7 itself). Supporting the extension on PHP 7 means rewriting the extension for PHP 7.
@stof that's great to hear! However I'm as interested of the C extension performance benefits as of Twig itself so the investigation regarding performance in PHP 7 is also important.
It looks like Drupal's performance increased with the Twig C extension on PHP 5.5, but some people doubt if there is any reasonable difference on newer PHP versions: https://www.drupal.org/node/2568247
Will the C extension make rendering even faster on PHP 7? Maybe PHP 7 support isn't necessary anymore.
Not sure whether it's a mistake or not, but it seems someone managed to compile PHP7 with the twig C extension to make this benchmark.
@cdaguerre look further down the thread, the author acknowledges his mistake, he couldn't compile the extension :p
:+1: Please add support for PHP7 to the Twig C extension. blackfire.io
suggests it but I cannot find a way to add it for our PHP7 usage.
@JHGitty You should open an issue to blackfire to make them take into account that it is not yet rewritten for PHP 7.
@stof Done. They answered (wow, they are fast!) that they put it on todo list.
Ran some quick benchmark on my local machine using Docker. Here's my results:
PHP 5.6 14.599s
PHP 5.6 + twig extension 12.361s
PHP 7 5.224s
Maybe you want to have a look at https://github.com/php7-extensions/ext-php7-testing/tree/master/twig and run the tests yourself.
@mre .. So it looks like this extension might not be needed under PHP 7 at all. Thanks for reporting.
... or it could be even faster with extension
Of course.. The issue calls for further testing.. Thanks for the efforts all.
hello, guys!
Am I right saying that I cannot get Twig extension working with php7?
I tried to compile, but getting:
twig/twig/ext/twig/twig.c:23:40: fatal error: ext/standard/php_smart_str.h: No such file or directory
@profuel Yes, the extension currently doesn't work with PHP 7.
You might want to benchmark if you really need the extension on PHP7 or if you can live without. At trivago we run PHP7 without the twig extension and we achieved 100ms backend response times nevertheless.
@xabbuh Thanks for fast response.
@mre thanks for your experience. I have to learn how to live without extension, since I'm getting error in compiled files about missing function twig_template_get_attributes
@profuel clear your cache to recompile templates without the extension (btw, recent versions of Twig use different cache keys depending on whether the extension is there or no)
@stof Thanks, I've also just learned that I have to update/disable cache for my case.
thank you guys, it was that simple, new cache works simply perfect!
FTR Drupal 8.2.x is no longer recommending the Twig C extension by lack of evidence it's still useful for modern PHP versions. See https://www.drupal.org/node/2568247 for details. We'll revisit if there's any evidence (via benchmarking, most likely) we should add it back.
@fabpot do you know when will it be ready for php 7?
No ETA, but that would be a great Christmas present :) closing this issue as we now have a PR that moves the min PHP version to PHP 7 (that's where Santa Claus could drop something soon).
I saw Twig extension is now tested against PHP7 but compile failed :
Leads to :