Especially when dealing with drupal_render() structures, it can be extremely difficult to determine if there is a configuration error in a theme function. That's because if a theme key is not found, theme() will just silently return NULL with no information, on screen or otherwise. That makes debugging a *expletive deleted*.
The solution that would save me and everyone else countless hours with a real time debugger just tracing through stuff would be for theme() to report a warning and possibly log to watchdog every time it can't find a theme function. This is a one line fix that can probably even be backported to D6, as it does not change any strings, just adds one.
I can do this, but I'm in a hurry right now so I'm just marking it as a novice patch for now in the hopes that someone beats me to it. :-)
Comment | File | Size | Author |
---|---|---|---|
#66 | theme_fail.patch | 1.61 KB | chx |
#65 | 412730-65.patch | 349 bytes | mikey_p |
#60 | just_return.patch | 548 bytes | catch |
#57 | theme_watchdog.patch | 1.8 KB | Crell |
#52 | theme-exception-412730-52.patch | 7.53 KB | JohnAlbin |
Comments
Comment #1
jbomb CreditAttribution: jbomb commentedComment #2
jbomb CreditAttribution: jbomb commentedCrell:
the following patch will write a record to the watchdog log when an unregistered theme function is called using theme();
Is this what you were looking for?
Comment #3
jbomb CreditAttribution: jbomb commentedComment #4
Crell CreditAttribution: Crell commentedClose! Three problems:
1) The theme key is not always a function. So an error message that says theme_$key is not found is wrong, since that doesn't have to exist. It could be a template. Probably just say "theme key @key not found".
2) $msg should be $message, or $error_message. Drupal favors descriptive variable names over terse ones.
3) In addition to the watchdog, I'd also include a drupal_set_message() of level "warning". Possibly that should only display if the "display errors to screen" variable is set. In fact not possibly, it should make use of that. :-) It should still log to watchdog either way.
Thanks for picking this up!
Comment #5
jbomb CreditAttribution: jbomb commentedCrell:
Thanks for the pointers!
I updated the patch as you described. Though I noticed it returns a warning about 'system_settings_form' on most of the administrative pages.
ex. /admin/settings/clean-urls
I'm not sure if this is desired functionality or not. Perhaps you're in a better position to make determination.
I should also note that I used '2' as the default for the error reporting level because '2' is the default on the error reporting log settings form found in /module/system/system.admin.inc (/admin/settings/clean-urls).
Comment #6
cburschkaWatchdog and status messages are user-facing (and translatable) text and should therefore employ full, properly capitalized sentences.
Comment #7
jbomb CreditAttribution: jbomb commented@Arancaytar: good point.
Comment #8
Crell CreditAttribution: Crell commentedI like it.
Comment #9
webchickLet's use a constant there instead of the 2. ERROR_REPORTING_DISPLAY_ALL, I believe. I'm still not quite sure how I feel about displaying a user-facing message for this. Hrm...
Also, we should have some tests for this. jbomb, are you feeling extra sparky? :)
Comment #10
webchickYeah. The more I think about this the more I really hate the idea of a dsm() here. Let's make it just a watchdog call, please.
We were talking on IRC and an idea came up for future expansion (in another issue): a "debug mode" or "development mode" for core that clears caches at the end of each request for hook_menu/theme and friends, and also dsm()s any watchdog 'warning'+ messages, etc.
Comment #11
Crell CreditAttribution: Crell commentedThe on-screen message was deliberate and was my original feature request. If I'm developing a site, I want to get "missing theme key" thrown in my face when it happens; not have to spend ten minutes figuring out what the problem is before I remember to go check watchdog. If you want to change the error reporting threshold for it, fine, but the whole point of this patch (IMO) is to throw it in the developer's face when building a module. (This is a *major* problem when writing, say, a CCK formatter, which is nested ridiculously deep in a render array.)
Comment #12
webchickI agree it's a problem. I've been bitten by it many times. But it's establishing a precedent of:
a) babysitting broken code
b) doing so in "user space"
Ideally, this would be added to Devel module instead, but I don't really think that's possible.
Comment #13
jbomb CreditAttribution: jbomb commentedSo, it sounds like the user-facing error message that Crell is asking for will be addressed in the debug/development mode that webchick mentioned in #10, but for now the missing key will be reported in watchdog only?
@webchic in regards to testing: I'd be happy to write some tests for this, could you possibly guide me in terms of what the tests should be testing?
Comment #15
jbomb CreditAttribution: jbomb commentedComment #16
jbomb CreditAttribution: jbomb commentedComment #17
Crell CreditAttribution: Crell commented@webchick: The only way I could see what you're talking about working is if devel had a mode to dsm() all watchdog entries over a certain threshold. I could actually see that being useful on its own, now that I think about it. If we can add that to D7 devel, then I withdraw my insistence that theme() print to the screen itself.
Comment #19
chx CreditAttribution: chx commentedCrell, please see hook_watchdog -- yes devel can display a message for every watchdog call, it's easy and been so since D6. No, this is not babysitting broken code that would be adding separate code paths for dealing with broken code. That's what the current code IS! Remove this if and you will get ample notices that something is wrong. How's that?
Comment #20
Crell CreditAttribution: Crell commentedTagging. This is being discussed in IRC right now. Possibly we'll remove the check and let Drupal just fatal out. :-)
Comment #21
pwolanin CreditAttribution: pwolanin commentedSeems like this is really only a developer issue?
I'd still prefer watchdog and return an empty string (not NULL as it does now).
Comment #22
Crell CreditAttribution: Crell commentedAfter discussion in IRC, here's a patch that replaces the "return NULL" with "throw an exception". That is, as chx said we remove a silent failure that is babysitting broken code with no way to figure out what's broken and replacing it with a big neon "hey, dude, you've got a bug, it's your problem, go fix it". :-)
Comment #24
Crell CreditAttribution: Crell commentedUh, seriously? How is that possible unless we have code now that's buggy and returning NULL and we don't know it?
Comment #25
chx CreditAttribution: chx commentedAbsolutely not. If we want to litter Drupal with all sorts of exceptions we can do that in D8 if we manage to agree. For now try just removing the guardian if. However, i have the suspicion that it first tries theme(form_id) and when the output from that is empty, proceeds to the normal rendering.
Comment #26
chx CreditAttribution: chx commentedHowever, theme(form_id) is checked against the theme registry first and that should be factored out and use for system_settings_form:
Comment #27
chx CreditAttribution: chx commentedOr alternatively you could move that piece of code from drupal_build_form to drupal_render itself and check there. Make sure to use a static (and not a drupal_static) to keep the theme registry otherwise the speed penalty will be big.
Comment #28
chx CreditAttribution: chx commentedLast comment for now -- but then you changed "calling theme('foo') where 'foo' is not a valid key returns silently" to "setting $renderable['#theme'] to 'foo' which is not a valid key returns silently". However, the latter is not a bug but an actual feature that our system apparently uses so probably we do want to live with it and remove that guardian if from theme() so it fails noisily -- there is no need for an exception.
Comment #29
eaton CreditAttribution: eaton commentedMy understanding is that
$renderable['#theme'] = 'foo';
just results in a call to theme('foo', $renderable) at present.The most recent patch Crell posted doesn't seem to include anything but a thrown exception in the theme() function, so I'm not sure where the problem with renderables being handled differently comes in. Perhaps I'm missing something important?
I too lean towards the watchdog message with contextual information about where it was called, only because throwing exceptions is currently a very rare case inside of Drupal, and breaks most of our standards. I agree that it should be standard in a future release, but it's not the kind of thing we can do piecemeal. watchdog it, return silently, and leave a // TODO noting that it should be an exception in the future?
Comment #30
Crell CreditAttribution: Crell commentedWe discussed this patch in IRC with webchick, and she OKed the exception approach rather than watchdog on the grounds that calling a non-existent key is a bug. It was only after writing it that we realized that FAPI actually relies on that bug in a few places, which is itself a bug. chx is suggesting a way to move that fallback behavior into FAPI itself rather than the theme layer that I don't fully grok. Eaton, since you know FAPI better than I do perhaps you can figure out the patch chx is talking about?
Comment #31
eaton CreditAttribution: eaton commentedFair point. If it's gotten the signoff, I'm actually totally fine with an exception being used.
#211015: #theme for System Settings Form prevents theming is related to this - quicksketch wanted it removed entirely. If anything, we should simply implement a default version of theme_system_settings_form() and have it return drupal_render($form).
Would everyone be happy with that approach?
Comment #32
mattyoung CreditAttribution: mattyoung commented.
Comment #33
chx CreditAttribution: chx commentedRegardless whether you convinced Angie outside of the issue that an exception here is a good idea it's not. a) Drupal does not throw an exception anywhere else outside of DBTNG. Doing that is a sweeping Drupal 8 feature. b) If you remove that few lines checking for an existing theme key, theme() blows up quite noisily so adding anything else is bloat and Drupal hates bloat.
Comment #34
Crell CreditAttribution: Crell commentedI like it so much that I wrote it. :-) For the record, though, it needs to be drupal_render_children(). (Hat tip to chx for that.)
I've attached 2 patches. One throws an exception, the other doesn't even bother checking and lets PHP throw nasty errors on its own. I can live with either of these, as long as one of them gets in so that I don't waste another 30 hours digging through nested drupal_render() calls looking for a typo.
Comment #35
Crell CreditAttribution: Crell commentedCorrection, here's the non-exception version. I forgot to remove the exception definition itself.
Comment #37
Crell CreditAttribution: Crell commentedconfirm_form() fail!
So confirm_form() does something very silly. It sets a #theme key of "confirm_form" on its wrapping form, but does not implement the corresponding theme function. Presumably that's for the same reason as system_settings_form(). However, the same fix does not work there because when you implement theme_confirm_form(), its form structure is so wonky that calling drupal_render() from inside theme_confirm_form(), which you have to do for forms to work, results in an infinite loop. I found no way to not make it do that. Thus, I conclude that the would-be override there never worked in the first place and we just never tested for that.
I have therefore removed the #theme key from confirm_form(), and things seem to be working now. This is not a feature removal because it's an undocumented trick that never worked in the first place, so we're not losing any functionality.
This is just the exception-based version of the patch. I asked webchick which version she wanted me to keep rolling and she said exception, so that's what I'm doing.
Hey bot, take this!
Comment #39
Crell CreditAttribution: Crell commentedARGH! APIs changing out from underneath me!
Right, so, the patch that changed the function signature of theme functions to always take an array bit me here. That was breaking BOTH system_settings_form and confirm_form until I realized that was the issue. Word to the wise: if you don't make that change, you get infinte loops. :-)
So here's a version that uses exceptions and should have working default implementations of both theme_system_settings_form and theme_confirm_form.
Come on, bot, work with me here...
Comment #41
Crell CreditAttribution: Crell commentedSo apparently some smart alec decided it was a good idea to take various forms in core and give them super-secret undocumented theme override capability that doesn't match the normal theme patterns, relying on the theme system to go out of its way to hide the fact that you're calling theme functions that don't exist. Said smart alec has now wasted me at least 8 hours this week just tracking down all the places that we had super-secret undocumented theme functions that don't actually exist, because they complain loudly when the theme system is switched to complain loudly at missing theme functions. Gee, who'd a thunk it?
I do hope I got them all now...
Comment #42
Crell CreditAttribution: Crell commentedI keep forgetting to properly tag this...
I should note that the existence of such super secret undocumented theme functions that don't exist is incontrovertible proof that we need this patch to avoid such super secret undocumented crap, as well as to catch more traditional bugs. Argh!
Comment #43
chx CreditAttribution: chx commentedRegardless whether you convinced Angie outside of the issue that an exception here is a good idea it's not. a) Drupal does not throw an exception anywhere else outside of DBTNG. Doing that is a sweeping Drupal 8 feature. b) If you remove that few lines checking for an existing theme key, theme() blows up quite noisily so adding anything else is bloat and Drupal hates bloat.
Comment #44
chx CreditAttribution: chx commentedComment #45
Crell CreditAttribution: Crell commentedFalse. There are currently exceptions defined as part of DBTNG, the Field system, the update system, and the new file transfer system. So yes we do have exceptions outside of DBTNG already.
It is also not bloat. Exceptions provide better debugging context for people to figure out WHY their code exploded in flames. The whole point here is to make bugs bleeding obvious so they can be fixed quickly. 1 IF statement and a throw line (2 lines of actual code) give us better debug context so we know where we went wrong. That's not bloat, that's not babysitting, that's correct error handling procedure: Complain loudly at me to say what I did wrong so I can fix it rather than trying to fix it for me.
And no, I did not convince Angie. There was a several-person discussion that ended up at exceptions. That's a not unusual way for such issues to get hashed out. The above is the basic reasoning behind it, which is sound.
Comment #46
chx CreditAttribution: chx commentedBe it.
Comment #47
webchickI wasn't "convinced" of anything. While I agree that a sweeping, Drupal-wide exception class is Drupal 8 material, I don't see any reason why we can't trap our own errors in the critical path, especially when they're fatal.
However, it looks like no one actually tested this, because the message is being double-escaped:
Comment #48
Crell CreditAttribution: Crell commentedAnother advantage of exceptions is that they're much easier to unit test for, at least when one remembers to do so. *sigh*
Comment #49
Crell CreditAttribution: Crell commentedAnd this version does NOT have the random debug flotsam left lying around index.php...
Comment #50
cweagansComment #52
JohnAlbinTrivial re-roll. Back to RTBC.
Comment #53
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #54
David_Rothstein CreditAttribution: David_Rothstein commentedThis seems to have broken command-line installs? (oh, I can't wait until we have tests for that...):
I think it's because install.php doesn't load the theme system in those cases (it shouldn't really need to), but likely there are calls to theme() that happen anyway. So the installer hits the exception, and then doesn't even catch it correctly because it's not looking for ThemeHookNotFoundExceptions (although that's a secondary problem).
Seems like this will either be really simple or really complicated to fix.
Comment #55
pwolanin CreditAttribution: pwolanin commentedPlease roll this back - it makes it nearly impossibly to test patches that add new theme function, especially if the theme function is called on all pages since there is then an uncaught exception on every page and no way to rebuild the theme registry except DB hacks.
Comment #56
catchPatch no longer applies when reverting - needs a re-roll.
Comment #57
Crell CreditAttribution: Crell commentedRight, so this patch takes us back to where we were in #15 above: watchdog(), and then add a "show watchdog calls as messages" checkbox to Devel. I know of no way to unit test that a watchdog was or was not triggered, so I am simply removing the exception-expecting unit test.
Comment #58
moshe weitzman CreditAttribution: moshe weitzman commentedComment #59
webchickThis isn't quite as nice, since it won't trigger a visible error condition, but the chicken/egg scenario Peter describes in #55 is much worse. Also, this broke command-line installation.
Committed #57 to HEAD.
Comment #60
catchThis caused all other kinds of weird test failures in cli installs and for me running simpletests via the ui. Posting a full revert to get us back to normal then we can fix properly later.
Comment #61
pwolanin CreditAttribution: pwolanin commentedcan we return an empty string vs NULL?
Comment #62
David_Rothstein CreditAttribution: David_Rothstein commentedLooks like that's just a straight revert to what was there before, so RTBC for now.
Comment #63
Crell CreditAttribution: Crell commentedpwolanin: No, if we just return then debugging the theme system becomes nigh on impossible.
I forgot the return in my last patch. We can add a return so that no addition errors get thrown, but we should not remove the watchdog(). We need *some* form of notification that something screwed up. Silently swallowing errors is not acceptable.
Comment #64
mikey_p CreditAttribution: mikey_p commentedI don't know if this is considered babysitting broken code or not, but if the theme key isn't found, couldn't we return then, as obviously the rest of theme() is going to be horribly broken? i.e. $info is never set, and since there is no hook it ends up assuming a template, and tries to build a template file based on $info['template'] and load it from $info['path'] which results in:
include(DRUPAL_ROOT/.tpl.php): failed to open stream: No such file or directory in theme_render_template()
While it does feel like babysitting broken code it feels even dumber to continue after we checked that something is broken.
Comment #65
mikey_p CreditAttribution: mikey_p commentedI would suggest returning after the watchdog, something like this:
Comment #66
chx CreditAttribution: chx commentedThis really should be optional.
Comment #67
Crell CreditAttribution: Crell commented#65, yes. That's what I should have done in #57. #66 is a waste of time. If I'm calling theme() directly, debugging that I got the name right is dead trivial. The problem comes when theme() is getting called indirectly or 4 layers of recursion down, which is happening more and more and more in Drupal. That's the problem we're trying to solve here. If I could pass in a "fail hard" flag, I wouldn't need one in the first place.
Comment #68
webchickI agree that #66 is the wrong approach, IMO.
Can someone confirm that #65 fixes the errors reported by
drush installcore
? (in the latest HEAD of Drush)Comment #69
moshe weitzman CreditAttribution: moshe weitzman commented#65 does help drush installcore. lets commit it.
after it, we get two harmless errors (i.e. the command works). in order to best fix the first error we need to commit these RTBC patches:
#601548: Loosen the dependency between t() and the theming layer
#599804: Unify page, AJAX 'path', and AJAX 'callback' callbacks
the second error looks like a symptom of a deeper bootstrap bug in the drupal_install_system(). specifically, chaos reigns in system_rebuild_module_data() because the bootstrap modules are not yet loaded like they are supposed to. #606146: DRUPAL_BOOTSTRAP_VARIABLES needlessly loads all bootstrap modules is needed but I have not found the whole solution.
Comment #70
webchickOk, committed to HEAD.
If I'm not mistaken, this effectively gets us back to the original code, with the exception that it'll fire a watchdog error if things go awry. Which is much better than failing silently.
Comment #71
Crell CreditAttribution: Crell commentedwebchick: 99% correct. Along the way we also ferreted out a few cases in core where we had secret undocumented theme-functions-that-weren't, which I think justifies this whole exercise. :-)
Setting to fixed on the assumption that you really did commit it.
Comment #72
webchickOh, right. :) Forgot about that. Yay!
And yep, thanks!
Comment #73
chx CreditAttribution: chx commentedNo, no, no! Why #66 would be a bad approach? This was a feature used by the form API and Views too. Why not allow additional theming if someone picks that up?
Comment #74
Crell CreditAttribution: Crell commentedBecause:
1) Actively hiding the fact that a developer has a bug (which makes fixing it needlessly hard) is itself a bug.
2) FAPI apparently did make use of that loophole... a fact no one seems to have known about because no one I know (except possibly you since you wrote so much of FAPI) knew of the secret theme functions we had defined. And the solution Eaton suggested above, now implemented, removes no functionality at all but is more standard and self-documenting anyway. So there's no regression.
3) The solution in #66 would not have fixed the bug in #1, because theme() is called indirectly in nearly all of the cases where that particular bug occurs. Thus it would have been useless to fix the bug.
Comment #75
eaton CreditAttribution: eaton commentedHold the phone.
chx, just wanting to be clear -- are you saying that Views made use of the 'loose' calls to theme(), iterating through theme functions to see if one was implemented? It did in D5, I know that, but in D6 it moved to the use of pattern-based theme functions, a little-used feature but a more explicit one than this issue.
Does this issue's change break pattern-matched theme functions? If so, we probably need some tests for them. If not, I'm not sure how it's a bad thing at all.
Comment #76
mikey_p CreditAttribution: mikey_p commented@eaton
Specifically it's the method that advanced help uses to place it's icon/links throughout any UI. This came up when porting views during the #d7cx, and I documented the result of it in #64.
I'm staying out of the right/wrong way, but I certainly think a hybrid approach of #65/#66 could work well.
Comment #77
pwolanin CreditAttribution: pwolanin commentedis this fixed or active? Seems like some status cross-posting
Comment #79
sunYes, this had a purpose. For example, consider the following:
There is no registered theme function for comment_form(). Because, by default, the comment form is rendered as-is.
Now consider that you need to override the output of this form.
1) You add
function mytheme_comment_form($form)
to your theme. What happens?2) You add
$form['#theme'] = 'comment_form';
to comment_form(). What happens?3) You add the following to your theme - what happens?
4) You do all of the points above. What happens?
Comment #80
Crell CreditAttribution: Crell commentedI give up, what happens? As of this patch those now have concrete overridable implementations so they no longer rely on the theme system to silently ignore them. They can still be themed. So this is fixed.