Twig Functions needed in templates

I don't know it's the best way to do this, but it's a start.

@Joelpittet, sorry for the late reaction.

I prefer the 'path_to()' + 'path_to_theme()' combo. I think path_to('theme','foo'), path_to('module','bar') is very readable.

What do you think of this list of methods:

• path_to() => returns path to theme/module/...
• path_to_theme() => returns path to current theme
• current_theme() => returns name of current theme
• file_url() =>returns web-accessible URL

Am I still missing something?

Maybe we can rename path_to_theme() to path_to_active_theme() ?

I prefer active_theme_name() because we return the theme name and not an object or something.

About the path_to('@block/css/block.admin.css') do you mean not to use drupal_get_path() and always use a string as '@block/css/block.admin.css' as parameter? When we use both i think it's kinda strange that we expect a type as parameter 1 and then give a path to a file, no?

// Original Gangster
    base_path() => base_path()
// Renamed:
    file_url() =>file_create_url()
    path_to() => drupal_get_path()
    path_to_active_theme() => path_to_theme()
// New:
    active_theme_name() => \Drupal::service('theme.negotiator')->getActiveTheme()

The path_to function is maybe related to : https://drupal.org/node/1308152

I am making this critical.

Not being able to output a link to an image and twig being completely out of sync with the rest of core (Url::fromUri) is a problem at this stage of beta and definitely release blocking.

And its a bug.

Okay, let's make this a meta. Adding a few related issues and I will add child issues.

The core maintainers got together today to triage the critical queue. While this seems like a significant DX problem for themers, it nevertheless only meets the threshold of a major issue (ref: https://www.drupal.org/node/45111):

"Issues that have significant repercussions but do not render the whole system unusable (or have a known workaround) are marked major."

There are several workarounds to this problem, including calling the ugly \Drupal::fooBar() wrappers, setting a variable up in a theme preprocess function, and creating your own module to accompany your theme which adds such helper functions.

Additionally, these are the types of improvements (API additions only) that could easily go into a minor feature release if they don't get done prior to 8.0.0.

So downgrading back to major. Still would like to see this happen, though.

Thats fine, the other issue for the finalization of the url generation API is still open and critical and strongly related to this one anyway ...

file_create_url() is already in as file_url.

This workaround worked for me:
$path_to_theme = \Drupal::service('theme.manager')->getActiveTheme()->getPath();

There's also {{ directory }}. Exists since... 2007 :)

Wow good point @Berdir I missed that one. It's kinda poorly named IMO but it works!

Not sure if this is the right place to comment, but I just found that {{ base_dir }} works in page.html.twig but not html.html.twig - any reason for that?

@jpamental Open an issue and reference it here.

Issue created for the {{ base_dir }} question here:
https://www.drupal.org/node/2611246

Curious what people think about a system object that includes various is* and get* functions that can be passed to Twig templates.

Eg: {{ system->getBaseDir() }} or {% if (system->isFrontPage()) %}.

That would avoid stuffing more vars into the global namespace as @joelpittet mentioned in #5. It would also provide a one-stop spot to add new variables to the list.

I feel like I'm still getting familiar with the whole theming model, so not sure if I can comment on @mikeker 's question above, but I wanted to let people know I've created a patch to address the {{ base_dir }} issue here: https://www.drupal.org/node/2611246#comment-10543916

Following up on @mikeker 's comment here and on the issue in #31, I do think it's important to have a few key variables available in all theme templates (like active_theme_path()). I'd think that it should include at least:

base_dir
active_theme_path
is_front
logged_in
is_admin

All of those could be useful to have in any theme template, but admittedly I haven't tested to see which ones are or are not (beyond base_dir)

I'm happy to work on a more generalized approach, but will need a little technical help along the way I suspect.

@joelpittet - thanks!

I did mean base_path, and wanted to verify that 'directory' and 'active_theme_path' are indeed the same. I'll run through a couple of tests and see if it's worth wrapping it all up. I just wanted to make sure they're all available whenever they're needed in a consistent manner. If all we needed was to add one variable to one template I wouldn't bother, but I think it's important to make a set that is available in all template files so we can reference them in a consistent manner.

See also _template_preprocess_default_variables().

After talking to @Cottser and @joelpittet on IRC, it doesn't look like there's any chance of this making it into core for 8.0.0. As such I've started a sandbox project at: https://www.drupal.org/sandbox/mikeker/2612132. The idea is to introduce system vars there. They'll be setup as Twig global variables, meaning they'll be available to all templates.

Currently it only has a the vars mentioned #32. Would love feedback (in that sandbox's issue queue) about other vars that need to be added or if this approach is completely wrong-headed!

(Edit: corrected the URL of the sandbox project).

Ack, cross-posted with #35. Looking into that to see if the twig_globals module is taking the wrong approach! Perhaps this could be handled with a simple preprocess function instead?

@mikeker - I tweaked the patch a bit to work more universally. @joelpittet - I'm not sure that themes really need to be able to alter these variables; I think they only need to be read. I also did verify that 'directory' is indeed the same as active_theme_path(), so it's consistent with base_path in this implementation.

New patch is posted here against 8.0.x - I'll re-roll against 8.1.x if this seems like the right approach. (I'll have to add a few more comments in other template files too)

