Feature request: add base_source_class option to Environment

[markhalliwell]

Follow-up from #3082 (comment):

I still don't think that this [metadata] belongs to Twig and there are ways to implement that outside of core.

I managed to get a working POC, but it unfortunately relied heavily on a decorator pattern to extend the Source class.

It was based on older Twig code, prior to it having the final syntax on the Source class (I was recently made aware that this change).

So it's really not that easy to implement this "outside of [Twig] core"; especially when there's a clear habit of making all classes final and locking down every bit of code for "reasons".

The entire point of that issue was to avoid this unnecessary boilerplate/wrapper code and performance issues that would be inherently caused by such code.

There are legitimate use-cases for needing associated metadata (see #3082) and it is a problem that hasn't been easily solvable due to the massively restrictive nature of the code; despite the quick dismissal that it somehow is.

I get that the thinking is that Twig is "highly extensible" via Filters, Functions, etc. and sure... at that level, it is.

However, this issue is extremely low level; way before it even reaches the rendering level.

It's my hope that this issue would actually be taken seriously by this project.

---

On actually attempting to solve this issue:

I'm still not convinced a loader is the best approach either. The metadata really should be associated with the source it was taken from; not a loader (which would have to keep a map of names/Source objects and metadata... again, costly and horrible DX).

Could there be another option introduced that allows an Environment to set the Source class that should be used (similar to base_template_class)?

'base_source_class' => Source::class,

This would allow final to be kept on Source directly but also allow custom Source objects to be used instead as well.

I know almost all interfaces were removed in 2.0, however, this would mean a SourceInterface would actually need to be created and used for type hints to prevent any BC break.

This would allow for a very clean and easy way for metadata to be captured and parsed (outside of Twig core) without having to use a custom loader and all the boilerplate code that comes with that approach.

From #2549 (comment):

Making this extendable will not be done without a clear use case justifying the extension point (any new extension point has a maintenance cost, and inheritance-based extension points are the most costly in regard to backward compatibility)

The clear use cases for needing metadata associated with a Source object (there are multiple, see #3082). The justification is that it's impossible to do otherwise.

[stof]

This would be an extension usable by only one tool (as each time would replace the source defined by others). As such, I don't think that custom source objects are a solution to associating metadata to a template.

Btw, if you don't have a custom loader, you would not be able to load the associated metadata either. So I don't see how you would avoid that part anyway.

Btw, your patch could be simplified a lot regarding the handling of fixing the line, if your custom loader injects a {% line %} tag at the beginning of the source to adjust the line number based on the size of the frontmatter instead of trying to adjust the line number in errors after they are thrown (and this makes me realize that this tag is not documented anywhere).

[markhalliwell]

First, I would like to say that it is not my intention to attack or berate others. I simply have a very direct way of thinking and speaking. Please do not take what I say below personally, it isn't meant as such.

---

This would be an extension usable by only one tool

No, #2549 proves that isn't true. Also, I'm not just some random person. I'm coming from Drupal. A platform that is used by millions of sites and that adopted this project as its primary templating engine. We have a legitimate need for a solution here. Please, I'm begging you, please be part of the solution here instead of shooting this topic down left and right.

As such, I don't think that custom source objects are a solution to associating metadata to a template.

Then please, pray tell, what is supposed to be the solution here??? I'm trying to collaborate and have an actual dialog around potential solutions to this issue.

However, it feels like anytime I interact with this project, I'm immediately dismissed and told "no". The claim is that it is in the interest of keeping "maintainer cost" at a minimum.

There is a very big difference between adding "unnecessary features" and simply loosening up a very restrictive piece of software so others can actually USE it. There is no real reason to place final on the Source class (or really many of these classes).

If that's so important to this project, then this project needs to provide an alternative way to replace such a restrictive and vital class for consumers that need the flexibility of actually extending it.

Why? Because that is essentially what we were told to do, by this project, because it didn't want to add more features to this project (i.e. can live outside Twig core).... except that it can't, given the current extremely restrictive nature of the project.

Btw, if you don't have a custom loader, you would not be able to load the associated metadata either.

Not true. The metadata is extracted in the custom Source object and then stored in the custom Source object. This is why this option is needed so that anytime a "Source" object needed to be created, it should create one using that class rather than just the normal Source object that ships with Twig.

We would likely still have a custom loader for Drupal (to allow decorated sources as we're very permissive is extensibility), but that isn't the point. The point is that by introducing this tiny little feature, it wouldn't actually require a custom loader for those who need to extend just the source object to attach additional information onto it.

if your custom loader injects a {% line %} tag at the beginning of the source

If you're referring to

Twig/src/Lexer.php

Line 135 in 38341bb

'lex_block_line' => '{\s*line\s+(\d+)\s*'.preg_quote($this->options['tag_block'][1], '#').'}As',

, then no... this isn't a viable solution.

I will state this again: the metadata needs to be extracted before any parsing/rendering happens. The only reason I was suggesting using a custom {@ tag in the other issues is that it would be a "special", separate parser that could be ran independently and would stop after the first block was found. It essentially would have been doing the same thing as a custom Source template we'd be using in Drupal, but in a more integrated and supported manner that was officially supported by Twig. That was shot down, so we're having to move in a different direction.

The primary reason this is needed prior to parsing/rendering is mainly for performance reasons. It's also because its data is needed (at a very low level) to construct registries of plugins and templates, that (at the time it's being executed) would have absolutely ZERO context of what is actually required to render the template.

Thus, this metadata is extracted prior to it even reaching the majority of Twig, which means that {% line %} would always be 1 because it'd be at the beginning of the source content (after the metadata at the beginning was removed).

[stof]

This would be an extension usable by only one tool

No, #2549 proves that isn't true. Also, I'm not just some random person. I'm coming from Drupal. A platform that is used by millions of sites and that adopted this project as its primary templating engine. We have a legitimate need for a solution here. Please, I'm begging you, please be part of the solution here instead of shooting this topic down left and right.

I mean that if you have 2 tools wanting to use them, they conflict with each other, as you cannot have 2 different source classes at the same time.

If you're referring to

Twig/src/Lexer.php

Line 135 in 38341bb

'lex_block_line' => '{\s*line\s+(\d+)\s*'.preg_quote($this->options['tag_block'][1], '#').'}As',

then no... this isn't a viable solution.

I'm not talking about replacing the extraction of the frontmatter with this {% line %} tag. I'm talking about replacing your lineOffset magic with this {% line %} tag (replacing the frontmatter by a line tag with the line offset instead of just removing the frontmatter), so that everything in Twig starts counting lines from higher than 0 to count the lines of the removed frontmatter. This way, all errors already report the original line in the source code, without needing any magic to try to update the reported template line.

Not true. The metadata is extracted in the custom Source object and then stored in the custom Source object. This is why this option is needed so that anytime a "Source" object needed to be created, it should create one using that class rather than just the normal Source object that ships with Twig.
The point is that by introducing this tiny little feature, it wouldn't actually require a custom loader for those who need to extend just the source object to attach additional information onto it.

But then, that means that any loader in the whole Twig ecosystem needs to make the source class configurable to use your own version (and Twig_Environment cannot pass options to loaders, that would break the layer of dependencies, as the loader is the dependency of the environment, not the opposite). The loader is the one instantiating the source object initially.

And that's not the only place using the source object. Compiled templates also instantiate a source object at runtime. Extracting metadata in the source object during instantiation would mean a runtime performance impact on the rendering of templates (and potentially many times per templates, as compiled templates don't try to avoid instantiating the value object many times if needed, as it is meant to be very cheap).

Thus, putting the extraction of the frontmatter inside the value object itself forbids any use case where the extraction logic would benefit from dependency injection (to use other logic, or to make the logic configurable).

There is a very big difference between adding "unnecessary features" and simply loosening up a very restrictive piece of software so others can actually USE it. There is no real reason to place final on the Source class (or really many of these classes).

The reason for that is to make it possible to make Twig evolve in a BC way. Inheritance is the worse extension point ever, from the point of view of maintainers. Almost any change to the codebase becomes a BC break. That's the same reason why private visibility is used since a long time.
The Symfony BC policy would never exist if we made everything protected for instance (compare what is allowed for contributors for protected vs private in https://symfony.com/doc/current/contributing/code/bc.html#changing-classes). And final classes are making things easier here (look at the section about public methods; any no with the note [7] on them become a yes when final)

I will state this again: the metadata needs to be extracted before any parsing/rendering happens. The only reason I was suggesting using a custom {@ tag in the other issues is that it would be a "special", separate parser that could be ran independently and would stop after the first block was found. It essentially would have been doing the same thing as a custom Source template we'd be using in Drupal, but in a more integrated and supported manner that was officially supported by Twig. That was shot down, so we're having to move in a different direction.

The primary reason this is needed prior to parsing/rendering is mainly for performance reasons. It's also because its data is needed (at a very low level) to construct registries of plugins and templates, that (at the time it's being executed) would have absolutely ZERO context of what is actually required to render the template.

Thus, this metadata is extracted prior to it even reaching the majority of Twig, which means that {% line %} would always be 1 because it'd be at the beginning of the source content (after the metadata at the beginning was removed).

From your patch, the way to access metadata is still through a Twig_Environment->getSourceMetadata, not from a loaded Template instance. And this will already do a separate loading that the compilation of the template. So I don't see why your loader could not have a separate method returning a metadata object, rather than trying to get this into the Source value object.
This is what has been suggested before, and will work without any change in Twig itself.

[markhalliwell]

I mean that if you have 2 tools wanting to use them, they conflict with each other

Well, there's only one Twig environment in Drupal so.

Also, this is why we'd likely use a decorator pattern; not uncommon.

so that everything in Twig starts counting lines from higher than 0 to count the lines of the removed frontmatter

I get what you're saying:

---
layout: simple
---
{% line %}
<div>

What you're not realizing though is that this is actually what Twig would parse/render:

{% line %}
<div>

The frontmatter is removed before it reaches the parse/render stage; thus, nullifying the usefulness of {% line %} at all. So yes, a tiny little bit of line offset handling is needed. This is the absolute least of my concerns.

any loader in the whole Twig ecosystem needs to make the source class configurable to use your own version

No. They could still use Twig's original Source class. This is an opt-in feature and wouldn't cause any BC break if they chose to ignore this option.

Twig_Environment cannot pass options to loaders, that would break the layer of dependencies, as the loader is the dependency of the environment, not the opposite

I'm not suggesting that a Loader suddenly got an environment dependency. Instead, I was thinking that this option would a method on Loaders (i.e. setSourceClass) that would be called when constructing the Environment using the value of the option.

And that's not the only place using the source object.

A very simple SourceClassTrait could be added to multiple classes so that it could support a different Source object.

Extracting metadata in the source object during instantiation would mean a runtime performance impact on the rendering of templates

Yes, this can be moved to a different method that's called when attempting to actually retrieve the source or metadata rather during __construct.

Thus, putting the extraction of the frontmatter inside the value object itself forbids any use case where the extraction logic would benefit from dependency injection (to use other logic, or to make the logic configurable).

Well, this is because of the way Twig is restricted/constructed. I tried asking for a more performant and less janky way of doing this.

Inheritance is the worse extension point ever, from the point of view of maintainers.

That's certainly one view...

From your patch, the way to access metadata is still through a Twig_Environment->getSourceMetadata, not from a loaded Template instance.

This is only because of a current limitation of Drupal and how antiquated the theme system API currently is. This is also just the first step in implementing this. There a lot of sub-systems that need to be adjusted to account for this, the least of which is theme engine(s) (aka. Twig).

So I don't see why your loader could not have a separate method returning a metadata object, rather than trying to get this into the Source value object.

Because ultimately, the idea is to pass around template objects rather than having to invoke the Twig service (Environment) all the time, for everything, like we're currently doing.

This means that the Environment (and subsequently the Loader) wouldn't be readily accessible and the metadata would need to be associated with the Source object.

Nevermind the semantics of it: the frontmatter literally lives in the actual "source" file. It only makes sense that it would have an additional "metadata" property/method for it, just like its "code" property. You said it's a value object, that's it's value: metadata and code.

---

At the very least, creating a SourceInterface and just changing the typehints throughout Twig to that rather than some final object would be the most beneficial and least disruptive for everyone involved.

This would still allow us to implement most of what we have currently, minus the extension from Source.

[ericmorand]

Just digressing a bit about the line tag: is it officially supported by Twig@2? It's not described in the documentation. Is it something specific to Twig@1 only?

[stof]

it is supported. It is missing from the doc though (this is quite a special tag, as you need it only if you preprocess the source of a template before passing it to Twig).

[ericmorand]

Thanks a lot. I'll have to add it to Twing.

Not sure how this will work with a source-map though. I'll have to think about that.

[stof]

well, this tag is used to force a particular line number. when encountering this tag, the lexer would update the current position (tracked based on newline chars) to the specified line number (and then all token will take that offset into account in their location automatically).
If your compiler generates a sourcemap, it should take it into account (which might even happen automatically depending on where the sourcemap gets hooked into your implementation).

[ericmorand]

Yes, you're right. Actually, it's hooked in the template itself so it should be correct out of the box since it is based on the node line/column - and the nodes get their line/column from the tokens.

[markhalliwell]

At the very least, creating a SourceInterface and just changing the typehints throughout Twig to that rather than some final object would be the most beneficial and least disruptive for everyone involved.

Is this really too much to ask?

[markhalliwell]

Also, FTR, I was extremely confused because @stof kept saying/using {% line %} and it made no sense to me how this would magically retrieve a new line number.

Given how there's no actual documentation, I easily overlooked the fact that the regex actually requires a number after {% line:

Twig/src/Lexer.php

Line 135 in 38341bb

'lex_block_line' => '{\s*line\s+(\d+)\s*'.preg_quote($this->options['tag_block'][1], '#').'}As',

It should really be referred to as {% line \d+ %} instead of just {% line %}. Some proper documentation about this also really needs to be added so that it can give an example of how to use it as well.

[ericmorand]

I'll provide a PR for the line documentation. It's needed, I totally agree.