According to http://drupal.org/node/1805952 we should move the files
to a "templates" folder. After that it doesn't really make sense to have a theme/theme.inc file, so let's move this to either the root
or the include folder.

CommentFileSizeAuthor
#17 vdc-1805980-17.patch20.68 KBtim.plunkett
#17 interdiff.txt2.51 KBtim.plunkett
#14 vdc-1805980-14.patch20.02 KBtim.plunkett
#14 interdiff.txt3.3 KBtim.plunkett
#13 vdc-1805980-13.patch20.34 KBtim.plunkett
#12 vdc-1805980-12.patch12.45 KBtim.plunkett
#6 views-1805980-6.patch10.46 KBtim.plunkett
#1 vdc-1805980.patch12.37 KBtim.plunkett

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Project: Views (for Drupal 7) » VDC
Version: 8.x-3.x-dev »
StatusFileSize
new vdc-1805980.patch12.37 KB

This is a patch on top of the sandbox.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Active » Needs review

Nice diffstat.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

This looks great so far.

I'm not sure whether the theme.inc should be part of the templates folder, but they are certainly related.

xjm’s picture

xjm
she/her
Primary language English
commented 
+++ b/core/modules/views/views.moduleundefined
@@ -108,13 +108,12 @@ function views_pre_render_view_element($element) {
+    'file' => 'views.theme.inc',
+    'path' => drupal_get_path('module', 'views') . '/templates',

diff --git a/core/modules/views/theme/theme.inc b/core/modules/views/views.theme.inc
rename from core/modules/views/theme/theme.inc
rename to core/modules/views/views.theme.inc

Maybe it's too early in the morning and I'm confused, but aren't these specifying different paths?

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs manual testing

Maybe double-check manually that everything still works. ;)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Project: VDC » Views (for Drupal 7)
Version: » 8.x-3.x-dev
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new views-1805980-6.patch10.46 KB

Moving back. I did mess it up.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Assigned: Unassigned » tim.plunkett
Status: Needs review » Needs work

This still needs changes. the /templates part should be autodiscovered

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Priority: Normal » Major
Status: Needs work » Postponed

Holding off on this to avoid the changes to the plugin managers.

merlinofchaos’s picture

merlinofchaos commented

One reason that theme.inc is in the same folder as the templates is that, in the past, you couldn't have separate include directories for the two files in hook_theme() -- meaning that preprocesses and such must be in the .module file or a file that is always included and cannot be automatically included by the theme system. I have no idea if this was ever changed for D8, however.

It's also nice to have the preprocesses right there with the templates, IMO.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Yeah, I think what will happen is just

git mv theme templates
rename from theme/theme.inc
rename to templates/views.theme.inc

So, they'll stay together.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: Code » views.module
Priority: Major » Normal
Status: Postponed » Active
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Assigned: tim.plunkett » Unassigned
Status: Active » Needs review
StatusFileSize
new vdc-1805980-12.patch12.45 KB

Rerolled.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new vdc-1805980-13.patch20.34 KB

The current system makes it prohibitively hard to provide your own theme functions for plugins. In addition, we have all of this custom code for a feature that I have yet to find a usage of in contrib: the ability for a plugin to declare custom templates PER THEME. It's not something that hook_theme supports, and Views shouldn't go out of its way to add custom functionality like that.

In addition, this switches spaces to underscores in keys that are used in annotations.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new interdiff.txt3.3 KB
new vdc-1805980-14.patch20.02 KB

We no longer need the $base stuff in views_ui_theme().

dawehner’s picture

dawehner
Primary language German
commented

Absolute nice cleanup! The changes are looking perfect, beside one small change.

+++ b/core/modules/views/views.moduleundefined
@@ -139,40 +137,27 @@ function views_theme($existing, $type, $theme, $path) {
-      if (isset($def['theme']) && (!isset($def['register theme']) || !empty($def['register theme']))) {

Can we let the register_theme bool in? Afaik drupal.org is using this flag for project module.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Needs work

We can, but we should document it and switch it to register_theme.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt2.51 KB
new vdc-1805980-17.patch20.68 KB

Okay, I added the default value to PluginManager, and a comment. I also moved some more stuff around in views_themes when I actually tried using it from contrib.

The interdiff is with -w to hide the indents.

dawehner’s picture

dawehner
Primary language German
commented

From the code side this looks RTBC now, considering that the docu will be done in bulk.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

#17: vdc-1805980-17.patch queued for re-testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: Move templates to a template folder and theme.inc probably to the include folder » Rename /theme to /templates and theme.inc to views.theme.inc
damiankloip’s picture

damiankloip commented
Status: Needs review » Reviewed & tested by the community

This looks good to me. Nice clean up.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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