Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#11 | 1942490-11.patch | 1.4 KB | star-szr |
#6 | drupal-twig-during-install--1942490-6.patch | 2.08 KB | steveoliver |
#3 | drupal-twig-during-install--1942490-3.patch | 1.87 KB | steveoliver |
#1 | drupal-twig-install-time--1942490-1.patch | 1.59 KB | steveoliver |
#1 | interdiff--1942490-0-1.txt | 1.49 KB | steveoliver |
Comments
Comment #1
steveoliver CreditAttribution: steveoliver commentedFirst 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.
Comment #3
steveoliver CreditAttribution: steveoliver commentedWoops, last patch was missing Definition 'use'.
Comment #4
Fabianx CreditAttribution: Fabianx commentedI 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.
Comment #4.0
Fabianx CreditAttribution: Fabianx commentedUpdated issue summary.
Comment #5
star-szrWe 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:
Patch from #1939082-12: Convert theme_status_messages() to Twig:
Comment #6
steveoliver CreditAttribution: steveoliver commentedAttached patch is a verbatim copy of the Twig registration from CoreBundle.php, using settings() instead of hardcoded cache, debug, and auto_reload parameters.
Comment #7
star-szrLooks good to me, thanks @steveoliver!
Comment #8
Fabianx CreditAttribution: Fabianx commented+1 for RTBC
Comment #9
webchickFeel 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. :)
Comment #10
xjmWell, the method is currently protected. We would need to make it public and static?
Comment #11
star-szrThe 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.
Comment #12
steveoliver CreditAttribution: steveoliver commentedLooks good :)
Comment #13
steveoliver CreditAttribution: steveoliver commentedComment #13.0
steveoliver CreditAttribution: steveoliver commentedupdated commit message
Comment #14
RobLoachGrepped through for
registerTwig
, andCoreBundle::build()
provides the only reference.Comment #15
webchickMUCH BETTER. :D
Committed and pushed to 8.x. Thanks!
Comment #16
Fabianx CreditAttribution: Fabianx commentedOh, that works amazingly well. Another +1 for RTBC, much nicer.
Comment #17
Fabianx CreditAttribution: Fabianx commentedx-post ...
Comment #18
xjmFiled #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. :)
Comment #19
star-szrThanks @xjm, good call.
Comment #20.0
(not verified) CreditAttribution: commentedAttribution to Cottser