Updated: Comment #0

Problem/Motivation

When grepping to determine the scope of #2049207: [Follow up] Replace .tpl.php with .html.twig in documentation it was determined that the "Theming information" functionality in Views still refers to .tpl.php files and is broken.

Proposed resolution

Remove this functionality, twig_debug either already does or will soon replicate this functionality in HTML comments.

Remaining tasks

None

Steps to Reproduce

  1. Navigate to admin/structure/views/view/content
  2. Go to Advanced > Output: Templates
  3. Click on "Field Content: Node operations bulk form (ID: node_bulk_form):"
    views-theming-information

You should see the contents of the currently used template for rows. Currently this is empty.

User interface changes

The "Theming information" UI will look for .html.twig files instead of .tpl.php files.

Screen shots from #12
Before #9:

After #9:

API changes

n/a

Comments

Cottser’s picture

This is a bit different than I thought, and maybe it would be nice to make this code theme engine agnostic.

Cottser’s picture

Issue summary: View changes

Add STR

Cottser’s picture

Title: .tpl.php should be .html.twig in Drupal\views\Plugin\views\display\DisplayPluginBase » .tpl.php should be .html.twig or theme engine agnostic in Drupal\views\Plugin\views\display\DisplayPluginBase

Retitling.

Cottser’s picture

Issue summary: View changes

Add screenshot

Manuel Garcia’s picture

Status: Active » Needs review
FileSize
2.79 KB
PASSED: [[SimpleTest]]: [MySQL] 57,138 pass(es). View

Hope I didn't miss any =)

Cottser’s picture

Status: Needs review » Needs work

Thanks @Manuel Garcia :) that gets them all.

Looking at this again it looks possible to be theme engine agnostic, there is some existing code in this class to do this which would be nice to try and reuse. It's in a big switch statement but maybe this code could be moved out into a helper method?

      case 'analyze-theme':
        $form['#title'] .= t('Theming information');
        if ($theme = drupal_container()->get('request')->request->get('theme')) {
          $this->theme = $theme;
        }
        elseif (empty($this->theme)) {
          $this->theme = config('system.theme')->get('default');
        }

        if (isset($GLOBALS['theme']) && $GLOBALS['theme'] == $this->theme) {
          $this->theme_registry = theme_get_registry();
          $theme_engine = $GLOBALS['theme_engine'];
        }
        else {
          $themes = list_themes();
          $theme = $themes[$this->theme];

          // Find all our ancestor themes and put them in an array.
          $base_theme = array();
          $ancestor = $this->theme;
          while ($ancestor && isset($themes[$ancestor]->base_theme)) {
            $ancestor = $themes[$ancestor]->base_theme;
            $base_theme[] = $themes[$ancestor];
          }

          // The base themes should be initialized in the right order.
          $base_theme = array_reverse($base_theme);

          // This code is copied directly from _drupal_theme_initialize()
          $theme_engine = NULL;

          // Initialize the theme.
          if (isset($theme->engine)) {
            // Include the engine.
            include_once DRUPAL_ROOT . '/' . $theme->owner;

            $theme_engine = $theme->engine;
            if (function_exists($theme_engine . '_init')) {
              foreach ($base_theme as $base) {
                call_user_func($theme_engine . '_init', $base);
              }
              call_user_func($theme_engine . '_init', $theme);
            }
          }
          else {
            // include non-engine theme files
            foreach ($base_theme as $base) {
              // Include the theme file or the engine.
              if (!empty($base->owner)) {
                include_once DRUPAL_ROOT . '/' . $base->owner;
              }
            }
            // and our theme gets one too.
            if (!empty($theme->owner)) {
              include_once DRUPAL_ROOT . '/' . $theme->owner;
            }
          }
          $this->theme_registry = _theme_load_registry($theme, $base_theme, $theme_engine);
        }

        // If there's a theme engine involved, we also need to know its extension
        // so we can give the proper filename.
        $this->theme_extension = '.html.twig';
        if (isset($theme_engine)) {
          $extension_function = $theme_engine . '_extension';
          if (function_exists($extension_function)) {
            $this->theme_extension = $extension_function();
          }
        }
Cottser’s picture

Status: Needs work » Needs review
FileSize
9.93 KB
FAILED: [[SimpleTest]]: [MySQL] 57,727 pass(es), 7 fail(s), and 0 exception(s). View

Something like this. Interdiff is the same size as the patch, so not uploading it.

Status: Needs review » Needs work

The last submitted patch, 2049209-5.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
7.37 KB
PASSED: [[SimpleTest]]: [MySQL] 59,224 pass(es). View

reroll

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community

@Cottser that makes sense =)
@lokapujya thanks for the re-roll!

I'll be bold and set it to rtbc.

Cottser’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.82 KB
9.92 KB
PASSED: [[SimpleTest]]: [MySQL] 59,181 pass(es). View

Thank you @lokapujya and @Manuel Garcia! The latest patch shrunk a bit, I think we still want to move the code from within the 'analyze-theme' case to the getThemeInformation method.

This is still failing tests locally for me but I don't think we want the repeated code. I just cut the code and pasted it back into getThemeInformation to include some recent changes. Probably something small would explain why this doesn't work but I haven't had a chance to debug. Manual testing might help.

Cottser’s picture

FileSize
899 bytes
10.13 KB
PASSED: [[SimpleTest]]: [MySQL] 59,306 pass(es). View

Something like this perhaps…

Edit: although, this seems like now the code in the helper function could be run up to 4 times within the analyze-theme case.

Cottser’s picture

Okay, well #9 is the patch to review then! :)

Tim Bozeman’s picture

FileSize
91.1 KB
37.52 KB

I followed the "Steps to Reproduce" which originally produced a warning: file_get_contents(...)
After I applied the patch it shows a doc block.

