[PP-1] Use hook_theme_suggestions in views

We need to be careful to not break plugins and customizations that rely on this and might customize it. I see you still call the themeFunctions() method, but I could imagine that some weird cases like feed output or so might use things differently. I know I did some tricks with custom feed things in the past, e.g. with https://www.drupal.org/project/views_googlenews.

@dawehner, the patch in #2 is essentially the same solution I came up with in my head yesterday when going through Views' code, so I think we're on the right track :D

---

Do you mind pointing me to the place you refer to with "some tricks with custom feeds"?

I'm not sure what specific "tricks" @Berdir is referring to. I did look at the code for this module and nothing seemed to jump out at me; it seems pretty straightforward.

http://cgit.drupalcode.org/views_googlenews/tree/src/Plugin/views/style/...
http://cgit.drupalcode.org/views_googlenews/tree/src/Plugin/views/row/Go...

As you can see with the above, these invoke $this->themeFunctions() directly and doesn't provide #views_plugin, so this contrib module wouldn't be affected.

I don't think we will have to worry about affecting other contrib modules at all as long as this remains "opt-in" as @dawehner says. Regardless, this will definitely need a change record and heavy promotion so Views plugin authors know to update their code and to stop using $this->themeFunctions() directly.

In fact, I think the existing method should probably be deprecated and a new one created named themeSuggestions, move existing code to this new method and then invoke this new method from the old one. This would allow IDEs (like PhpStorm) to indicate to developers that they shouldn't use this method directly anymore. A link to the TBD change record could be added even.

---

I'm sure the above patch was just focusing on seeing if this would work in the first place, but let's not forget about ViewExecutable::buildThemeFunctions too.

Thanks,
patch from #8 is showing all my views theme suggestions now in twig debug

Patch #8 worked beautifully.

Path #8 was just the intital POC and it seems stable enough.

There's still work to be done though as I mentioned in #7.

