Problem/Motivation

In D7 we had access to functions and variables to get the path of the current theme, a module, profiles to help link assets within relative to them.

Proposed resolution

Add these functions into the twig environment functions.

Remaining tasks

  1. Decide which variables/functions need to be in there for BC and features.
  2. Add them in
  3. Write tests to make sure they are available and doing as they should.

User interface changes

N/A

API changes

N/A... adding functions that existed previously for BC.

Follow-up from #1757550: [Meta] Convert core theme functions to Twig templates.

Files: 
CommentFileSizeAuthor
#1 theme_path_functions_twig-2168231-1.patch997 bytesJeroenT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,712 pass(es). View

Comments

JeroenT’s picture

Status: Active » Needs review
FileSize
997 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,712 pass(es). View

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

joelpittet’s picture

@JeroenT thanks for the patch, we likely don't need to make the methods but that's more or less how you do it. But before we just add them in I'd like to discuss this. Maybe if you are around for the twig hangout later today?

Also what do you think of naming 'drupal_get_path' function to just 'get_path()'? or 'path_to()' + 'path_to_theme()' combo?

We also may need to get file_create_url() in there because url() may get changed to routes via Drupal::url() and it doesn't work well with assets(think multilingual prefixing paths /fr/).

JeroenT’s picture

Status: Needs review » Active

@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?

joelpittet’s picture

There is a feature in twig that allows for include/extend template look ups to be based on the module location, I wonder if we couldn't use that as well?

This is kinda also related to #2073811: Add a url generator twig extension.

Going along what you put in #3 @JeroenT how about this?

// Original Gangsters
    path_to_theme() => path_to_theme()
    base_path() => base_path()

// Renamed:
    file_url() =>file_create_url()
    path_to() => drupal_get_path()

// New:
    current_theme() => \Drupal::service('theme.negotiator')->getActiveTheme()
joelpittet’s picture

path_to_theme() throws me off because you'd expect it to take arguments...

current_theme() should maybe be active_theme() or active_theme_name()? I like the idea of keeping it as a function instead of a variable because it doesn't clobber the variable namespace.

joelpittet’s picture

Regarding #4 an example of what I mean is you can do {% include @module_name/page.html.twig %}
So we could maybe make path_to() do that too?

path_to('@block/css/block.admin.css') for example?

@namespace_name http://twig.sensiolabs.org/doc/api.html#built-in-loaders

JeroenT’s picture

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()
JeroenT’s picture

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

Cottser’s picture

Fabianx’s picture

Priority: Major » Critical

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.

Fabianx’s picture

Category: Task » Bug report

And its a bug.

Cottser’s picture

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

joelpittet’s picture

Issue summary: View changes
Cottser’s picture

webchick’s picture

Priority: Critical » Major

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.

Fabianx’s picture

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 ...

cilefen’s picture

Issue summary: View changes

file_create_url() is already in as file_url.

cilefen’s picture

Issue summary: View changes
cilefen’s picture

Issue summary: View changes
cilefen’s picture

Issue summary: View changes
catch’s picture

Title: [meta] Functions/variables needed in Twig templates » Functions/variables needed in Twig templates
Category: Bug report » Plan
joelpittet’s picture

Title: Functions/variables needed in Twig templates » Twig Functions needed in templates
Version: 8.0.x-dev » 8.1.x-dev

We've hit RC, we can move these into contrib if they are really needed but I've not seen any requests for these yet.

Moving to 8.1.x.

pixelwhip’s picture

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

joelpittet’s picture

Apparently I gave bad advice. We have that one in core as @Cottser pointed me to the child of this meta.
https://www.drupal.org/node/2501967
{{ active_theme_path() }}

Berdir’s picture

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

Cottser’s picture

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

jpamental’s picture

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?

cilefen’s picture

@jpamental Open an issue and reference it here.

jpamental’s picture

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

mikeker’s picture

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.

jpamental’s picture

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

jpamental’s picture

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’s picture

@jpamental

You can add those missing ones via:
hook_template_preprocess_default_variables_alter

user, logged_in, is_admin are provided by the user module with this hook.
@see user_template_preprocess_default_variables_alter

I think directory is your active_theme_path

base_dir was never a thing in D7 nor a global in D8. I think maybe you mean base_path? Which is only provided to template_preprocess_page() but could be done with that hook.

jpamental’s picture

@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.

Cottser’s picture

joelpittet’s picture

I may add them to Basic theme if they become useful enough. At least base_path

mikeker’s picture

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).

mikeker’s picture

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?

joelpittet’s picture

Dang noticed only modules can use that hook. Making a follow-up. #2612196: Themes should be able to alter global template variables

jpamental’s picture

@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

Fabianx’s picture

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.

jpamental’s picture

@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.

Fabianx’s picture

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.

mikeker’s picture

+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?

Fabianx’s picture

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

joelpittet’s picture

@Fabianx re #43 I think that may over complicate things where it's not needed.

Regarding the advantages:

  1. We can use an anonymous function if we need that but in lots of cases we have the work done and just need to expose it to the template, can't we?
  2. We really need to be able to add cachability information without objects (for simple variable data) because people will try to do this outside of core and it would be nice to not shoot ourselves in the foot so easily.
jpamental’s picture

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.

Fabianx’s picture

#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.

jpamental’s picture

@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

joelpittet’s picture

@Fabianx There will be variables that are not objects sent to templates and will very likely still be going forwards. Requiring wrapping them up for metadata doesn't seem like the best choice. It really is meta data, and that meta data really needs to work with simple variables too IMO. Forcing objects for everything feels overkill. Maybe $variables['#cache_contexts'][] = 'route' (Still fuzzy on how that black magic works, not a real example) would suffice adding the meta data necessary.

joelpittet’s picture

catch’s picture

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

joelpittet’s picture

Status: Active » Closed (duplicate)

Actually consolidating this over at #1696760: TwigExtension: Filters and Functions since we have some of this in core already.

joelpittet’s picture

The variables can go into a different issue. #2612196: Themes should be able to alter global template variables