https://www.drupal.org/node/2611246#comment-10548894

Can we please use a global lightweight object?

We really want to be able to add cacheability meta data when any of that functions are used like:

system.isFrontPage() => cache per url

etc.

@Fabianx - just so I understand are you thinking we need to change the strategy within theme.inc as a whole? The solution above leverages the existing 'variables' array that gets created in _template_preprocess_default_variables(). Only asking because that seems like it would be a much larger change. I honestly don't know enough about the advantages or disadvantages to comment on one versus the other.

I should add that I had posted the wrong patch, and the one I just uploaded is what I meant to share, which essentially only moves one line (the variable assignment) from the 'page' preprocess to the template preprocess function, and then adds code comments. That completely solves the issue I had in as non-invasive a way as possible. That's not to say it addresses your concern, but it does make it something that can be of benefit to all themers right away.

I think indeed for 8.1 we should deprecate all global variables and replace them with one lightweight global 'drupal' or 'system' object, which provides all the information.

i.e. {% if drupal.isFrontPage %}

and that object providing methods to achieve that.

The advantages are that:

- We don't need to pre-calculate things we possibly don't need (less effort)
- We can add contextual cacheability information as the information is added where its used, not where it is defined, which is a difference.

+1 for #43. I especially like the delayed rendering of what could be expensive objects.

@Fabianx, just to clarify, the lightweight global object you're thinking of would be using render arrays (or, rather, building render arrays, rendering them, and returning a string) to leverage caching there?

#44: Yes, likely. Implementation wise it should not matter much if it returns render arrays, bools (for getFoo/isFoo), or strings.

To expand a bit on @joelpittet 's comment - the variables currently present in the 'variables' array in theme.inc are:

'attributes' => array(),
'title_attributes' => array(),
'content_attributes' => array(),
'title_prefix' => array(),
'title_suffix' => array(),
'db_is_active' => !defined('MAINTENANCE_MODE'),
'is_admin' => FALSE,
'logged_in' => FALSE,
(none of which need a DB connection)

and

$variables['directory'] = \Drupal::theme()->getActiveTheme()->getPath();
(which calls a function)

I've (in my patch) added 'base_path', but that already exists in that context, so doesn't seem to incur any other overhead.

While I don't really know enough to argue one way or another about the merits of what @Fabianx & @mikeker are proposing, I do know that having the base_path variable is VERY useful in theme templates (especially where you may be referencing assets in the outer 'html.html.twig' template).

The patch is done, it's very simple, and NOT having it present for that outer theme template will present a lot of challenges for themers. It would be really great to get this in, so we can give the larger discussion more time for evaluation.

#46: That is exactly what that is about - to prevent people from shooting themselves in the foot.

Example:

Whenever we expose an is_front_page variable, we don't need to necessarily vary this template per page - cache wise.

Only if in a template you do:

{% if is_front_page %}
foo
{% else %}
bar
{% endif %}

we need to add the 'url' or 'route' cache context. (and currently twig not even can easily add cacheability data, so we would need to use preprocess for that ...)

However within an object, this could be automatically done by just rendering the data on the render stack where it belongs.

This is the big advantage.

Of course we could have an object per variable, but I just though having on drupal object / service with lots of functionality built-in that is commonly needed would make more sense.

This does not affect the theme_path() function at all, just more 8.1.x discussion.

@Fabianx - I completely understand. I do think it's worth thinking through (and thank you for the explanation; I'm understanding it better)

My question is: can we look at the patch I've supplied as a way to address a significant missing piece right now, and then look at this new solution for 8.1.x? It's a one-line change to theme.inc that will help a lot of themers out.

Here's the link again: https://www.drupal.org/node/2611246

Adding related issue given the direction this is starting to take.