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

Override Twig Template Class #4704

Closed
markhuot opened this issue Aug 3, 2019 · 4 comments

Comments

@markhuot
Copy link
Contributor

commented Aug 3, 2019

In some instances I would love to be able to hook in to Twig's display() method on each template. A few examples of why:

  • https://laravel-news.com/laravel-view-x-ray hooking in to the display and adding custom markup when in debug mode or something like that.
  • adding specific variables to the $context based on the render. E.g., set up a "hook" that adds variables to all templates that start with sidebar* or something like that.
  • call a "view model" on display to run PHP code prior to the template rendering.

Some thoughts, this could be handled with an event. In src/web/twig/Template.php you could call Event::trigger($this, 'beforeDisplayTwigTemplate', $event); but then you're calling a bunch of events and that would probably affect performance.

The other option, which I like more, in src/web/View.php add a way to override _getTwigOptions and/or customize the base_template_class via a configuration. This would then allow developers the ability to completely override the compiled template and save some performance because modifications would go in to the compiled template.

If you would consider exploring either of these, please let me know, and I will open a PR with a suggestion.

Steps to reproduce

  1. Install Craft
  2. Attempt to override the base_template_class and/or hook in to the Template::display method in some way.

Additional info

  • Craft version: All
  • PHP version: Any
  • Database driver & version: Any
  • Plugins & versions: None
@brandonkelly

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

I like the event-based option better for two reasons:

  • Changing the base class would only have an effect on newly-compiled templates. So you’d need to clear out the whole storage/runtime/compiled_templates folder as part of the plugin installation process or something. Likewise it would need to clear the folder out on uninstall as well, so sites don’t generate a bunch of Twig errors when that class is no longer available.

  • Allowing plugins to change the base class could lead to plugin conflicts, if two separate plugins are trying to change the class to their own.

@markhuot

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Makes sense. The only issue I had with the event approach was the potential performance issues. But, boy, the extensibility of multiple plugins hooking in to a beforeTemplateDisplay seems awesome!

Want me to open PR for review? I’ve been messing around with it locally to work with some template-based view-models so I’m happy to push what I have.

@brandonkelly

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Sure. And as for performance, if you wrap $this->trigger with a $this->hasEventHandlers() condition, it shouldn’t have much of an impact unless there are actually any registered event handlers. Example:

// Fire a 'beforeSaveSection' event
if ($this->hasEventHandlers(self::EVENT_BEFORE_SAVE_SECTION)) {
$this->trigger(self::EVENT_BEFORE_SAVE_SECTION, new SectionEvent([
'section' => $section,
'isNew' => $isNewSection
]));
}

@brandonkelly

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Turns out that base_template_class is a deprecated setting as of Twig 2.7 (see #4755). Per twigphp/Twig#3129 the best way to hook into before/after template rendering is with a node visitor that looks for the ModuleNode and swaps out its display_start and display_end nodes with new nodes that encompass the old nodes + custom nodes that compile whatever logic you need right into the template classes.

For example, here’s what we’re doing for Craft 3.3 (instead of overriding Template::display()):

public function leaveNode(Node $node, Environment $env)
{
if ($node instanceof ModuleNode) {
$name = $node->getTemplateName();
$node->setNode('display_start', new Node([
new ProfileNode(Template::PROFILE_STAGE_BEGIN, Template::PROFILE_TYPE_TEMPLATE, $name),
$node->getNode('display_start')
]));
$node->setNode('display_end', new Node([
new ProfileNode(Template::PROFILE_STAGE_END, Template::PROFILE_TYPE_TEMPLATE, $name),
$node->getNode('display_end')
]));

If you go this route with a plugin, make sure to call this from the install migration’s safeUp() and safeDown() methods:

FileHelper::clearDirectory(Craft::$app->getPath()->getCompiledTemplatesPath(false));
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.