Problem/Motivation

In #2160643: Add status report "requirement" for Twig C extension for PHP a status report "requirement" for Twig C extension for PHP was added. However, the wording exceeds expectations.

The justification to introducing this check was based on a) a post from 2011 on why we needed to have a C extension to speed up the slow and already-improved PHP Twig_Template::getAttribute function and b) a 2013 article which claimed we could get a perf increase of ~15%...but that was with PHP 5.3. We're now requiring 5.5.9 at a minimum, and benchmarks show PHP 7 greatly improves overall PHP performances.

https://github.com/twigphp/Twig/issues/1695 also discusses porting the Twig C extension to PHP 7 but it's unclear at this point if it'll ever happen. Solid benchmarking is required to determine if PHP 7 improvements are making the Twig C extension obsolete or if it's still something we should consider recommending. In doubt, we should no longer recommend it on the reports page and thus prevent confusion from arising as a result.

Proposed resolution

Remove the requirement altogether.

Remaining tasks

None. Patch is trivial and has been written already.

User interface changes

Reports page no longer shows a requirement for the Twig C extension.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#32 drupal-2568247-32-twig-c.patch1.2 KBgapple
#28 interdiff_26-28.txt920 bytesheddn
#28 drupal-make_the_twig_c-2568247-28.patch1.66 KBheddn
#26 interdiff_21-26.txt702 bytesheddn
#26 drupal-make_the_twig_c-2568247-26.patch1.37 KBheddn
#21 2568247-21.patch1.35 KBmstrelan
#11 2568247-11.patch995 bytesanavarre
#8 benchmark_results_ordered.png57.7 KBanavarre
twigc_wording.patch995 bytesanavarre
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anavarre created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

It would be great to actually have numbers on 5.5

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented

First, apologies for the very unscientific benchmarks on my busy laptop with Rasmus' php7dev box.

The quick methodology I used was to switch from PHP 5.5, then PHP 5.6 and finally PHP 7 without the Twig C extension and then with it.

