Drupal Core imho is lacking a clean way of adding a TWIG filter or function to a custom module.

It would be really nice if this could be done with the new found services / annotations system, and fortunally enough Erik Seifert created such a system in a contrib module called Twig Extender

For example adding a |truncate filter could easily become one class doing :

namespace Drupal\twig_extender\Plugin\TwigPlugin;

use Drupal\twig_extender\Plugin\Twig\TwigPluginBase;
use Drupal\Component\Utility\Unicode;

/**
 * Example plugin for truncate
 *
 *
 * @TwigPlugin(
 *   id = "twig_extender_truncate",
 *   label = @Translation("Truncate string"),
 *   type = "filter",
 *   name = "truncate",
 *   function = "truncate"
 * )
 */

class Truncate extends TwigPluginBase {
  public function truncate($string, $max_length,$wordsafe = FALSE, $add_ellipsis = FALSE, $min_wordsafe_length = 1) {
    return Unicode::truncate($string, $max_length, $wordsafe , $add_ellipsis , $min_wordsafe_length);
  }
}

source : http://cgit.drupalcode.org/twig_extender/tree/src

I personally believe adding this to core instead of adding a dependency to contrib for every project that wants to extend TWIG. This could save a lot of modules, including one of my own, in re-inventing the wheel on adding Twig extension to a project. Also having a unified way to adding TWIG filters and functions will help DX a lot.

At this moment 8.1.x is in beta, so i guess it's to late to add it to that version, however the impact could be minimal, so it can be added to point release without any issues. Would be nice to convert the core filters and functions, but it's not trivial for a release (IMHO!)

I did not create a patch, because I wanted to check with core devs first if this idea is feasible.

Comments

Rene Bakx created an issue. See original summary.

Cottser’s picture

Version: 8.1.x-dev » 8.2.x-dev
Component: base system » theme system

First of all: thanks for taking the time to submit this! Forgive me if I'm missing something, at this point I'm trying to understand the benefits and long term considerations (maintenance, standardization, etc.) of this proposal. I'm moving the version to 8.2.x because as a new feature/API this is too late to get into 8.1.x because 8.1.x is in beta now: https://www.drupal.org/core/d8-allowed-changes#minor

I'm not necessarily opposed to this proposal but I must say it doesn't seem very compelling to me at the moment. It would be helpful to know more about how our current approach isn't unified as well as what the benefits of this proposal are, that is not clear to me.

Unless I'm mistaken with the proposal here: what you are gaining is no need to create/add to mymodule.services.yml and the removal of some boilerplate? The following change record shows how to add a Twig extension in contrib currently: https://www.drupal.org/node/1831138#twig-extensions

