Problem/Motivation

path_to_theme() and $theme_path from D7 have no equivalent in Drupal 8.

Proposed resolution

Add an active_theme_path twig function.

Remaining tasks

User interface changes

API changes

The new twig function, active_theme_path.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is adding a twig function
Issue priority Major because tstoekler marked in comment #10
Disruption Minimal disruption as it is a getter method. Possible disruption if changes upstream (Twig)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cilefen’s picture

Issue summary: View changes
cilefen’s picture

Status: Needs review » Active
cilefen’s picture

Status: Active » Needs review
FileSize
1.1 KB
chr.fritsch’s picture

FileSize
1.18 KB

Status: Needs review » Needs work

The last submitted patch, 4: add_an-2416857-4.patch, failed testing.

star-szr’s picture

Issue tags: +Needs tests, +Novice

Comment from #2416831-6: Add an active_theme twig function applies here too I think.

Patch from #3 still applies, for the record :)

cilefen’s picture

Status: Needs work » Postponed

Postponed on #2416831: Add an active_theme twig function for the service injection.

star-szr’s picture

Status: Postponed » Needs work

But the tests aren't postponed :) someone else can be working on those in the meantime.

cilefen’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.31 KB
tstoeckler’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

Hit this today, as well. And was about to roll the exact same patch. ... And this one comes with tests also. Awesome. Looks good to me.

Also marking major per #2168231: Twig Functions needed in templates.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: add_an-2416857-9.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
Issue tags: +Needs beta evaluation
FileSize
4.38 KB

reroll

Status: Needs review » Needs work

The last submitted patch, 12: add_an-2416857-12.patch, failed testing.

Status: Needs work » Needs review

Cottser queued 12: add_an-2416857-12.patch for re-testing.

star-szr’s picture

Status: Needs review » Postponed

I think this is blocked on #2416831: Add an active_theme twig function for injecting the theme manager.

tstoeckler’s picture

Status: Postponed » Needs work

OK, the parent is in now. Now this just needs to be changed to use the injected theme manager.

cilefen’s picture

Status: Needs work » Needs review
FileSize
2.45 KB
zaurav’s picture

Issue summary: View changes
heddn’s picture

Issue summary: View changes
heddn’s picture

Issue tags: -Needs beta evaluation

Removing tags.

lauriii’s picture

Issue tags: +Needs change record
cilefen’s picture

Issue tags: -Novice

Draft CR. There being no further Novice tasks, I removed that tag.

cilefen’s picture

Issue tags: -Needs change record
Anonymous’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +Needs reroll

This needs another reroll. So tagging novice (instructions).

$ git apply --index patches/add_an-2416857-17.patch
error: patch failed: core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php:99
error: core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php: patch does not apply
evgeny.chernyavskiy’s picture

Assigned: Unassigned » evgeny.chernyavskiy
evgeny.chernyavskiy’s picture

evgeny.chernyavskiy’s picture

Status: Needs work » Needs review

Small conflict probably due to an extra space/new line. Marking as "Needs review".

Anonymous’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice
FileSize
18.01 KB

Ok great! I tested manually using an inline template render array, to check that this actually does what we expect:

We have test coverage and a CR.

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -274,6 +275,17 @@ public function getActiveTheme() {
+

Epic nitpick. This newline is not needed. I guess this can be removed on commit?

evgeny.chernyavskiy’s picture

@pjonckiere,

Epic nitpick. This newline is not needed. I guess this can be removed on commit?

Doh, missed that one. Not sure what you mean by removing on commit though?

Anonymous’s picture

If you can make a new patch with that line removed, that would be great and I'll RTBC again. But I didn't want to reject the patch just because of that line, so I RTBC'd anyway and a branch maintainer could remove that line on commit. The former is preferred though.

evgeny.chernyavskiy’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs Review
FileSize
3.28 KB

Re-rolling my previous patch from https://www.drupal.org/node/2416857#comment-10370239 with amendments per https://www.drupal.org/node/2416857#comment-10370595. Also updated documentation for a couple methods to include the missing @throws declarations.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs Review

Looks good to me! Back to RTBC.

ps: changing status to "needs review" is sufficient, there is no need to tag them

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 31: add-twig-active-theme-path-2416857-2.patch, failed testing.

Anonymous’s picture

Failed to set field driver to mysql

This looks like something unrelated to this patch, so retesting.

Status: Needs work » Needs review
Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 31: add-twig-active-theme-path-2416857-2.patch, failed testing.

cilefen’s picture

Issue tags: +Needs reroll
lauriii’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
3.24 KB
lauriii’s picture

+++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
@@ -190,5 +217,4 @@ public function __construct($string) {
-

This change shouldn't be in the patch. Commiter please remove this change :)

catch’s picture

Does this get resolved into the compiled templates or is it done when the template is rendered?

I think it's the latter, but we really don't want the theme path compiled with the templates - moving directory would break it then.

catch’s picture

Status: Reviewed & tested by the community » Needs review
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

#41: It is determined dynamically on run-time.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: add_an-2416857-39.patch, failed testing.

The last submitted patch, 39: add_an-2416857-39.patch, failed testing.

Fabianx’s picture

Issue tags: +Needs reroll
cilefen’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.27 KB
cilefen’s picture

FileSize
3.11 KB

oops

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed
+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -361,6 +372,8 @@ public function attachLibrary($library) {
+   *
+   * @throws \Exception

@@ -474,6 +487,8 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
+   *
+   * @throws \Exception

These changes are totally out-of-scope. Removed.

Committed 7594f7e and pushed to 8.0.x. Thanks!

  • alexpott committed 7594f7e on 8.0.x
    Issue #2416857 by cilefen, evgeny.chernyavskiy, lauriii, chr.fritsch,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Wim Leers’s picture

thijsv’s picture

So it is smarter to not use this anymore and wait for the stream wrappers solution? I mean, is it possible that this active_theme function will disappear in the future?

bradjones1’s picture

I agree with Wim, here - this function doesn't return a path relative to the base path, and you may not necessarily have the base path in your twig template context. Stream wrappers are a much better solution.

But, since this is committed to core, that ship has (mostly) sailed. What's the process to get this marked as deprecated for 9.0?

cilefen’s picture

Open an issue unless there is already one.