Command used was (hitting Bartik's front page of a fresh D8 site):

$ ab -n 1000 http://d8.vbox/

...and I simply ran it twice to have 'cold' opcache numbers first.

PHP 5.5

PHP 5.5.30-dev (cli) (built: Sep 15 2015 09:58:07) 
Copyright (c) 1997-2015 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2015 Zend Technologies
    with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2015, by Zend Technologies

Without the Twig C extension

# First pass
Requests per second:    54.56 [#/sec] (mean)
Time per request:       18.328 [ms] (mean)
Time per request:       18.328 [ms] (mean, across all concurrent requests)
Transfer rate:          565.31 [Kbytes/sec] received

# Second pass
Requests per second:    43.88 [#/sec] (mean)
Time per request:       22.789 [ms] (mean)
Time per request:       22.789 [ms] (mean, across all concurrent requests)
Transfer rate:          454.65 [Kbytes/sec] received

With the Twig C extension

# First pass
Requests per second:    60.63 [#/sec] (mean)
Time per request:       16.494 [ms] (mean)
Time per request:       16.494 [ms] (mean, across all concurrent requests)
Transfer rate:          628.19 [Kbytes/sec] received

# Second pass
Requests per second:    60.90 [#/sec] (mean)
Time per request:       16.420 [ms] (mean)
Time per request:       16.420 [ms] (mean, across all concurrent requests)
Transfer rate:          631.01 [Kbytes/sec] received

PHP 5.6

PHP 5.6.14-dev (cli) (built: Sep 15 2015 09:48:20) 
Copyright (c) 1997-2015 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2015 Zend Technologies
    with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2015, by Zend Technologies

Without the Twig C extension

# First pass
Requests per second:    57.54 [#/sec] (mean)
Time per request:       17.380 [ms] (mean)
Time per request:       17.380 [ms] (mean, across all concurrent requests)
Transfer rate:          596.16 [Kbytes/sec] received

# Second pass
Requests per second:    69.14 [#/sec] (mean)
Time per request:       14.463 [ms] (mean)
Time per request:       14.463 [ms] (mean, across all concurrent requests)
Transfer rate:          716.40 [Kbytes/sec] received

With the Twig C extension

# First pass
Requests per second:    58.14 [#/sec] (mean)
Time per request:       17.201 [ms] (mean)
Time per request:       17.201 [ms] (mean, across all concurrent requests)
Transfer rate:          602.37 [Kbytes/sec] received

# Second pass
Requests per second:    54.04 [#/sec] (mean)
Time per request:       18.504 [ms] (mean)
Time per request:       18.504 [ms] (mean, across all concurrent requests)
Transfer rate:          559.94 [Kbytes/sec] received

PHP 7

PHP 7.1.0-dev (cli) (built: Sep 15 2015 10:09:40) 
Copyright (c) 1997-2015 The PHP Group
Zend Engine v3.0.0-dev, Copyright (c) 1998-2015 Zend Technologies
    with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2015, by Zend Technologies

Without the Twig C extension

# First pass
Requests per second:    55.62 [#/sec] (mean)
Time per request:       17.979 [ms] (mean)
Time per request:       17.979 [ms] (mean, across all concurrent requests)
Transfer rate:          576.24 [Kbytes/sec] received

# Second pass
Requests per second:    76.94 [#/sec] (mean)
Time per request:       12.997 [ms] (mean)
Time per request:       12.997 [ms] (mean, across all concurrent requests)
Transfer rate:          797.12 [Kbytes/sec] received

With the Twig C extension

# First pass
Requests per second:    82.25 [#/sec] (mean)
Time per request:       12.158 [ms] (mean)
Time per request:       12.158 [ms] (mean, across all concurrent requests)
Transfer rate:          852.17 [Kbytes/sec] received

# Second pass
Requests per second:    64.16 [#/sec] (mean)
Time per request:       15.585 [ms] (mean)
Time per request:       15.585 [ms] (mean, across all concurrent requests)
Transfer rate:          664.75 [Kbytes/sec] received
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented

Thanks @anavarre!

It would be interesting to see these numbers with a lot more content on the page than a clean install. Even with a clean install they still seem pretty relevant to me - 2.5ms on PHP7 with warm caches seems pretty significant and with closer to a real world scenario the difference would likely be higher. Whether this qualifies as "greatly" might be very subjective.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Are you sure we could trust 

Without the Twig C extension

on PHP 7? Its around as fast as PHP 5.5 without it. Do you mind including the standard derivation of those values?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented

Oh I read it wrong. So it's possibly making things slower on PHP 5.6 and PHP 7 when the caches are warm? Hmm.

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented

Methodology

  • Rasmus's php7dev (virtualbox/vagrant) box with switchable PHP versions
  • D8 HEAD
  • AB ran locally from within the VM

Render complexity

Used devel_generate to create 100 nodes. Used Views to create an unformatted list of all 100 nodes on the frontpage. This should be more complex to render.

Results

Without the Twig C extension

$ for i in 55 56 7 ; do newphp $i ; drush cr ; service nginx restart ; echo "====>  First pass" ; ab -n 1000 http://d8.vbox/node | egrep '(Requests per second|Time per request|Time per request|Transfer rate)' ; echo "====> Second pass" ; ab -n 1000 http://d8.vbox/node | egrep '(Requests per second|Time per request|Time per request|Transfer rate)' ; done

Activating PHP 5.5.30-dev (cli) (built: Sep 19 2015 09:02:19) and restarting php-fpm
Cache rebuild complete.                                                                                                       [ok]
====>  First pass
Requests per second:    98.38 [#/sec] (mean)
Time per request:       10.165 [ms] (mean)
Time per request:       10.165 [ms] (mean, across all concurrent requests)
Transfer rate:          6318.15 [Kbytes/sec] received
====> Second pass
Requests per second:    132.86 [#/sec] (mean)
Time per request:       7.527 [ms] (mean)
Time per request:       7.527 [ms] (mean, across all concurrent requests)
Transfer rate:          8532.66 [Kbytes/sec] received

Activating PHP 5.6.15-dev (cli) (built: Sep 19 2015 08:25:13) and restarting php-fpm
Cache rebuild complete.                                                                                                       [ok]
====>  First pass
Requests per second:    100.10 [#/sec] (mean)
Time per request:       9.990 [ms] (mean)
Time per request:       9.990 [ms] (mean, across all concurrent requests)
Transfer rate:          6428.29 [Kbytes/sec] received
====> Second pass
Requests per second:    137.25 [#/sec] (mean)
Time per request:       7.286 [ms] (mean)
Time per request:       7.286 [ms] (mean, across all concurrent requests)
Transfer rate:          8814.14 [Kbytes/sec] received

Activating PHP 7.1.0-dev (cli) (built: Sep 19 2015 08:04:20) and restarting php-fpm
Cache rebuild complete.                                                                                                       [ok]
====>  First pass
Requests per second:    129.95 [#/sec] (mean)
Time per request:       7.696 [ms] (mean)
Time per request:       7.696 [ms] (mean, across all concurrent requests)
Transfer rate:          8345.17 [Kbytes/sec] received
====> Second pass
Requests per second:    164.28 [#/sec] (mean)
Time per request:       6.087 [ms] (mean)
Time per request:       6.087 [ms] (mean, across all concurrent requests)
Transfer rate:          10550.15 [Kbytes/sec] received

With the Twig C extension

PHP 5.5

$ for i in 55 ; do newphp $i ; drush cr ; service nginx restart ; echo "====>  First pass" ; ab -n 1000 http://d8.vbox/node | egrep '(Requests per second|Time per request|Time per request|Transfer rate)' ; echo "====> Second pass" ; ab -n 1000 http://d8.vbox/node | egrep '(Requests per second|Time per request|Time per request|Transfer rate)' ; done

Activating PHP 5.5.30-dev (cli) (built: Sep 19 2015 09:02:19) and restarting php-fpm
Cache rebuild complete.                                                                                                       [ok]
====>  First pass
Requests per second:    97.37 [#/sec] (mean)
Time per request:       10.270 [ms] (mean)
Time per request:       10.270 [ms] (mean, across all concurrent requests)
Transfer rate:          6253.47 [Kbytes/sec] received
====> Second pass
Requests per second:    128.07 [#/sec] (mean)
Time per request:       7.808 [ms] (mean)
Time per request:       7.808 [ms] (mean, across all concurrent requests)
Transfer rate:          8225.00 [Kbytes/sec] received

PHP 5.6

# clear ; for i in 56 ; do newphp $i ; drush cr ; service nginx restart ; echo "====>  First pass" ; ab -n 1000 http://d8.vbox/node | egrep '(Requests per second|Time per request|Time per request|Transfer rate)' ; echo "====> Second pass" ; ab -n 1000 http://d8.vbox/node | egrep '(Requests per second|Time per request|Time per request|Transfer rate)' ; done

Activating PHP 5.6.15-dev (cli) (built: Sep 19 2015 08:25:13) and restarting php-fpm
Cache rebuild complete.                                                                                                       [ok]
====>  First pass
Requests per second:    99.48 [#/sec] (mean)
Time per request:       10.052 [ms] (mean)
Time per request:       10.052 [ms] (mean, across all concurrent requests)
Transfer rate:          6389.01 [Kbytes/sec] received
====> Second pass
Requests per second:    131.01 [#/sec] (mean)
Time per request:       7.633 [ms] (mean)
Time per request:       7.633 [ms] (mean, across all concurrent requests)
Transfer rate:          8413.68 [Kbytes/sec] received

PHP 7

Oops. Got the below issue this time so I'm currently unable to provide numbers

$ newphp 7 ; cd /var/www/default/sites/default/ ; (cd /var/www/default/core/vendor/twig/twig/ext/twig/ ; phpize ; ./configure ; make ; make install)
Activating PHP 7.1.0-dev (cli) (built: Sep 19 2015 08:04:20) and restarting php-fpm
Configuring for:
PHP Api Version:         20131218
Zend Module Api No:      20141001
Zend Extension Api No:   320140815
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for a sed that does not truncate output... /bin/sed
checking for cc... cc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables... 
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether cc accepts -g... yes
checking for cc option to accept ISO C89... none needed
checking how to run the C preprocessor... cc -E
checking for icc... no
checking for suncc... no
checking whether cc understands -c and -o together... yes
checking for system library directory... lib
checking if compiler supports -R... no
checking if compiler supports -Wl,-rpath,... yes
checking build system type... x86_64-unknown-linux-gnu
checking host system type... x86_64-unknown-linux-gnu
checking target system type... x86_64-unknown-linux-gnu
checking for PHP prefix... /usr/local/php70
checking for PHP includes... -I/usr/local/php70/include/php -I/usr/local/php70/include/php/main -I/usr/local/php70/include/php/TSRM -I/usr/local/php70/include/php/Zend -I/usr/local/php70/include/php/ext -I/usr/local/php70/include/php/ext/date/lib
checking for PHP extension directory... /usr/local/php70/lib/php/extensions/no-debug-non-zts-20141001
checking for PHP installed headers prefix... /usr/local/php70/include/php
checking if debug is enabled... no
checking if zts is enabled... no
checking for re2c... re2c
checking for re2c version... 0.13.5 (ok)
checking for gawk... no
checking for nawk... nawk
checking if nawk is broken... no
checking whether to enable twig support... yes, shared
checking for ld used by cc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for /usr/bin/ld option to reload object files... -r
checking for BSD-compatible nm... /usr/bin/nm -B
checking whether ln -s works... yes
checking how to recognize dependent libraries... pass_all
checking for ANSI C header files... yes
checking for sys/types.h... yes
checking for sys/stat.h... yes
checking for stdlib.h... yes
checking for string.h... yes
checking for memory.h... yes
checking for strings.h... yes
checking for inttypes.h... yes
checking for stdint.h... yes
checking for unistd.h... yes
checking dlfcn.h usability... yes
checking dlfcn.h presence... yes
checking for dlfcn.h... yes
checking the maximum length of command line arguments... 1572864
checking command to parse /usr/bin/nm -B output from cc object... ok
checking for objdir... .libs
checking for ar... ar
checking for ranlib... ranlib
checking for strip... strip
checking if cc supports -fno-rtti -fno-exceptions... no
checking for cc option to produce PIC... -fPIC
checking if cc PIC flag -fPIC works... yes
checking if cc static flag -static works... yes
checking if cc supports -c -o file.o... yes
checking whether the cc linker (/usr/bin/ld -m elf_x86_64) supports shared libraries... yes
checking whether -lc should be explicitly linked in... no
checking dynamic linker characteristics... GNU/Linux ld.so
checking how to hardcode library paths into programs... immediate
checking whether stripping libraries is possible... yes
checking if libtool supports shared libraries... yes
checking whether to build shared libraries... yes
checking whether to build static libraries... no

creating libtool
appending configuration tag "CXX" to libtool
configure: creating ./config.status
config.status: creating config.h
/bin/bash /var/www/default/core/vendor/twig/twig/ext/twig/libtool --mode=compile cc  -I. -I/var/www/default/core/vendor/twig/twig/ext/twig -DPHP_ATOM_INC -I/var/www/default/core/vendor/twig/twig/ext/twig/include -I/var/www/default/core/vendor/twig/twig/ext/twig/main -I/var/www/default/core/vendor/twig/twig/ext/twig -I/usr/local/php70/include/php -I/usr/local/php70/include/php/main -I/usr/local/php70/include/php/TSRM -I/usr/local/php70/include/php/Zend -I/usr/local/php70/include/php/ext -I/usr/local/php70/include/php/ext/date/lib  -DHAVE_CONFIG_H  -g -O2   -c /var/www/default/core/vendor/twig/twig/ext/twig/twig.c -o twig.lo 
mkdir .libs
 cc -I. -I/var/www/default/core/vendor/twig/twig/ext/twig -DPHP_ATOM_INC -I/var/www/default/core/vendor/twig/twig/ext/twig/include -I/var/www/default/core/vendor/twig/twig/ext/twig/main -I/var/www/default/core/vendor/twig/twig/ext/twig -I/usr/local/php70/include/php -I/usr/local/php70/include/php/main -I/usr/local/php70/include/php/TSRM -I/usr/local/php70/include/php/Zend -I/usr/local/php70/include/php/ext -I/usr/local/php70/include/php/ext/date/lib -DHAVE_CONFIG_H -g -O2 -c /var/www/default/core/vendor/twig/twig/ext/twig/twig.c  -fPIC -DPIC -o .libs/twig.o
/var/www/default/core/vendor/twig/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.
Makefile:193: recipe for target 'twig.lo' failed
make: *** [twig.lo] Error 1
/bin/bash /var/www/default/core/vendor/twig/twig/ext/twig/libtool --mode=compile cc  -I. -I/var/www/default/core/vendor/twig/twig/ext/twig -DPHP_ATOM_INC -I/var/www/default/core/vendor/twig/twig/ext/twig/include -I/var/www/default/core/vendor/twig/twig/ext/twig/main -I/var/www/default/core/vendor/twig/twig/ext/twig -I/usr/local/php70/include/php -I/usr/local/php70/include/php/main -I/usr/local/php70/include/php/TSRM -I/usr/local/php70/include/php/Zend -I/usr/local/php70/include/php/ext -I/usr/local/php70/include/php/ext/date/lib  -DHAVE_CONFIG_H  -g -O2   -c /var/www/default/core/vendor/twig/twig/ext/twig/twig.c -o twig.lo 
 cc -I. -I/var/www/default/core/vendor/twig/twig/ext/twig -DPHP_ATOM_INC -I/var/www/default/core/vendor/twig/twig/ext/twig/include -I/var/www/default/core/vendor/twig/twig/ext/twig/main -I/var/www/default/core/vendor/twig/twig/ext/twig -I/usr/local/php70/include/php -I/usr/local/php70/include/php/main -I/usr/local/php70/include/php/TSRM -I/usr/local/php70/include/php/Zend -I/usr/local/php70/include/php/ext -I/usr/local/php70/include/php/ext/date/lib -DHAVE_CONFIG_H -g -O2 -c /var/www/default/core/vendor/twig/twig/ext/twig/twig.c  -fPIC -DPIC -o .libs/twig.o
/var/www/default/core/vendor/twig/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.
Makefile:193: recipe for target 'twig.lo' failed
make: *** [twig.lo] Error 1
anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented
FileSize
benchmark_results_ordered.png57.7 KB

In case this helps...

hoff331’s picture

hoff331 CreditAttribution: hoff331 as a volunteer commented

From anavarre's second round of tests, the Twig C extension appears to be making things worse. I agree with Cottser, the word "greatly" is subjective and may set unrealistic expectations. Simply changing the status page to read, "Enabling the Twig C extension may increase rendering performance" will help until further benchmark testing is performed.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

I think we need a reroll here, the patch no longer applies. I have no strong feelings towards either the word "can" or "may" here :)

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented
Status: Needs work » Needs review
FileSize
2568247-11.patch995 bytes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

So according to the benchmarks it makes things worse, sounds like recommending in that case is kinda pointless :)

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented

#12 Honestly I'd be wary of my benchmarks. Maybe someone else can confirm/deny?

wahono77’s picture

wahono77 CreditAttribution: wahono77 commented

Without C extension, Drupal 8 is very slow after even after DOM loaded fully.

I read in http://twig.sensiolabs.org/doc/installation.html#installing-the-c-extension that Twig 1.4 is added C Extension into core.

Why Drupal 8 don't use twig 1.4. For shared hosting, install Twig C Extension is impossible.

regards

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

Hi @wahono77, we use a version much newer than 1.4. By adding to the (Twig) core they only meant it's not an additional download. As far as I know it's impossible to install a C extension in the way you are talking about (i.e. on most shared hosting with no intervention).

Are you sure it's Twig that's slowing things down? After the DOM is loaded Twig is finished its work (unless you are doing Ajax requests or something) so I'm not sure it's fair to pin any slowness you're seeing on Twig.

wahono77’s picture

wahono77 CreditAttribution: wahono77 commented

I don't know exactly what's the reason of slow (delay) after DOM loaded, all link, etc can not be clicked.
In my hosting C extension doesn't active.

Please try in http://acosinfo.com/contoh-animasi

After all content loaded, there are some millisecond delay and you can not click any URL or button there.

And after I try Drupal 8 in https://simplytest.me/ (with C extension enabled), this problem doesn't happen there.

regards

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

@wahono77 I don't think that has anything to do with Twig, sorry. It's more of a coincidence that the simplytest.me server has the C extension enabled. More likely the slowness might be coming from custom JavaScript or a JS library on your site.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@wahono77 I agree with @Cottser here, it's likely JS loading assets issue. May want to run it through http://www.webpagetest.org/ and track down the front end performance. After stuff shows in the browser it's all on the front-end performance of the CSS selectors, JS blocking calls and assets. Most likely JS blocking calls. Consider opening up a new issue for this issue. May have something to do with the asset placeholder stuff that got in before RC.

EDIT: looks like CSS/JS aggregation is not turned on your site. That should help things a bunch.

mstrelan’s picture

mstrelan CreditAttribution: mstrelan commented

This requirement should not appear if you're running PHP 7, at least until Twig compiles on PHP 7. See https://github.com/twigphp/Twig/issues/1695

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Needs work

This actually still needs the :url placeholder, this patch looks to have reverted that. #19, @mstrelan would you mind maybe wrapping this in a condition and posting a new patch with your suggestion?

mstrelan’s picture

mstrelan CreditAttribution: mstrelan commented
Status: Needs work » Needs review
FileSize
2568247-21.patch1.35 KB

Added version_compare(), removed the word "greatly" and fixed the :url placeholder.

piyuesh23’s picture

piyuesh23 CreditAttribution: piyuesh23 at QED42 commented
Issue tags: -Needs reroll
gapple’s picture

gapple
he/they
Primary language English
CreditAttribution: gapple as a volunteer commented

version_compare() expects a string, so probably best to avoid the implicit conversion from an integer.

Otherwise +1

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Needs review » Needs work

@gapple thanks, good point. We can use 7.0.0 I think.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
drupal-make_the_twig_c-2568247-26.patch1.37 KB
interdiff_21-26.txt702 bytes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented 
+++ b/core/modules/system/system.install
@@ -796,19 +796,19 @@ function system_requirements($phase) {
-  if ($phase == 'runtime') {
+  if ($phase == 'runtime' && (version_compare($phpversion, '7.0.0') < 0)) {

Can we document why we check the PHP version 7 here>

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
FileSize
drupal-make_the_twig_c-2568247-28.patch1.66 KB
interdiff_26-28.txt920 bytes

I wasn't sure how opinionated I should make the comment about performance gains with the extension so I opted to document the simple fact that a version of the extension does not exist.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

This looks perfect to me and seems to address comments from above.

Thanks @heddn et al.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs review

Given that the evidence for performance improvement is not there should we just remove it entirely?

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented

Given https://github.com/twigphp/Twig/issues/1695 and the current lack of real-life numbers, I'd also tend to agree with #30. Also, we don't know if it's ever going to be compiled for PHP7 (does it even need it?).

gapple’s picture

gapple
he/they
Primary language English
CreditAttribution: gapple as a volunteer commented
FileSize
drupal-2568247-32-twig-c.patch1.2 KB

Without a clearer benefit to installing the C extension, I agree it's probably better noted in documentation as a potential option and not a persistent item on the status page.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Reviewed & tested by the community

I've already removed/updated it from a d.o. docs page a week ago (cannot find the link ATM). Is there anything else stopping this from RTBC?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update

I think we need an issue summary and title update based on the proposed solution :)

Other than that I think it's fine, it basically looks like a revert of #2160643: Add status report "requirement" for Twig C extension for PHP.

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented
Title: Make the Twig C extension requirement less optimistic » Remove the Twig C extension requirement as it's no longer relevant for modern PHP versions
Issue summary: View changes
Issue tags: -Needs issue summary update
anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented
Title: Remove the Twig C extension requirement as it's no longer relevant for modern PHP versions » Remove the Twig C extension requirement as it needs to be proven it's still relevant for modern PHP versions
gapple’s picture

gapple
he/they
Primary language English
CreditAttribution: gapple as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Since the title and summary are updated, I think it's reasonable to move this back to RTBC

  • Cottser committed fb8cdd4 on 8.2.x 
    Issue #2568247 by heddn, anavarre, gapple, mstrelan, Cottser, dawehner,...
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
Status: Reviewed & tested by the community » Fixed

Committed fb8cdd4 and pushed to 8.2.x. Thanks!

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

This seems trivial enough, can we backport to 8.1 too?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented

It's technically a UI change so I'm a bit torn, that's why I didn't commit to 8.1.x. https://www.drupal.org/core/d8-allowed-changes

Edit: also a task.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@cottser, this should not be considered a UI change. It's documentation at best and closer to a typo, IMO. Should be UI when it affects how you interface/use the product.

I've seen questions in vanDUG slack channel asking about this and PHP7 and were confused which referenced this issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

I agree with @joelpittet an entry in the status report is not a UI. Also an incorrect recommendation could be considered a bug.

  • Cottser committed 5c01c88 on 8.1.x 
    Issue #2568247 by heddn, anavarre, gapple, mstrelan, Cottser, dawehner,...
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
Version: 8.2.x-dev » 8.1.x-dev

I'm good with that thanks @heddn @joelpittet @alexpott :)

Committed 5c01c88 and pushed to 8.1.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.