Proposed commit message:

Issue #1942490 by Cottser, steveoliver, larowlan: Make Twig engine available during install.

In order to convert some of the lower level themeables like form elements, we'll need Twig to be available during install. Attached patch does this. This will unblock issues like #1898480: [meta] form.inc - Convert theme_ functions to Twig.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

steveoliver’s picture

First patch I thought we'd want to pair down Twig at install time. This patch uses all the same settings as is used normally after install.

Status: Needs review » Needs work

The last submitted patch, drupal-twig-install-time--1942490-1.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review
FileSize
1.87 KB

Woops, last patch was missing Definition 'use'.

Fabianx’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

I don't really like that we are duplicating code all over the place, but the install container is only meant temporary, so this is fine and not worth the trouble of creating a twig_setup function in container.

=> RTBC
=> Priority: Major, this blocks conversion work
=> Updated issue summary with commit message to include larowlan.

This was also proposed by larowlan in http://drupal.org/node/1939082#comment-7205220.

Fabianx’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

We definitely need to get this in - it's blocking more and more conversion issues. We talked about this issue briefly in IRC yesterday but I hadn't had a chance to review the patch yet. If we're not worried about code duplication, why not just a straight copy and paste of Drupal\Core\CoreBundle::registerTwig() as in #1939082-12: Convert theme_status_messages() to Twig?

My biggest concern is the hardcoding of Twig settings. At least during conversion, having debug and others available would be very useful.

This patch:

+++ b/core/includes/install.core.incundefined
@@ -374,6 +375,31 @@ function install_begin_request(&$install_state) {
+        'cache' => TRUE,
...
+        'debug' => FALSE,
+        'auto_reload' => FALSE,

Patch from #1939082-12: Convert theme_status_messages() to Twig:

+++ b/core/includes/install.core.incundefined
@@ -374,6 +375,33 @@ function install_begin_request(&$install_state) {
+        'cache' => settings()->get('twig_cache', TRUE),
...
+        'debug' => settings()->get('twig_debug', FALSE),
+        'auto_reload' => settings()->get('twig_auto_reload', NULL),
steveoliver’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.08 KB

Attached patch is a verbatim copy of the Twig registration from CoreBundle.php, using settings() instead of hardcoded cache, debug, and auto_reload parameters.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks @steveoliver!

Fabianx’s picture

+1 for RTBC

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Feel free to bounce back to RTBC, but a quick question.

Is there a reason we didn't just change the code in CoreBundle.php to do things like this: 'auto_reload' => settings()->get('twig_auto_reload', NULL), and then simply call registerTwig from here?

This seems like an awful lot of duplicated code, and the part that's not duplicated seems like it would make sense for both CoreBundle.php and installer.

I'm actually surprised that settings() works here, but that's cool. :)

xjm’s picture

Is there a reason we didn't just change the code in CoreBundle.php to do things like this: 'auto_reload' => settings()->get('twig_auto_reload', NULL), and then simply call registerTwig from here?

Well, the method is currently protected. We would need to make it public and static?

star-szr’s picture

FileSize
1.4 KB

The code being added to install.core.inc in #6 is entirely duplicated from CoreBundle. CoreBundle::registerTwig() has the settings() code as well, and this is what I brought up in #5, that Twig in the installer should behave the same way and respect settings added to settings.php.

Here's a patch that does #10. The patch works as expected locally – I'm able to install i.e. #1939082-3: Convert theme_status_messages() to Twig and #1885564-10: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig, but am not able to without the patch. Let's see what testbot thinks.

steveoliver’s picture

Looks good :)

steveoliver’s picture

Status: Needs review » Reviewed & tested by the community
steveoliver’s picture

Issue summary: View changes

updated commit message

RobLoach’s picture

Grepped through for registerTwig, and CoreBundle::build() provides the only reference.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

MUCH BETTER. :D

Committed and pushed to 8.x. Thanks!

Fabianx’s picture

Status: Fixed » Reviewed & tested by the community

Oh, that works amazingly well. Another +1 for RTBC, much nicer.

Fabianx’s picture

Status: Reviewed & tested by the community » Fixed

x-post ...

xjm’s picture

Filed #1957042: Document why CoreBundle::registerTwig() is public and static. I think I had mute on when I tried to suggest this to @webchick on the phone. :)

star-szr’s picture

Thanks @xjm, good call.

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

Anonymous’s picture

Issue summary: View changes

Attribution to Cottser