Whats next, code review?

Tim Bozeman’s picture

Issue summary: View changes

Clarify expected behaviour a bit

Tim Bozeman’s picture

Issue summary: View changes

Added screenshots from #12 to issue summary changes.

Tim Bozeman’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

Issue tags: +Needs tests
FileSize
11.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch vdc-2049209-13.patch. Unable to apply patch. See the log in the details link for more information. View

We seem to have all kind of problems with this functionality.

This fixes at least the overview screen for me. I would like to have someone working on this who actually understands the new theme system.

Cottser’s picture

Issue tags: -Novice

I've never used this functionality myself. Another question might be do we need this with twig_debug in core?

dawehner’s picture

Well, this functionality is more about creating and finding views templates. I agree that this is someone which is not done anywhere else in core.

Can you refer that twig debug in core would mean. Would you have an easy way to find out which templates would be possible to override?

Cottser’s picture

Yes, when twig_debug is enabled we display all possible template suggestions and show the current one in use (marked with an x).

https://drupal.org/node/1922666

dawehner’s picture

Views using passing multiple theme callbacks to #theme, so this sadly does not work.

<!-- THEME DEBUG -->
<!-- CALL: theme('views_view') -->
<!-- BEGIN OUTPUT from 'core/modules/views/templates/views-view.html.twig' -->
<div class="view view-frontpage view-id-frontpage view-display-id-page_1 view-dom-id-399b9a59b86d8fc62c77452743c39d6b06da845d349221b1a6b5b8c5ea40fb61">
  
    
      
      <div class="view-empty">
      No front page content has been created yet.<ul class="links"><li class="0 odd first last"><a href="/d8/node/add">Add new content</a></li></ul>
    </div>
  
dawehner’s picture

Issue summary: View changes

fixed screen shots from #12 widths

jibran’s picture

13: vdc-2049209-13.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 13: vdc-2049209-13.patch, failed testing.

tim.plunkett’s picture

Another problem is that #1886448: Rewrite the theme registry into a proper service added \Drupal::service('cache.theme');, which doesn't exist. If this had been fixed, that would have been caught.

jibran’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
8.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,710 pass(es). View

Here is a reroll. Fixed #20. I was unable to access admin/structure/views/nojs/display/frontpage/page_1/analyze-theme because of #20. Analyzing the theme is very important from development point of view so bumping it to major.

Status: Needs review » Needs work

The last submitted patch, 21: vdc-2049209-21.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

21: vdc-2049209-21.patch queued for re-testing.

tim.plunkett’s picture

Per #14 and #16, I think we may want to just rip this out completely.

dawehner’s picture

FileSize
11.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,712 pass(es). View

If it will be really common for theme developers to look into the HTML i would be totally fine with it.

damiankloip’s picture

Yeah, we all (me, Daniel, Tim) talked about this a while back and decided it's good to let this go!

tim.plunkett’s picture

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

+1

Cottser’s picture

Title: .tpl.php should be .html.twig or theme engine agnostic in Drupal\views\Plugin\views\display\DisplayPluginBase » Remove 'Theming information' feature from Views in favor of twig_debug
Category: Bug report » Task
Issue summary: View changes
Related issues: +#2118743: Twig debug output does not display all suggestions when an array of suggestions is passed to #theme, +#2201781: Pass all theme hook suggestions to theme preprocess functions to allow for suggestion-specific overrides

Retitling, tweaking the issue summary, and adding a couple related issues.

#2118743: Twig debug output does not display all suggestions when an array of suggestions is passed to #theme was opened to make sure an array of theme hooks passed to _theme() is displayed in twig_debug output, but that looks like it will be solved by #2201781: Pass all theme hook suggestions to theme preprocess functions to allow for suggestion-specific overrides though.

Cottser’s picture

I'll post a rough initial patch at #2201781: Pass all theme hook suggestions to theme preprocess functions to allow for suggestion-specific overrides in a few minutes but here's a before/after example:

Before patch

<!-- THEME DEBUG -->
<!-- CALL: _theme('views_view') -->
<!-- BEGIN OUTPUT from 'core/modules/views/templates/views-view.html.twig' -->

After patch

<!-- THEME DEBUG -->
<!-- CALL: _theme('views_view') -->
<!-- FILE NAME SUGGESTIONS:
   * views-view--content--page-1.html.twig
   * views-view--page-1.html.twig
   * views-view--default.html.twig
   * views-view--content--page.html.twig
   * views-view--page.html.twig
   * views-view--content.html.twig
   x views-view.html.twig
   x views-view.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/views/templates/views-view.html.twig' -->

Obviously we don't want views-view.html.twig there twice, but you get the idea :)

jibran’s picture

Priority: Major » Normal
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record, +Quick fix

We need a change record here. It is a simple code removal so not major more.

tim.plunkett’s picture

Priority: Normal » Major
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

It doesn't need a change notice, we're just removing a bit of functionality that was added in D8.
And it is major, since it's a bug right now, we just happen to be fixing it by removing it.

catch’s picture

Status: Reviewed & tested by the community » Fixed

The debug output looks fine, no point providing this twice.

Committed/pushed to 8.x, thanks!

damiankloip’s picture

This really IS good news!

joelpittet’s picture

Status: Fixed » Reviewed & tested by the community

It has potential to be good news... though I know a number of themers/site builders that rely on that template list including me. twig_debug has the potential to be a more global solution for all templates. We will likely need a way to surface this info and if I remember correctly there may have started a devel_themer style UI to parse twig_debug comments.

So I'm hoping is the controlled burning to spur growth type of removal;)

joelpittet’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Quick fix

Whoops, crosspost.

Status: Fixed » Closed (fixed)

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