Basically:

  • yourmodule.services.yml, which tags a service class as twig.extension (and you can also set the priority here for what it's worth), pointing to:
  • A class which extends \Twig_Extension, defining filters, functions, etc. and the code for those filters and functions can either be in this class or a separate class.

IMO this also has the benefit of being essentially identical to how it works in Symfony land: http://symfony.com/doc/current/cookbook/templating/twig_extension.html

Also, if your Twig extension wanted to define multiple filters, functions, etc. wouldn't this plugin approach result in more files required? One file per filter/function/etc. because they would each require their own plugin annotation if I'm not mistaken?

joelpittet’s picture

To simplify and understand. This is a proposal to use plugin, annotation discovery in addition to the tagged services approach we have already.

While I don't know how many people would prefer this, I really like annotation discovery because the the "info" declaration is in context with the thing it's describing instead of in a separate file (MODULE.services.yml).

We should think hard about what this means as @Cottser mentioned for maintainability and any potential drawbacks of having another way to do this and hopefully the benefits outweigh them but in general i'm a +1 for this.

It would be good to have someone from the plugin system to weigh in here.

EclipseGc’s picture

So, some general thoughts here without really weighing in on whether it's specifically a good idea or not (I leave that to those with more familiarity with the specifics).

As Joel pointed out in #3, annotations place information directly on the implementation. You're NEVER going to find a plugin class and wonder "how does this get used?". Services are essentially the opposite of that. They blend in with the classes around them and have virtually nothing characterizing them as services. This isn't necessarily bad, but when you have a set of classes that are intended to be used as kind of a "cloud" of possible implementations, I really dislike services for this. Tagged services attempt to do this, but there are a number of drawbacks here, not the least of which is complete lack of introspectability and metadata.

That being said, let's acknowledge that I am likely a bit biased on this topic and look at the other side of the coin. Cottser asks a great question in #2 around "more classes" and while the implementation could vary in such a way to prevent that, I don't think it would improve readability. I'd also argue that "more classes" isn't necessarily a problem, the bigger problem is identifying what the class you're looking at does, and how. The faster you can identify that, the less time you waste scratching your head. However, looking at this specific implementation, the addition of arbitrarily named public methods does give me pause, but I'm betting the tagged service in core does exactly the same thing, so probably a fairly even trade off.

The bigger wtf imo is doing it both ways. I understand the argument around conforming to Symfony's approach, however if we adopt that as our status quo, we'll see the expansion of this particular approach (which in general I don't support). I guess I'll leave you with this: There's been a lot of talk about reviving Contemplate module now that we have twig. If you had a plugin system backing your twig filters like this it'd be trivial to add a human-readable, translate-able list of them into a UI for building twig templates. If something like that is appealing, then I'd ++ the plugin approach (or if you have any other twig filter UI ideas). If you can't see a world where anything I'm proposing feels relevant, then stick with tagged services.

Eclipse

Cottser’s picture

Thanks @joelpittet and @EclipseGc for the thoughts, super helpful! The Contemplate point is a great one, that could be very nice. I do agree that having the annotation inline is helpful in figuring out what things are doing, and I also agree that more PHP classes are not necessarily a bad thing :)

Having had a few more days to think about this, the other point I want to add is that perhaps we want to think about creating a plugin API for more than just filters and functions. One option for doing so would be to have @TwigFilter, @TwigFunction, @TwigTest, etc. rather than a 'type' parameter on @TwigPlugin (I think I would prefer this approach personally but not sure if it would be consistent or inconsistent with other plugins). I would also prefer to see interface(s) with specific method name(s) rather than the use of a 'function' parameter.

Getting back to the Twig extension type point, we don't necessarily need to cover every extension type but I think just doing filters and functions might not be enough. If we go the annotation route, use it in core, and promote its use in contrib, limiting our "Twig plugins API" to filters and functions might not be very friendly when people want to extend Twig in a different way (like adding a global or test) and find that the API is completely different. If you're doing something really advanced/rare (like adding a tag) I think it's perfectly OK for that to use a different method (like tagged services) but I do want to make sure this is something we think about. http://twig.sensiolabs.org/doc/advanced.html has a nice little table giving an idea of the difficulty/frequency of the different types of extensions, that might be helpful in deciding how far we might want to go with the plugin implementation(s) of Twig extension types.

Rene Bakx’s picture

Thanks to all three of you for your thoughts and insights on this!

That extra classes part, was something I overlooked. Yes, in some cases it could help creating clean code.

For me it was all about the annotations, I know that the current way is the symfony way, nothing wrong with that. But Drupal added some annotation sugar the other constructions that easily could have been done as .yml service ( @RestResource springs to mind, but that is because I just build a dozen of those)

I do admit i'm not 100% up to speed when it comes to the annotation discovery process in D8, but if possible it would be really nice if .theme files are scanned for annotations so a themer could create a theme specific twig filter/function without creating a new module and all. (just brainstorming here ;))

I like the idea of @Cottser to split the annotations in different types, and I do think we should keep it at @TwigFilter, @TwigFunction and @TwigTest. Those are the most common extensions needed when moving 'logic' from twig back to php. NodeVisitors and TokenParsers are rarely needed, and too complex to be created through annotations, and are better off in the service space.

Tests could be a welcome addition. I sometimes create a simple test in PHP instead of creating a nested if/else construction in TWIG. ( {%if node is renderable %} pops to mind as type this reply.

But I'am more worried about the god class TwigPluginBase and I should have specified in the original draft that I wanted to further investigate on remove that dependency.
In theory there shouldn't be any need for it, however I do believe it was created to circumvent the Twig_Simple.. methods :

new \Twig_SimpleFunction('link', array($this, 'getLink'))

without creating complexity in the creating of those callables. Twig needs a callable wrapped in that Twig_Simple.. method and the hard nut to crack is keeping it SRP without creating a complex system when the filter is part of an other class.

If the filter or function is defined in a class that is not a instance of a Twig_Extension, it would mean that this class containing the filters etc, needs to be initialized and available on Twig renderer level, even if the rest of the class has no other functionality at that moment. The current 'service' approach circumvents that, because the class needs to extend Twig_Extension (If i am not mistaken!)

A developer easily could create a bunch of procedural php functions, wrap it in a Twig_Simplefunction without the actual function being a part of the Twig_Extension class. (For example, this is the way I handle hook_twig_filters in TFD7) but that would mean that the filters and functions should be defined in a .module file or as statics in a class. That would work for most functions and filters, but it's fugly and it kind of beats the purpose of having twig filters and functions as annotated part of a any class in the first place.

Cottser’s picture

Could we potentially remove the need for TwigPluginBase/TwigFilterPluginBase by creating those callables in TwigExtenderService instead? This is using twig_extender as the example. To me that would seem like a reasonable context for doing that. Potentially it's a bit less flexible that way though, not sure the best way.

If we're talking about twig_extender, overall the code looks promising to me. The main things I would change on a high level would be:

  1. Separate annotations for extension types as we discussed rather than a 'type' parameter.
  2. Like I mentioned in #5 have at least one interface, potentially two or three for the different extension types. TwigFilterBase (if it exists) implements TwigFilterInterface which has a filter() method - or some better name, potentially a more generic one. This way we don't have the "random public methods" issue.
  3. Make TwigExtenderService a regular tagged Twig extension rather than replacing the core twig.extension service via TwigExtenderServiceProvider (that was a bit scary to find).
  4. Following from that, make TwigExtenderService extend the vendor \Twig_Extension rather than Drupal\Core\Template\TwigExtension (there are very few practical cases for extending the core TwigExtension IMO). getFunctions and getFilters would no longer need to call their parents and merge and such, just return the array.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.