+++ b/core/modules/aggregator/src/Plugin/views/row/Rss.php
@@ -68,7 +68,8 @@ public function render($row) {
-      '#theme' => $this->themeFunctions(),
+      '#theme' => $this->definition['theme'],

+++ b/core/modules/views/views.module
@@ -226,6 +228,15 @@ function views_theme($existing, $type, $theme, $path) {
+ * Implements hook_theme_suggestions_alter().

Maybe better use specific hook_theme_suggestions_HOOK_alter() for each allowed plugin instead of common one?

I'm not sure if the issue I'm experiencing is related to this patch or if views actually checks templates in the wrong order.
Here is a twig debug suggestion that works and goes from specific at the top, to more generic at the bottom.

<!-- FILE NAME SUGGESTIONS:
   * paragraph--p-view-reference--default.html.twig
   x paragraph--p-view-reference.html.twig
   * paragraph--default.html.twig
   * paragraph.html.twig
-->

As you can see, I have my own custom template that is loading successfully for my paragraph of type 'p-view-reference'.

With the patch in #8 I can see the template suggestions as follows.

<!-- FILE NAME SUGGESTIONS:
   x views-view.html.twig
   * views-view--services.html.twig
   * views-view--block.html.twig
   * views-view--services--block.html.twig
   * views-view--homepage.html.twig
   * views-view--services--homepage.html.twig
   x views-view.html.twig
-->

You can see that the order of listed suggestions is from most generic to most specific, exactly the opposite of the others. Also, my more specific template views-view--services--homepage.html.twig refuses to be used and it always chooses views-view.html.twig.

NOTE: The paragraph override still works and displays correctly with this patch. It's only the view suggestions that are incorrect and don't work with #8

@RedEight #2752443: Incorrect order and duplicate theme hook suggestions

+++ b/core/modules/views/views.module
@@ -226,6 +228,15 @@ function views_theme($existing, $type, $theme, $path) {
+    $suggestions = array_unique(array_merge($suggestions, $variables['views_plugin']->themeFunctions()));

Suggestions are expected to be returned with ASC specificity, ie from system_theme_suggestions_field():

$suggestions[] = 'field__' . $element['#field_type'];
  $suggestions[] = 'field__' . $element['#field_name'];
  $suggestions[] = 'field__' . $element['#entity_type'] . '__' . $element['#bundle'];
  $suggestions[] = 'field__' . $element['#entity_type'] . '__' . $element['#field_name'];
  $suggestions[] = 'field__' . $element['#entity_type'] . '__' . $element['#field_name'] . '__' . $element['#bundle'];

We may need to either array_reverse() here or on the themeFunctions().

I understand returning the suggestions from views_theme_suggestions_alter() is much easier and quicker then implementing all the singles instances of hook_theme_suggestions_HOOK(), so IMHO +1 for that.

Applying both patches doesn't solve it. Also see gambry's comment there.

I've got a related_content view with a block display and a custom views-view--related-content--block.html.twig template.
This are my output with all scenarios:

With #2923634: [PP-1] Use hook_theme_suggestions in views:

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'views_view' -->
<!-- FILE NAME SUGGESTIONS:
   x views-view.html.twig
   * views-view--related-content.html.twig
   * views-view--block.html.twig
   * views-view--related-content--block.html.twig
   * views-view--block-1.html.twig
   * views-view--related-content--block-1.html.twig
   x views-view.html.twig
-->
<!-- BEGIN OUTPUT from 'themes/custom/mytheme/templates/views/views-view.html.twig' -->

With #2923634: [PP-1] Use hook_theme_suggestions in views with array_reverse():

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'views_view' -->
<!-- FILE NAME SUGGESTIONS:
   * views-view--related-content--block-1.html.twig
   * views-view--block-1.html.twig
   x views-view--related-content--block.html.twig
   * views-view--block.html.twig
   * views-view--related-content.html.twig
   * views-view.html.twig
   * views-view.html.twig
-->
<!-- BEGIN OUTPUT from 'themes/custom/mytheme/templates/views/views-view--related-content--block.html.twig' -->

With #2752443: Incorrect order and duplicate theme hook suggestions + #2923634: [PP-1] Use hook_theme_suggestions in views:

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'views_view' -->
<!-- FILE NAME SUGGESTIONS:
   x views-view.html.twig
   * views-view--related-content.html.twig
   * views-view--block.html.twig
   * views-view--related-content--block.html.twig
   * views-view--block-1.html.twig
   * views-view--related-content--block-1.html.twig
   x views-view.html.twig
-->
<!-- BEGIN OUTPUT from 'themes/custom/mytheme/templates/views/views-view.html.twig' -->

With #2752443: Incorrect order and duplicate theme hook suggestions + #2923634: [PP-1] Use hook_theme_suggestions in views with array_reverse():

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'views_view' -->
<!-- FILE NAME SUGGESTIONS:
   * views-view--related-content--block-1.html.twig
   * views-view--block-1.html.twig
   x views-view--related-content--block.html.twig
   * views-view--block.html.twig
   * views-view--related-content.html.twig
   * views-view.html.twig
   * views-view.html.twig
-->
<!-- BEGIN OUTPUT from 'themes/custom/mytheme/templates/views/views-view--related-content--block.html.twig' -->

With #2752443: Incorrect order and duplicate theme hook suggestions with fix (#40 .4) + #2923634: [PP-1] Use hook_theme_suggestions in views with array_reverse():

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'views_view' -->
<!-- FILE NAME SUGGESTIONS:
   * views-view--related-content--block-1.html.twig
   * views-view--block-1.html.twig
   x views-view--related-content--block.html.twig
   * views-view--block.html.twig
   * views-view--related-content.html.twig
   * views-view.html.twig
-->
<!-- BEGIN OUTPUT from 'themes/custom/mytheme/templates/views/views-view--related-content--block.html.twig' -->

Working on this.

Attached patch to reverse suggestions array output as the ThemeManager would expect and test coverage.

Some outstanding doubts the reviewer should take in consideration are:
- I still think using hook_theme_suggestions_alter() is our best option, otherwise we need to implement hook_theme_suggestions_HOOK() for each single hook, which increase the amount of code without reason. Also developers providing their own plugins will require their own hook_theme_suggestions_HOOK(), while with the _alter() we do the job for them.
- I'm reversing the suggestions array inside views_theme_suggestions_alter(), but wondering if outputting the right order from ViewExecutable::buildThemeFunctions() is better.

Has no one read #7?

Confirming #22 works!

The only minor issue is that one views template suggestion is repeated (the last one):

<!-- FILE NAME SUGGESTIONS:
   x views-view--globaltest--page-1.html.twig
   * views-view--page-1.html.twig
   * views-view--globaltest--page.html.twig
   * views-view--page.html.twig
   * views-view--globaltest.html.twig
   * views-view.html.twig
   * views-view.html.twig
-->

Steps for testing:

1. installed Drupal 8.6-dev
2. enabled twig template debugging in services.yml
3. applied patch #22
4. verified template suggestion list against Drupal docs
5. created template file to verify template is actually used

I will leave status as-is due to the duplication of the final view template suggestion. Once this is resolved we can retest and go to RTBC.

I agree with #24. I tested and got the same result. Lets get a few more community members testing it, but the initial issue has been covered on the #22 patch

+++ b/core/modules/views/views.module
@@ -225,6 +227,15 @@ function views_theme($existing, $type, $theme, $path) {
+    $suggestions = array_reverse(array_unique(array_merge($suggestions, $variables['views_plugin']->themeFunctions())));

This is reversing the entire $suggestions array, not just the view plugin suggestions.

That being said, most of this is premature optimization due to #2752443: Incorrect order and duplicate theme hook suggestions which is an entirely separate issue from this issue and shouldn't be included here.

As far as the "order" of these view plugin theme hook suggestions, I would like to again recommend (#7) that this issue add a dedicated themeSuggestions method (using the correct order) and deprecate the old themeFunctions method.

---

Last, but not least, this still (#7) hasn't addressed ViewExecutable::buildThemeFunctions.

Has no one read #7?

@markcarver my bad. I had read #7 but in my mind probably got mixed up with your comments on #2752443: Incorrect order and duplicate theme hook suggestions. I beg pardon.

Your suggestions make sense to me, I will update my patch adding your suggestions in #7.

This is reversing the entire $suggestions array, not just the view plugin suggestions.

Good catch! will fix.

That being said, most of this is premature optimization due to #2752443: Incorrect order and duplicate theme hook suggestions:

If you are referring to the array_reverse() then is not premature. Views is sending the suggestions in the wrong order. See in #17 how 'field' suggestions order is returned.

If you are referring to #24 - #25 than you are correct and #2752443: Incorrect order and duplicate theme hook suggestions will fix that issue. As the problem is just with the base hook being repeated as last suggestions I don't think #2752443: Incorrect order and duplicate theme hook suggestions is a blocker for this issue.

Last, but not least, this still (#7) hasn't addressed ViewExecutable::buildThemeFunctions.

Not sure what you mean exactly. Are you thinking about a re-factor of the method name similar to themeFunctiona()?

If you are referring to the array_reverse() then is not premature.

It is when we can simply create a new method designed specifcally for theme hook suggestions that returns the correct order in the first place and thus eliminating the need for array_reverse() in the first place :D

Not sure what you mean exactly. Are you thinking about a re-factor of the method name similar to themeFunctiona()?

Correct.

The above patch(es) take care of the plugin theme hook suggestions.

However, ViewExecutable::buildThemeFunctions is for the View entities themselves which still populates #theme with an array of theme hooks. This will also need to be converted to use a new method specifically for theme hook suggestions and then deprecate the existing method.

This whole thing stems from the fact that Views is using an antiquated theming technique to provide "theme hook suggestions" by passing an array of theme hooks to #theme instead of a single string. That includes ViewExecutable::buildThemeFunctions, not just its plugins.

If you are referring to the array_reverse() then is not premature.

It is when we can simply create a new method designed specifcally for theme hook suggestions

Perfect! I thought you meant premature as "the ThemeManager will already revert those". I already mentioned I don't like the reverse to stay in the *_alter_hook.

That includes ViewExecutable::buildThemeFunctions, not just its plugins.

Just to be clear again - better safe than sorry :D - buildThemeFunctions() itself just builds the array, it's how other piece of codes make use of it we need to refactor. Is that what you mean?
My idea was to deprecate PluginBase::themeFunctions() as well as buildThemeFunctions() in favour of new methods, so it looks like we are on the same page again.

Said that I won't be able to work on this until the Winter Open Source Sprint, so if anyone want to tackle #27 and #28 in the meanwhile feel free to do it.

buildThemeFunctions() itself just builds the array

Correct. That is the problem. Views shouldn't be passing an array of theme hooks to anything.

The only thing that should be an array when dealing with theme hooks is suggestions, which now have dedicated alter hooks.

This may help explain things a little better:

ViewExecutable::buildThemeFunctions (view entity) === PluginBase::themeFunctions (view plugins)

So far, this issue has focused primarily on PluginBase::themeFunctions (view plugins).

My idea was to deprecate PluginBase::themeFunctions() as well as buildThemeFunctions() in favour of new methods

Good. I was envisioning something along the lines of:

/**
 * ...
 * @deprecated
 *   Passing an array of theme hooks to #theme has been deprecated.
 *   Use ::themeSuggestions() instead.
 */
public function themeFunctions() {
  return array_reverse($this->themeSuggestions());
}

Using array_reverse() here would be appropriate since the new themeSuggestions() method should return the correct array order and this must provide BC support.

Since its likely to not be used at all (or at the very least very infrequently), it's OK to implement it here as performance isn't the primary goal of BC code and the performance impact of adding in array_reverse() here is rather negligible given that it will be deprecated.

BC is hard. (Can I have a trademark on that sentence? I seem to use it in every second issue)

It needs to go in both ways. If someone customized that method in a plugin, it still needs to work, we can't just break that. And that's actually pretty hard to do. Only idea I had so far is some trickery with moving the BC version of themeFunctions() to a __call() implementation and then in the new method, check if the plugin implements a themeFunctions() method, if yes, call it and trigger a deprecated warning.

Hm. Maybe, the new themeSuggestions method should check if the deprecated themeFunctions method was overridden in a subclass (https://stackoverflow.com/a/16523542). If it was, then the new method would invoke that, reverse the array and trigger a deprecation notice.

If it wasn't then it would return the normal theme hook suggestions.

Works great! Thanks for the patch

Setting back to CNW, see #26-#32, which still hasn't been fully addressed.

1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -292,6 +292,13 @@ public function themeFunctions() {
   +    return array_reverse($this->themeFunctions());
   

   This should have the array of theme suggestions and the themeFunctions method should be calling this method and use array_reverse.

2. +++ b/core/modules/views/src/Plugin/views/ViewsPluginInterface.php
   @@ -178,4 +178,14 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state);
   +  public function themeSuggestions();
   

   Adding something to an existing interface is a backwards incompatible change (at least that's what was told to me before).

---

This also, still, does not not address ViewExecutable::buildThemeFunctions.

...so we have to reverse twice?

No.

The new themeSuggestions() method should not have/use array_reverse at all. It should order the suggestions correctly to begin with.

The only array_reverse that should happen would be in ViewExecutable::buildThemeFunctions and PluginBase::themeFunctions because they should now be invoking the new themeSuggestions() method for both classes (which should already have the correct array order). That is why array_reverse in these existing methods is important: to keep BC and not change the expected behavior.

These existing methods should also be marked @deprecated.

Hm. I think something is getting lost in translation. I'll see if I can't clear it up real quick.

Ok, let's see if this makes more sense.

I also went ahead and fixed some of the tests, so everything _should_ pass now.

One nit is duplicate 'views-view-unformatted.html.twig' suggestion in list:

Yes, that will/should be handled by #2752443: Incorrect order and duplicate theme hook suggestions.

This really should use #context, not arbirary #views_* properties.

Here's a patch that takes into account the above issue. Will set to CNR and test it once that issue makes it in.

Solution described on #50 solved the issue for me. After applying both patches the theme suggestions are properly displayed.

For what it's worth, here's a re-rolled patch that applies to both 8.7.x and 8.6.0.

Making the interdiff was not easy, but basically it was just some coding standard issues on `core/modules/views/tests/src/Functional/ViewsTemplateSuggestionsTest.php` are already in.

Updated IS with link to issue that this issue is waiting on.

I've re-rolled the patch from #55 for 8.7.x.

An undefined index caused a load of errors. I've added a quick return if the index hasn't been set.

#58 isn't needed once #2511548: Add a "context" array variable to all theme hooks and "#context" array property to all elements to provide optional contextual data gets in, which this issue is postponed on.

New patch. This one actually appears to work.

(I think there's going to be one of the tests failing – a difference in ordering of arrays)

@John Cook, please stop reverting the patch to work without context.

This issue is dependant on #2511548: Add a "context" array variable to all theme hooks and "#context" array property to all elements to provide optional contextual data.

Apply the patch from that issue and then use #55.

Patch from #2511548 solved the billion warnings I got about #context not being set.

Heres a semi re-roll and modification of the deprecation version.

Updated CR: https://www.drupal.org/node/2962549

Removed the views_plugin variables that were added to views theme hooks prior to the #context switch.

#22 works for me with charm.

#65 and the related issue patch worked for me.

#65 can not be applied anymore...

I re-rolled patch #65

#22 works for me. Thank you @gambry .

This patch I am using in #22 requires for deprecation that the base theme be set in the theme file. Should I make this against the latest patch 65, or off of 22 which I am using?

#70 gave me the following notice all over the page on D8.9:

Notice: Undefined index: context in views_theme_suggestions_alter() (line 233 of core/modules/views/views.module).

I just added another wrapper to check if context is set fixes it for me.

Last 8.9 patch breaks theme suggestions for views, for example media-library template from classy is not found anymore

#22 it's working like a charm. Thanks so much!

#22 it's working for me as well. Thank you @gambry

I was looking at #3139335: [D9 ONLY] The 'core_version_requirement' constraint requires the 'core' key not be set, but test modules might end up with both and I think comments 6 and 7 refer to a couple of things stale on the views_test_suggestions_theme.info.yml file, here's an updated patch for those on 9.x

Reroll #22 for D9.

I'm using 9.4.8 with PHP 8.0 and have applied the patch in comment #22. Thank you @gambry.

The twig debug info is displayed in the DOM, not in the inspector. Is this the expected output?

I tried the patch in comment #84 with the same results (thank you @phparkle).

If this is the expected results, I'm OK with that; at least I have twig debugging information.

I applied #84 in 9.5.0 and it's working as expected. Suggestions are now displayed. Thank you :)

Just a quick question on this, I couldn't find a more appropriate issue. I wondered why ViewExecutable::buildThemeFunctions prefers (or even involves) the display_id as first argument over the view id.

$themes[] = $hook . '__' . $display['id'];
...
$themes[] = $hook . '__' . $id;

Is this intended? For me this looks a little bit like gambling, to even involve display_id as first argument. I stumbled on this after using a "views-view--something.html.twig" and wondered why the display of a view matched another html.twig, because its display id matched it, which I never expected...

Hi,

So @captain hindsight , do it be necessary to patch to get views template suggestions with display id or just adding code into .theme file ? Something like:
$suggestions[] = 'views_view__' . $variables['view']->id(). '__' . $display['id'];

#83 it's working fine for me. Thanks !

I am currently upgrading a project to Drupal 10 and so I have re-rolled #84 for D10.

The patch (2923634-92.patch) is identical as #84, except for removal of the changes to /core/modules/aggregator/src/Plugin/views/row/Rss.php, due to the removal of the aggregator module from core. I have not created an interdiff as the first patch no longer applies on D10.

I hope this assists if you are currently upgrading to D10. As the issue is postponed I have not created a patch against D11.

@tcrawford, thanks for the D10 patch.
#92 patch cleanly applied on Drupal 10.1.1.

With the patch in #70 against 9.x I got this for example for an exposed views block:

<!-- THEME HOOK: 'form' -->
<!-- FILE NAME SUGGESTIONS:
   * form--audio-series-listing--block-1.html.twig
   * form--block-1.html.twig
   x form--audio-series-listing--block.html.twig
   * form--block.html.twig
   * form--audio-series-listing.html.twig
   * form.html.twig
   * form.html.twig
-->

With the latest patches here against 10.x I only get

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'form' -->
<!-- BEGIN OUTPUT from 'themes/contrib/stable/templates/form/form.html.twig' -->

Going back to #83 against 10.x brings back the theme suggestions (form---VIEW_ID--DISPLAY_ID.html.twig,..) (though it doesn't apply cleanly due to the patch for core/modules/aggregator/src/Plugin/views/row/Rss.php)

Something went missing in #92

Re-rolled patch from #83 to apply cleanly against 10.1.x

I did not dig in to this much, but just noting that during an recent upgrade to 10.1 (from 9.5) this patch was breaking suggestions for views - existing template overrides (e.g. views-view-list--[machine_name]) were not being used. Removing the patch resolved for me.

This doesn't work quite right for Drupal 10.

Example:

1. We have one content type that is displayed in 2 views with the same view mode.
2. The views are placed on different pages.
3. Create templates for the content from these views.
4. Open page 1 and we have the expected result.
5. Open page 2 and the content that has already been displayed on page 1 is displayed with the template for the first view.

@ben.hamelin Have you tested wit the re-rolled patch from #96? I did that re-roll because previous patches were broken after upgrading to 10.1

#92 works for me (Drupal 10.1.5), thanks.

The patch applies with som difficulties against 10.2.2, but I don't get file name suggestions for Views table:

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'views_view_table' -->
<!-- BEGIN OUTPUT from 'core/themes/olivero/templates/views/views-view-table.html.twig' -->
<table class="views-table cols-1">

Applying the patch:

$ patch -p1                < 4947.diff 
patching file core/modules/comment/src/Plugin/views/row/Rss.php
patching file core/modules/node/src/Plugin/views/row/Rss.php
patching file core/modules/search/src/Plugin/views/row/SearchRow.php
patching file core/modules/views/src/Form/ViewsExposedForm.php
Hunk #1 succeeded at 136 (offset -8 lines).   <<<<<< A few of these, as well as "succeeded at 63 with fuzz 1"
[...]