Posted by jibran on February 8, 2013 at 7:06am
8 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | views.module |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | #SprintWeekend, Needs Documentation, Needs tests, VDC |
Issue Summary
Problem/Motivation
For Custom StylePlugin
<?php
/**
* Style plugin to render each item as my style.
*
* @ingroup views_style_plugins
*
* @Plugin(
* id = "my_style",
* module = "my_views_style_plugin",
* title = @Translation("My Style"),
* help = @Translation("Displays rows as my style."),
* theme = "views_view_my_style",
* type = "normal"
* )
*/
?>Views trying to locate
views-view-my-style.tpl.php in core/modules/views/templates rather modules/my_views_style_plugin
Proposed resolution
See the patch
Remaining tasks
Fix the issue.
User interface changes
None
API changes
None
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| custom_styleplugin_template.patch | 555 bytes | Idle | PASSED: [[SimpleTest]]: [MySQL] 49,568 pass(es). | View details | Re-test |
Comments
#1
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...
#2
How about this? If you want to use views'/any other theme functions add theme_path in your annotation.
#3
The last submitted patch, custom_styleplugin_template-1911492-2.patch, failed testing.
#4
Seems random failure.
#5
#2: custom_styleplugin_template-1911492-2.patch queued for re-testing.
#6
The last submitted patch, custom_styleplugin_template-1911492-2.patch, failed testing.
#7
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.
#8
I will try to write tests.
#9
#10
@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: Views Responsive Grid in drupal 8 core
#11
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].
#12
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
#13
The last submitted patch, views-template-test-1911492-11.patch, failed testing.
#14
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,
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?
#15
I have merged the two patches. #11 and #2. But test will still fail because it tries to locate template file in
core/modules/views/tests/views_test_data/instead ofcore/modules/views/tests/views_test_data/templates/. It passes when tpl file is moved tocore/modules/views/tests/views_test_data/#16
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
register_themehas to be set to FALSE.modulehas to be set in every plugin (other than views plugins). It is used to locate the module's root directory.theme_pathis NOT used to alter the destination of template file. Therefore I'd suggest to remove it completely and ...theme_fileto 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.
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?
#17
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.
#18
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.
#19
+++ 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!
#20
I think I agree with #19,
register_theme = FALSEis 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...#21
@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.
#22
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.
#23
@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
#24
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?
#25
#18: views-template-test-1911492-18.patch queued for re-testing.
#26
The last submitted patch, views-template-test-1911492-18.patch, failed testing.