Views try to find Custom StylePlugin template in core/modules/views/templates

But then what if a custom style plugin wanted to just use one of views' theme functions? Wouldn't that change in logic break that? I haven't checked this properly btw :) This is just from looking at that diff and what I remember of views_theme...

The views_theme() logic coming from the annotations works for our current use cases, but might not work for contrib, as seen above. Either way, we need tests to try putting theme functions and template files in various subdirectories.

@tim.plunkett, do you have a hint for what tests to write?
I'll give it a try.

I wrote a port for a views style plugin, where that bug made it totally complicated to implement the template: #1932830: D8 Port

There's my first approach for a test that checks for a module defined template file. The test will fail, as there isn' already a fix for that issue ;)

The generall problem, that we have to deal with a lot of confusion in the hook_theme() implementation, when implementing themes for other modules. Additionally by default template files should be located in the templates subdirectory as stated in [#1805952].

This seems to be nearly ready already, do they really break, let's see what the testbot tells us about that.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsTemplateTest.phpundefined
@@ -0,0 +1,50 @@
+class ViewsTemplateTest extends ViewTestBase {

Just wondering: Have you tried to use ViewUnitTestBase? These ones are way faster to execute, and you don't use actual page requests but just API calls, so this seems to be possible.

+++ b/core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/display/DisplayTemplateTest.phpundefined
@@ -0,0 +1,29 @@
+ * Definition of \Drupal\views_test_data\Plugin\views\display\DisplayTemplateTest.

... should be Contains \ now, sorry

So, the test fails as expected.
@dawehner, you may be right with ViewUnitTestBase, I haven't test it yet. I will do after we could clarify some of the points below ;)

I think we should also cover the test cases,

• when a plugin wants to use an existing theme (of another module)
• two plugins declare the same theme

For the first case, that module should have to set register_theme = FALSE and theme to the desired template - or is that wrong?
For the second case, there is no handling at all for the moment?

I guess the theme_path variable is something we cannot use really anymore with the Annotations (or is there something like drupal_get_path() for that), to point to a different module's directory.

Additionally I think we should better document that views theme register part. I could not find any documentation on that. Where is a good practice to document that Annotation-Parameters?

jibran, that's the problem. In D8 template files should by default be located in /templates, @see Template files are now located in a templates subdirectory.

The problem lies in the fact, that via annotation we should be able to implement a "theme file", e.g. a "mymodule.theme.inc" that in most cases would be located in the module's root, but on the other hand we are expected to provide a "directory" via the "theme path" annotation.

We have to make sure this default behaviour is implemented. Additionally we still have the problem with modules wanting to reuse another module's theme definitions.

My suggestion would be to implement the following

• If a plugin wants to reuse an existing theme (e.g. some defined by another module), register_theme has to be set to FALSE.
• module has to be set in every plugin (other than views plugins). It is used to locate the module's root directory.
• A template file is always located in the module's templates folder.
• theme_path is NOT used to alter the destination of template file. Therefore I'd suggest to remove it completely and ...
• only use theme_file to locate the theme include file. Therefore it may contain folders, but should always be relative to the module's root.

This should be all fine with the needs of core. Every module that needs a totally different implementation might use hook_theme() or hook_theme_registry_alter() and set register_theme=FALSE.

Imho, the views theme auto-generation should be held very simple and should not clutter up with own functionality, that could be easily implemted by the core theme hooks.

To make the suggested changes more clear, here an example for the annotation of the existing Rss RowPlugin in the node module, that needs to add register_theme = FALSE to be consistent.

id = "node_rss",
title = @Translation("Content"),
help = @Translation("Display the content with standard node view."),
theme = "views_view_row_rss",
register_theme = FALSE,
base = {"node"},
type = "feed",
module = "node"

I'd very much like to hear your opinion on that. And then might to provide a patch to that.

One question though: were is the best practice to document those Annotation params?

Yeah I really think the templates should be located in the templates directory. If someone really wants to put in somewhere else, implementing hook_theme itself/alter it seems to be an acceptable way.

So, I finally put it together. Therefore I sort of reworked views_theme(). I added a bunch more comments and tried to better document the specification.

The interdiff goes against my last patch in #11.

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -23,6 +23,7 @@
  *   theme = "views_view",
+ *   register_theme = FALSE,

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/row/Rss.phpundefined
@@ -20,6 +20,7 @@
  *   theme = "views_view_row_rss",
+ *   register_theme = FALSE,

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/display/EntityReference.phpundefined
@@ -26,6 +26,7 @@
  *   theme = "views_view",
+ *   register_theme = FALSE,

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/row/EntityReference.phpundefined
@@ -21,6 +21,7 @@
  *   theme = "views_view_fields",
+ *   register_theme = FALSE,

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/style/EntityReference.phpundefined
@@ -21,6 +21,7 @@
  *   theme = "views_view_unformatted",
+ *   register_theme = FALSE,

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/row/Rss.phpundefined
@@ -20,6 +20,7 @@
  *   theme = "views_view_row_rss",
+ *   register_theme = FALSE,

I'm really wondering whether this is right here ... what else then views will register these theme functions.

+++ b/core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/display/DisplayTemplateTest.phpundefined
@@ -0,0 +1,29 @@
+ * Definition of \Drupal\views_test_data\Plugin\views\display\DisplayTemplateTest.

... "Contains \", sorry.

+++ b/core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/display/DisplayTest.phpundefined
@@ -18,6 +18,7 @@
  *   theme = "views_view",
+ *   register_theme = FALSE,

Oh I see you set views_view for all the displays, as it's the default value, mhhhhhhhhhh

+++ b/core/modules/views/views.moduleundefined
@@ -127,32 +133,50 @@ function views_theme($existing, $type, $theme, $path) {
+      // @todo: watchdog or exception?
+      if (!isset($def['module'])) {
+        continue;
+      }

We currently not force plugins to set the module key, but I think this would be valid at some point. Instead of watchdog() you could also call debug().

+++ b/core/modules/views/views.moduleundefined
@@ -127,32 +133,50 @@ function views_theme($existing, $type, $theme, $path) {
+      // We allways use the module directory as base dir.

allllllways ;)

+++ b/core/modules/views/views.moduleundefined
@@ -127,32 +133,50 @@ function views_theme($existing, $type, $theme, $path) {
+      // If there is no theme function for the given theme definition, we
+      // assume a template file shall be used. By default this file is located
+      // in the /templates directory of the module's folder.
+      // If a module wants to define its own location it has to set
+      // register_theme of the plugin to FALSE and implement hook_theme() by

Great addition to the documentation!

I think I agree with #19, register_theme = FALSE is reserved in my mind for plugins that don't want to register OR use any theme functions.

I think we may need to wind this back in a bit, as essentially we are trying to remedy the fact that we can't compute and values in the plugin definition as before e.g. drupal_get_path('module', 'my_module') etc...

@dawehner, register_theme has to be set to FALSE as that plugin shall not create a theme definition by itself. As those are plugins of the comment, block and node module, those need to make clear that they do not define that theme but simply use it (form views module).

For plugins that want to register the theme (currently the default behaviour as pluginmanager sets register_theme to TRUE), a module definition is needed. Maybe we shall change that default to FALSE.

@damiankloip, a plugin that does not want to use any theme functions, simply should not define "theme". register_theme is - as the name says - for registering that theme function to hook_theme().

That theme auto-register in the plugin definition is only a quick way to add theme and template definitions. All parts that are auto-registered can also be handled by the module itself in a more advanced way. Therefore the auto-register part does not need that full flexibility of setting using drupal_get_path(). Thats why -imho- that implementation should be and can be hardcoded to the expected path /templates.

That theme auto-register in the plugin definition is only a quick way to add theme and template definitions.

I think you are missing what I meant, I'm not saying it should do more. I'm saying that is the essence of what needs to be solved.

Also, unless a plugin is overriding render() itself, it will be expecting a 'theme' key in the definition.

@damainkloip, ok now I'm confused and really don't understand what you meant, I'm defenitely missing something. Maybe you can make it more clear for me :D

For plugins that want to register the theme (currently the default behaviour as pluginmanager sets register_theme to TRUE), a module definition is needed. Maybe we shall change that default to FALSE.

I would vote to leaf it to TRUE, as the most used case is the one, where views should register the theme for you.

@dawehner, register_theme has to be set to FALSE as that plugin shall not create a theme definition by itself. As those are plugins of the comment, block and node module, those need to make clear that they do not define that theme but simply use it (form views module).

Oh perfect, this makes sense!!

@damiankloip
I'm confused as well, do you think we should get rid of the automatic registering?

Converting views_accordion here to d8, stuck due to this issue... tried applying the patch, not working:

manuel@manuel-laptop:~/htdocs/drupal8$ git apply -v views-template-test-1911492-18.patch
Checking patch core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php...
Checking patch core/modules/comment/lib/Drupal/comment/Plugin/views/row/Rss.php...
error: while searching for:
 *   title = @Translation("Comment"),
 *   help = @Translation("Display the comment as RSS."),
 *   theme = "views_view_row_rss",
 *   base = {"comment"},
 *   type = "feed"
 * )

error: patch failed: core/modules/comment/lib/Drupal/comment/Plugin/views/row/Rss.php:20
error: core/modules/comment/lib/Drupal/comment/Plugin/views/row/Rss.php: patch does not apply
Checking patch core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/display/EntityReference.php...
Checking patch core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/row/EntityReference.php...
error: while searching for:
 *   title = @Translation("Entity Reference inline fields"),
 *   help = @Translation("Displays the fields with an optional template."),
 *   theme = "views_view_fields",
 *   type = "entity_reference"
 * )
 */

error: patch failed: core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/row/EntityReference.php:21
error: core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/row/EntityReference.php: patch does not apply
Checking patch core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/style/EntityReference.php...
error: while searching for:
 *   title = @Translation("Entity Reference list"),
 *   help = @Translation("Returns results as a PHP array of labels and rendered rows."),
 *   theme = "views_view_unformatted",
 *   type = "entity_reference"
 * )
 */

error: patch failed: core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/style/EntityReference.php:21
error: core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/style/EntityReference.php: patch does not apply
Checking patch core/modules/node/lib/Drupal/node/Plugin/views/row/Rss.php...
error: while searching for:
 *   title = @Translation("Content"),
 *   help = @Translation("Display the content with standard node view."),
 *   theme = "views_view_row_rss",
 *   base = {"node"},
 *   type = "feed",
 *   module = "node"

error: patch failed: core/modules/node/lib/Drupal/node/Plugin/views/row/Rss.php:20
error: core/modules/node/lib/Drupal/node/Plugin/views/row/Rss.php: patch does not apply
Checking patch core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.php...
error: while searching for:
use Drupal\Component\Plugin\PluginBase as ComponentPluginBase;
use Drupal\views\ViewExecutable;

abstract class PluginBase extends ComponentPluginBase {

  /**

error: patch failed: core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.php:11
error: core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.php: patch does not apply
Checking patch core/modules/views/lib/Drupal/views/Tests/ViewsTemplateTest.php...
Checking patch core/modules/views/tests/views_test_config/test_views/views.view.test_view_display_template.yml...
Checking patch core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/display/DisplayNoAreaTest.php...
error: core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/display/DisplayNoAreaTest.php: No such file or directory
Checking patch core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/display/DisplayTemplateTest.php...
Checking patch core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/display/DisplayTest.php...
error: core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/display/DisplayTest.php: No such file or directory
Checking patch core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/row/RowTest.php...
error: core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/row/RowTest.php: No such file or directory
Checking patch core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/style/MappingTest.php...
error: core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/style/MappingTest.php: No such file or directory
Checking patch core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/style/StyleTest.php...
error: core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/style/StyleTest.php: No such file or directory
Checking patch core/modules/views/tests/views_test_data/templates/views-data-test-display-template.tpl.php...
Checking patch core/modules/views/views.module...
error: while searching for:
}

/**
 * Implement hook_theme(). Register views theming functions.
 */
function views_theme($existing, $type, $theme, $path) {
  module_load_include('inc', 'views', 'views.theme');

error: patch failed: core/modules/views/views.module:76
error: core/modules/views/views.module: patch does not apply

Here's a partial reroll of patch #18.
I've only rerolled the part on views hook_theme implementation.. the rest of the previous patch (tests etc) I've left out...

At least this patch applies and gets plugin's twig files located properly.

Full refactor of #18...

I hope I haven't missed anything, a lot of changes, and some are now located at different places...

Oops forgot to add the new files.

Output: [error: core/modules/views/lib/Drupal/views/Tests/ViewsTemplateTest.php: No such file or directory
error: core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view_display_template.yml: No such file or directory
error: core/modules/views/tests/modules/views_test_data/lib/Drupal/views_test_data/Plugin/views/display/DisplayTemplateTest.php: No such file or directory
error: core/modules/views/tests/modules/views_test_data/templates/views-data-test-display-template.tpl.php: No such file or directory].

I guess I'm not so sure about making this patch... :S any pointers?

Patch looks fine to me, did a re-roll and used git --staged just in case it's not a testbot hiccups

Just one comment, though

+++ b/core/modules/views/tests/modules/views_test_data/templates/views-data-test-display-template.tpl.php
@@ -0,0 +1,8 @@
+This module defines its own display template.

Why is this text here?

Awesome thanks for following up on this =)

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsTemplateTest.php
@@ -43,7 +43,7 @@ public function testTemplate() {
-    $this->assertTrue(strpos($output, 'This module defines its own display template.') !== FALSE, 'Display plugin DisplayTemplateTest defines its own template.');
+    $this->assertTrue(strpos(drupal_render($output), 'This module defines its own display template.') !== FALSE, 'Display plugin DisplayTemplateTest defines its own template.');

Looks like this change is making it not green? Don't ask me why though...

@@ -24,6 +24,7 @@
  *   theme = "views_view",

@@ -20,6 +20,7 @@
  *   theme = "views_view_row_rss",

@@ -26,6 +26,7 @@
  *   theme = "views_view",

@@ -21,6 +21,7 @@
  *   theme = "views_view_fields",

@@ -21,6 +21,7 @@
  *   theme = "views_view_unformatted",

@@ -20,6 +20,7 @@
  *   theme = "views_view_row_rss",

As these are the default themes, could we also remove them, just wondering ...

@@ -13,6 +13,29 @@
+ * Via the @Plugin definition the plugin may specify a theme function or
+ * template to be used for the plugin. It also can auto-register the theme
+ * implementation for that file or function.
+ * - theme: the theme implementation to use in the plugin. This may be the name
+ *   of the function (without theme_ prefix) or the template file (without
+ *   template engine extension).
+ *   If a template file should be used, the file has to be placed in the
+ *   module's templates folder.
+ *   Example: theme = "mymodule_row" of module "mymodule" will implement either
+ *   theme_mymodule_row() or mymodule-row.tpl.php in the
+ *   [..]/modules/mymodule/templates folder.
+ * - register_theme: (optional) When set to TRUE (default) the theme is
+ *   registered automatically. When set to FALSE the plugin reuses an existing
+ *   theme implementation, defined by another module or views plugin.
+ * - theme_file: (optional) the location of an include file that may hold the
+ *   theme or preprocess function. The location has to be relative to module's
+ *   root directory.
+ * - module: machine name of the module. It must be present for any plugin that
+ *   wants to register a theme.
+ */

#1969388: Change notice: Add dedicated annotations for Views plugins introduces a proper plugin annoation so we might want to move all this documentation later?

@@ -0,0 +1,51 @@
+class ViewsTemplateTest extends ViewTestBase {

This class needs some docs.

@@ -0,0 +1,51 @@
+

No need for this empty line.

@@ -128,33 +134,48 @@ function views_theme($existing, $type, $theme, $path) {
+      if (!isset($def['module'])) {
+        continue;
+      }

Isn't it called "provider" now?

Nope. We are still using module key in @Plugin annotation

Nope, provider is added for us (and is for all annotated plugins) when the annotations are discovered. The module key will be removed.

No, because this is a really small change, and should definitely be included in this patch. Why rely on a key that might not be there (when we need it to be there) when we have a provider key that is baked into our annotation discovery?

I have saved you the bother and made that change. This is all we mean :)

I think maybe we should leave it in there; you never know what people may do with altering definitions etc... :)

I don't want to bikeshed but this blocks like every style plugin in contrib from being ported.

I updated the issue summary so it is a bit nicer to read.

Pretty straightforward and much needed. Committed to 8.x. Thanks!

+++ b/core/modules/views/views.module
@@ -126,32 +132,47 @@ function views_theme($existing, $type, $theme, $path) {
+      // For each theme registration we a base directory to look for the
+      // templates folder. This will be in any case the root of the given module
+      // so we always need a module definition.
+      // @todo: watchdog or exception?
+      if (!isset($def['provider'])) {
+        continue;
+      }
...
+      $module_dir = drupal_get_path('module', $def['provider']);

The use-case of \Drupal\Core providing views theme implementations or whatever is a bit far-fetched, but for correctness this should actually check whether $def['provider'] is a module or not. drupal_get_path('module', 'Core') simply makes no sense.

+1

Committed/pushed to 8.x, thanks!

I don't really see much point in checking this too, we're adding (at the moment at least 2 to 3) extra function calls for every plugin of every type...for an edge case that is never going to happen, we're babysitting ourselves?

I guess this *could* be useful if some module wanted to provide some theme function on behalf of another, but that's about it.

If we are keeping this in, can we at least not get the module handler for every plugin of every type (which I think is 19)? :)

Yes, that makes sense as well.

That works.

Is there an issue open to watchdog/throw the exception? Silently failing is not good.

I guess if we're adding this, we might as well minimise the function calls :)

I'm not even sure if we want this watchdog message/todo anymore? As we can't determine between a plugin providing for another module and anything else. Aside from that 'provider' will always be set from the annotation discovery, so will always be there.

I originally intended this @todo for a developer that forgot to annotate the provider for the plugin. It should help the developer to do stuff right, for that throwing an exception might be the best choice?

Besides: if modules are now called providers (I missed that), we should update the documentation. I could not find the part, where it is documented in code currently, maybe someone can help me ;) ... it simply has been to long I last looked into D8 core :D

Aside from that 'provider' will always be set from the annotation discovery...

Throwing an exception in hook_theme() is going to mean it's impossible to render a page until the bug is fixed. Personally that works for me given this can only happen when a module is in development but we need to figure out what we're doing in these cases. Would be good to discuss this a bit more.

@damiankloip, where do have that information from? The current implementation (calling views_get_plugin_definitions()) does not really say something about that. I do not find any code currently that states "provider" is filled in the definition for each plugin.

In addition we have still some Class documentation in Drupal\views\Plugin\views\PluginBase talking about modules and not providers. If there is an auto-fill of the provider for each plugin (generally in D8 Core or only in views?), using that module attribute might be obsolete.

@derhasi, as mentioned, this comes from the Annotation discovery, so take a look there (AnnotatedClassDiscovery). We have also already discussed the fact the the 'module' key is obsolete, as we're using provider (I've just added a novice issue to remove these).

Can we keep this focused on this issue? I've created two follow ups. Catch, this should take care of the concerns/discussion about the exception throwing? So I think we should commit #60, and put this issue to bed.

#2070609: Remove obsolete "module" keys from Views plugin annotations
#2070613: Figure out error handling in views_theme

Marking back to RTBC as per #61, if anyone is unhappy with this, feel free to change back.

bah

@damiankloip, thanks for pointing me in the direction. I will have a closer look there. ... and for opening the two issues.

+1 for resolving this ticket with #60

Works for me. Committed/pushed to 8.x.