Problem
- Contributed modules are not able to test their functionality, because they provide functionality for themes.
Goal
- Allow contributed modules to test their functionality by allowing modules to register themes.
Details
- Drupal core contains only a few tests that attempt to verify theme-related functionality. E.g., parsing of .info files, base/sub-theme detection and inheritance, and checking for update status.
- To make those tests possible, Drupal core ships with a few dedicated and hidden testing themes in the
/themes
directory. Each of those themes exists for a particular test case. - Only Drupal core is able to put testing themes into a themes directory.
- Contributed modules are not able to put a theme into one of the
themes
directories. Without a theme that implements the module's functionality, the functionality cannot be tested. - Modules need a way to ship with a testing theme that implements their functionality.
- The proposed solution is simple: A new
hook_system_theme_info()
is added.- It is invoked in all enabled modules after Drupal has scanned the usual filesystem locations for themes, and before the collected themes are processed.
- This allows testing modules to ship with additional testing themes, and to register and enable them when running tests.
- The new hook may lead to the impression that the separate concepts of modules and themes are getting blurred and wrongly intermixed. However, this patch does not touch any of the fundamental aspects of themes. It merely allows an enabled module to tell the system to look into additional directories to find themes. It does not change any other logic or processing that is bound to themes.
- Other implementation approaches were attempted, but failed very quickly, since they attempted to hack and squeeze in the additional theme information at runtime, not always providing the additional information to all theme info consumers, and got stale, obsolete, or lost upon clearing of statics and caches, etc.
- A side benefit of this patch is that Drupal core itself is able to move the testing themes into the test directories of the corresponding modules that need them. Since Drupal automatically registers all themes it finds in the filesystem, this change slightly improves performance when themes are rebuilt.
Notes
- This issue is not about testing a theme itself. It is about testing module functionality, which can be used/implemented by themes.
- The testing theme files in the patch are merely moved/renamed without changes.
User interface changes
None
API changes
Addition only. No existing code will be affected.
Adds hook_system_theme_info() to system.module and system.api.php.
Original report by @sun
Modules like http://drupal.org/project/edge and http://drupal.org/project/skinr are not able to test major parts of their functionality, since the functionality requires a testing theme.
Only Drupal core is able to put testing themes into /themes/tests/*. Modules have no way to do that.
Attached patch allows modules (ideally hidden testing modules) to implement hook_system_theme_info() to return additional theme .info files outside of the regular 'themes' directories in a Drupal installation, so they get picked up during the rebuilding of themes.
--
Testing themes is the primary use-case only. There are further, secondary use-cases, such as #951212: Dashboard blocks should be configured completely independently from the default/admin theme blocks or http://drupal.org/project/admin. The idea for this solution actually comes from aforementioned issue #951212, in which I successfully used this hook already, so I know that it works.
Comment | File | Size | Author |
---|---|---|---|
#119 | drupal8.system-theme-info-fix-bad-commit-109.patch | 7.89 KB | bfroehle |
#117 | drupal7-system-theme-info-953336-113.patch | 6.65 KB | xjm |
#113 | drupal7-system-theme-info-953336-113.patch | 6.65 KB | Tor Arne Thune |
#113 | interdiff-106-113.txt | 7.55 KB | Tor Arne Thune |
#106 | drupal8.system-theme-info.106.patch | 7.31 KB | sun |
Comments
Comment #1
carlos8f CreditAttribution: carlos8f commentedThe idea is reasonable. The new hook itself looks clumsy:
I understand the hook is kind of edge-case, but it still doesn't make sense to have to return 'filename' and 'name', etc. if you're already giving the full path. Can we just have the hook return,
and then derive 'filename' and 'name' from that?
Powered by Dreditor.
Comment #2
JacineOh man, I didn't realize this wasn't possible.
Having the ability to do this would obviously be a huge help. I'm not sure I'd be inclined to go through the process of creating a bunch of tests without this, since they would not cover a large proportion of use cases.
Comment #3
sunThanks for the reviews! Attached patch simplifies the return value of the hook and moves the conversion into a file object to the caller.
Comment #4
carlos8f CreditAttribution: carlos8f commentedLooks good, let's try to add a test too. Especially because this hook might not end up being implemented by core (although #951212 is certainly a good case to use it).
Comment #5
sunBefore work on tests happens, I'd like to see a fundamental yay or nay from core maintainers.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedSeems like a good solution for the problem. +1
Comment #7
David_Rothstein CreditAttribution: David_Rothstein commented+1 from me as well. This is a new hook, but it's very nicely targeted to fix the bug.
I'm not sure we need to write a specific test for this because we should be using this hook when testing other theme functionality in core. For example, after this patch is committed, I would assume that all the test themes currently stored in themes/tests could be moved out of there and associated with test modules instead (does not have to be part of this issue). That would be very nice because currently, all those test themes will get loaded from the database via list_themes() on every single non-cached page request on a real Drupal site. That wasteful behavior would not happen any more if they were added via a test module's hook implementation instead.
Comment #8
EvanDonovan CreditAttribution: EvanDonovan commented+1 - this would be a great cleanup for the theme system, and is very simply designed. Also, would definitely be a better way to do #951212: Dashboard blocks should be configured completely independently from the default/admin theme blocks, as comment #16 on that issue states.
Comment #9
sunActually, even though I saw all of those testing themes when doing dsm()s for this patch, I totally didn't think of potential performance improvements David mentioned in #7. That's correct: We are needlessly loading, parsing, and preparing 3 hidden testing themes on each and every Drupal site. I suspect the performance impact is negligible, but nevertheless, tagging with Performance.
"Needs committer feedback" doesn't seem to work for non-RTBC issues, so bumping to RTBC in the hope to get some.
Comment #10
webchickIt's more that "Needs committer feedback" doesn't work when said committer is on-site with a client and has no availability for anything but critical bug triage for two weeks...
It's a little redundant to essentially have the name of the .info file there twice (once as the key in the array, once as the actual file in the filepath), but I can't really think of a better thing to use as the key.
This looks basically okay, but it looks like theme_test module (or something) needs to implement this hook in order to register the test themes and gain the stated performance benefit?
Comment #11
dmitrig01 CreditAttribution: dmitrig01 commentedMy module, wp_theme (http://drupal.org/project/wp_theme) would really, really, really, like to use this as well.
Comment #12
EvanDonovan CreditAttribution: EvanDonovan commented@sun: So it sounds like webchick is cool with this, as long as you add to the patch an implementation in the theme_test module....
Comment #13
sun.
Comment #14
sunSo let's try and see what fails.
Comment #16
sunD'oh. Wrong testing theme paths.
Comment #17
sunPassed! Hence, all of the existing Theme API and Update module tests are sufficient.
Comment #18
sunAny other feedback? Or is this RTBC?
Comment #19
carlos8f CreditAttribution: carlos8f commentedLooks good to me. Moving /themes/tests to /modules/simpletest/tests/themes/ is a nice win. Let's just hope none of this stuff gets lost in the CVS commit :)
Note to @Dries/@webchick: we can safely remove the /themes/tests folder after this patch.
Comment #20
sunGood call.
That should cut it.
Comment #21
webchickI think this makes sense, on the grounds of:
"Only Drupal core is able to put testing themes into /themes/tests/*. Modules have no way to do that."
It also keeps all of our testing code in the modules* or profiles* (which are basically modules in D7) directories, rather than messing with themes.
However, this definitely feels like the kind of issue that needs Dries's sign-off, since it really murkies up the distinction/relationship between modules/themes.
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedBelated review, but the patch (as well as the CVS spaghetti instructions in #20) look good to me also.
Comment #23
sun#16: drupal.system-theme-info.16.patch queued for re-testing.
Comment #24
sun#16: drupal.system-theme-info.16.patch queued for re-testing.
Comment #25
sun#16: drupal.system-theme-info.16.patch queued for re-testing.
Comment #26
sun#16: drupal.system-theme-info.16.patch queued for re-testing.
Comment #27
sun#16: drupal.system-theme-info.16.patch queued for re-testing.
Comment #28
sun#16: drupal.system-theme-info.16.patch queued for re-testing.
Comment #29
EvanDonovan CreditAttribution: EvanDonovan commented@webchick: You said in #21 that you were OK with this, I think, but wanted Dries to take a look? Is that still possible?
Comment #30
Kiphaas7 CreditAttribution: Kiphaas7 commented#16: drupal.system-theme-info.16.patch queued for re-testing.
Comment #31
bfroehle CreditAttribution: bfroehle commented~
Comment #32
sunThis should really get in prior to release.
Comment #33
webchickI don't really understand why it would have to. It doesn't break anything API-wise, so could just as easily make it into 7.1, 7.2...
Comment #34
sun#16: drupal.system-theme-info.16.patch queued for re-testing.
Comment #35
JacineIt would really be nice to have this as soon as possible.
Comment #36
sunAt minimum the Skinr project really needs this patch now. A big part of the module's functionality is based around themes. It needs to test proper basetheme/subtheme inheritance (#1015614: Subtheme inheritance), theme hooks, and other stuff, which is impossible without this patch.
Comment #37
sun#16: drupal.system-theme-info.16.patch queued for re-testing.
Comment #38
sun#16: drupal.system-theme-info.16.patch queued for re-testing.
Comment #39
sunIf anyone could tell me what's missing or required to get this patch in, that would be great.
Comment #40
moonray CreditAttribution: moonray commentedsubscribe
Comment #41
effulgentsia CreditAttribution: effulgentsia commented+1. We need to get it into D8 first, right? But please, let's do that, and follow up with a quick backport to D7 too. @Dries: see #21, webchick punted this to you.
Comment #42
sun#16: drupal.system-theme-info.16.patch queued for re-testing.
Comment #44
bfroehle CreditAttribution: bfroehle commentedFrom @rfay in http://groups.drupal.org/node/141529
Perhaps this patch be cleaned up as well...
Comment #45
sunoh dear, what a freakin' mess. Re-rolled against HEAD.
Comment #46
sunBack to RTBC.
Comment #47
catchTagging for backport.
Comment #48
sun#45: drupal8.system-theme-info.45.patch queued for re-testing.
Comment #49
sun#943212: hook_form_system_theme_settings_alter() vs. THEME_form_system_theme_settings_alter() name-clash would benefit from this patch.
Comment #50
sun#45: drupal8.system-theme-info.45.patch queued for re-testing.
Comment #51
sunWe have
for more than half a year here.
So pretty please, what's the effin' reason for not committing it?
Comment #52
JacineYeah, I have to agree with @sun. Not having this patch in has negatively affected the progress of the 7.x version of Skinr quite a bit, so it would be great to see this committed and back-ported soon.
Comment #53
moonray CreditAttribution: moonray commentedAgreed. Need this patch in ASAP.
Comment #54
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #55
webchickSomething that would be helpful to know here is why the people +1ing this are +1ing it. It looks atm like apart from sun, no one has actually tested this patch, and are saying it looks good on visual inspection. Is there another use case out there apart from Skinr? If so, could more details be provided?
Not moving from RTBC, but some more details here would be great.
Comment #56
catchJust this from #7 is sufficient reason.
Comment #57
moonray CreditAttribution: moonray commentedI've +1-ed this patch because I've been running it on Skinr patches for months now. It was already marked as RTBC, so I didn't figure I needed to add much more. I have yet to run into any issues with it.
Also, besides #7, #8 and #49 link to issues in core that could benefit form this patch. #7 in itself seems big since any resources saved to cut down on core testing time at this point would be beneficial.
Comment #58
catch#7 isn't just testing time. All themes in the themes directory are loaded into memory every request on every Drupal install, because there is no real install status for themes, so this is an albeit small saving but one that will affect every full bootstrap for every D7 install.
Comment #59
JacineI'm +1ing this because we cannot run the unit tests we've written for Skinr without it. This means having the testbot enabled for our project is useless and that anyone who wants to help out and tests needs to hack core to do so. I've been running this patch since November of last year and haven't run into any problems with it at all.
Skinr is not the only use case here. Any module that needs to test its functionality in a "test theme" is screwed without this patch. @sun mentioned a few use cases in the OP, there's the Edge module, and @dmitrig01 mentioned he wants to use it for his wp_theme module in #11.
I would imagine there aren't many modules in this category that actually have unit tests yet, though. Almost everyone asking for this is a core developer, and there isn't a huge amount of contrib modules with unit tests so far. If we leave this, and make it impossible for these modules to write tests that actually work with the testbot, we're going to have a hard time getting people to write tests.
Comment #60
sunSummary:
Problem
Goal
Details
/themes
directory. Each of those themes exists for a particular test case.themes
directories. Without a theme that implements the module's functionality, the functionality cannot be tested.hook_system_theme_info()
is added.Notes
HTH
Comment #61
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedIt seems like catch, moonray and Jacine has explained their +1 from previous posts. Comment #41 is really the only unexplained +1 now, as I see it.
Comment #62
webchickIndeed; now if only one of those folks had given this issue a detailed tech review. :\
Anyway, Dries has been pinged again about this issue so hopefully he'll be able to get it to it in the next week or so.
Comment #63
catchI've reviewed the patch several times but couldn't find much to nitpick, it looks like I never actually said that on this issue though.
Only two things stand out:
This is one of my very favourite hook names ever.
This is another situation where we're adding a hook to a low level API function, and concerns me a bit in the same way that #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap concerns me.
However there are two reasons it doesn't concern me that much:
1. We already have two hooks like this (hook_system_info_alter() and hook_registry_files_alter()) - this isn't something really introduced by the patch.
2. Much more importantly, the theme system is considerably higher level than the module system and registry - i.e. you very rarely try to use a theme without modules being loaded, so in terms of extricating these circular dependencies it is probably the least of our worries. Also hook_system_info_alter() I'm guessing runs for themes already, so meh.
Comment #64
chx CreditAttribution: chx commentedI dunno, using a hook for this gives me the willies. Let's say you run
drupal_system_listing('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.info$/', 'modules', 'name', 0);
and throw away (i think array_diff_key) those returned bydrupal_system_listing('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.module$/', 'modules', 'name', 0);
. To me, this looks like we will get a list of info files that are under the modules search path but do not have modules belonging to them. (Yes, there are more modules than info files but that does not affect us right now) What those would be , I wonder. Maybe themes under the modules, hm?Edit: note that this can not break anything because modules are discovered by the second expression and that's exactly what we do not take into consideration so there is no way we mistake a module for a theme.
Edit2: http://drupal.org/node/474684#comment-4473162 allows you to depend on modules. We need the modules list for that but we already need the modules list for what I described above. Win?
Comment #65
chx CreditAttribution: chx commentedI think this is CNW. Is there a "needs discussion" status :) ?
Comment #66
catchAt least the memcache project used to ship with memcache.info but no memcache.module. Last release actually added a module but that's at least one example where there was a .info in the filesystem without a .module file accompanying it. Cnr seems fine for needs discussion.
Comment #67
sun@chx: Thanks - while that's a nice idea,
Comment #68
chx CreditAttribution: chx commented@catch wrong it had a .module it was empty and it needed to have one -- @catch, @sun check _system_rebuild_module_data it scans for .module files. Without a .module it never recognizes your project. I know, I just hit this the weekend, check modulate 1.0 it had to have a 0 byte module file.
Comment #69
catch@chx, no, it didn't. Look at the 6.x-1.4 tag of memcache and find memcache.module in there.
Comment #70
chx CreditAttribution: chx commentedWell, go for it. As these themes depend on the modules , short of a hook we can have only ugly workarounds.
Comment #71
jct CreditAttribution: jct commentedAdded issue summary
Comment #71.0
jct CreditAttribution: jct commentedAdd issue summary
Comment #72
sunThanks. However, there already was a very concise summary in #60. I've replaced the new with that existing.
Comment #73
jct CreditAttribution: jct commentedCool, thanks for fixing it, @sun. Trying our hand at an issue summary sprint here at DrupalCamp Wisconsin.
Comment #73.0
jct CreditAttribution: jct commentedUpdated issue summary.
Comment #74
sunComment #75
webchickPutting this into Dries's queue to review (sorry, sun).
Comment #76
sunRe-rolled with file rename detection.
Comment #77
aquariumtap CreditAttribution: aquariumtap commentedI've tested and used this patch with Skinr, which is required for its automated tests.
@webchick, re: #55:
The next version of Fusion will also require this patch to test some of its functionality. To repeat a distinction @sun made, this patch is not about testing the theme, it's about testing the module's interaction with a theme. We are building a module that provides theme functionality.
So, +1 :)
Comment #78
pillarsdotnet CreditAttribution: pillarsdotnet commentedFor HTMLMail to have tests, this patch is needed. There have been several issues where template overrides located in the theme directory are not working as expected, and having tests would be really helpful in establishing, documenting, and ensuring proper behavior.
Comment #79
tstoecklerAlso, Color module currently tests itself by turning on Garland, and should really ship with a testing theme instead (follow-up issue, though).
Comment #80
stephthegeek CreditAttribution: stephthegeek commentedsubscribing
Comment #81
sheena_d CreditAttribution: sheena_d commentedsubscribing
Comment #82
catch#76: drupal8.system-theme-info.76.patch queued for re-testing.
Comment #83
catch#76: drupal8.system-theme-info.76.patch queued for re-testing.
Comment #84
catchI've opened #1292284: Require 'type' key in .info.yml files to discuss adding a type key to .info files, that'd let us pick up any kind of extension-type thing anywhere in the file system but also allow for .module-less module files.
I'm going to leave this in Dries' queue since it's been there for a while already.
I'd support committing this for now since it blocks contrib module testing, and in the realm of hacks we have to support testing (like hook_registry_files_alter()) it is a small one.
Comment #85
chx CreditAttribution: chx commentedThe wording is , by far , not strong enough. This needs to emphasize never to use this feature for anything but testing. Chaos would prevail otherwise as people ship ordinary themes as modules.
Comment #86
sunClarified the phpDoc to be for testing purposes only.
Comment #87
pillarsdotnet CreditAttribution: pillarsdotnet commentedIt might be nice to include a pointer to the Mock Modules page which explains that Mock (hidden) modules (or themes), have
hidden = TRUE
in their.info
files, and also explains that they're used for testing certain hooks:Comment #88
sunAt the time you're writing tests you should already know about .info file properties and how to hide extensions via the hidden property.
Comment #89
pillarsdotnet CreditAttribution: pillarsdotnet commentedSure; whatever. I vaguely remember hearing about hidden modules, but had to do several searches to find the docs on how to implement one.
Of course, we want to set the bar high enough that stupid people like me never hack core, and the tests I've written while in my ignorant unlearned state are probably unworthy and should be removed.
Comment #90
pillarsdotnet CreditAttribution: pillarsdotnet commentedNevertheless...
Comment #92
pillarsdotnet CreditAttribution: pillarsdotnet commentedOkay; obviously I broke something. Re-uploading the patch from #86.
Comment #93
pillarsdotnet CreditAttribution: pillarsdotnet commentedNow I see what I did wrong -- didn't check "git status" after applying the base patch, and hence missed the added directories.
Re-rolled.
Comment #94
sunComment #95
pillarsdotnet CreditAttribution: pillarsdotnet commentedOkay; fixed from the other end, then. Now at least I can search for "hidden modules" and find the right page:
http://drupal.org/node/302577/revisions/view/630432/1684272
Comment #96
catchLooks like the comment is identical to me "most commonly used".
Comment #97
pillarsdotnet CreditAttribution: pillarsdotnet commented@sun's patch is in two parts. You have to read all the way through to catch the comment change.
Comment #98
chx CreditAttribution: chx commentedWell let it be but the next unnecessary git format-patch will be thrown back to CNW. Edit: make sure to use git apply for this patch.
Comment #99
catch#94: drupal8.system-theme-info.86.patch queued for re-testing.
Comment #101
Dries CreditAttribution: Dries commentedI re-visited this patch and I believe it makes sense. It's "yet another weird hook" (YAWH) but it gets the job done.
It looks like the patch in #94 need to be re-rolled though. Once it passes again, it can be committed by either catch or myself.
Thus, I'm removing this issue from my queue as a decision has been made.
Comment #102
chx CreditAttribution: chx commentedOK then, rerolled.
Comment #104
chx CreditAttribution: chx commented#102: 953336_102.patch queued for re-testing.
Comment #106
sunComment #107
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThe new patch looks good to me. The difference is that it just includes a new test theme (together with the others as before) that was added after the previous patch was made.
Comment #108
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedSince it's just a re-roll, setting back to RTBC.
Comment #109
Dries CreditAttribution: Dries commentedCommitted to 8.x. Moving to 7.x for Angie to review.
Comment #110
rfayLooks to me like this one broke 8.x block tests: http://qa.drupal.org/8.x-status
Comment #111
xjmLooks like the commit did not include the moved files: http://drupalcode.org/project/drupal.git/commit/cb4d787
git status
git add .
;-)
Comment #112
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedAnd a
git rm core/themes/tests/README.txt
is needed too.EDIT: Or not... Didn't check the commit. I just assumed, and you know what they say about that ;)
Comment #113
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedHere's the D7 version of the patch committed to 8.x (#106). Attaching an interdiff between the D8 patch in #106 and the attached D7 version to show that the only changes are path changes. Also one file didn't exist in D7 (
test_theme/theme_test.template_test.tpl.php
).Leaving without -D7 prefix so that the patch can be tested when the time comes to move this issue to 7.x-dev.
Comment #117
xjmSame as above, just re-uploading 'cause I tested the other against 8.x.
Comment #118
xjmFor real this time, back to 8.x/NW for #110. :) Sorry about that.
Comment #119
bfroehle CreditAttribution: bfroehle commentedAttached is a patch which, when applied to 8.x, fixes the bad commit in #109 (by adding the missing files).
git apply --index
is quite useful...Comment #120
xjmPatches can't be tested now, soooo...
Comment #121
webchickOk, I think I committed and pushed that, so D8 should be fixed. If not please mark back and I'll check again later tonight. Thanks for the great tip in git apply --index. I am totally going to use that from now on. I also learned about git clean -df. :)
The patch for D7 is too late for 7.10 (tomorrow), but I can review for commit either later this week or next week. I've already reviewed it in the past though, so if it's good for Dries, I should be able to commit it more or less from there.
Comment #122
webchickOops. And restoring status.
Comment #124
xjmRTBC for #113.
Comment #125
webchickI guess it was next week. ;)
The only thing that's slightly worrisome on D7 is the moving around of those themes, since people doing FTP uploads overriding old files will likely not remember to go back and remove the tests from the other directory. However, in testing locally that didn't seem to bring up any errors.
Soooo....!
Committed and pushed to 7.x. Hopefully! :)
Comment #126
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedGreat! Another long-going major bug bites the dust. The commit looks good :)
Comment #127.0
(not verified) CreditAttribution: commentedClearer indication that it is an API addition only.