Theme system should optionally report when a theme key is not found

Crell:

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?

Crell:

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).

Watchdog and status messages are user-facing (and translatable) text and should therefore employ full, properly capitalized sentences.

@Arancaytar: good point.

Let'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? :)

Yeah. 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.

I 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.

So, 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?

Crell, 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?

Seems like this is really only a developer issue?

I'd still prefer watchdog and return an empty string (not NULL as it does now).

Absolutely 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.

However, theme(form_id) is checked against the theme registry first and that should be factored out and use for system_settings_form:

form_set_theme(&$element, $theme_key) {
  drupal_theme_initialize();
  $registry = theme_get_registry();
  if (isset($registry[$theme_key])) {
    $element['#theme'] = $theme_key;
  }
}

Or 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.

function drupal_render(&$elements) {
  static $defaults, $theme_registry;
  if (!isset($theme_registry)) {
    drupal_theme_initialize();
    $theme_registry = theme_get_registry();
  }
...
  if (isset($elements['#theme']) && isset($theme_registry[$elements['#theme']])) { // the part after the && is new

Last 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.

Last 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.

My 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?

We 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.

Fair point. If it's gotten the signoff, I'm actually totally fine with an exception being used.

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?

#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?

.

Regardless 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.

Regardless 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.

Be it.

I 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:

Trivial re-roll. Back to RTBC.

Committed to CVS HEAD. Thanks.

This seems to have broken command-line installs? (oh, I can't wait until we have tests for that...):

  <li><em>ThemeHookNotFoundException</em>: <em>Theme hook &quot;placeholder&quot; not found.</em> in <em>theme()</em> (line <em>771</em> of <em>/home/droth/web/drupal/includes/theme.inc</em>).</li>

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.

Please 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.

Patch no longer applies when reverting - needs a re-roll.

This 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.

This 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.

can we return an empty string vs NULL?

Looks like that's just a straight revert to what was there before, so RTBC for now.

I 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.

I would suggest returning after the watchdog, something like this:

This really should be optional.

I 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)

#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.

Ok, 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.

Oh, right. :) Forgot about that. Yay!

And yep, thanks!

No, 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?

Hold 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.

@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.

is this fixed or active? Seems like some status cross-posting

So 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...

Yes, 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?

function mytheme_theme() {
  $theme['comment_form'] = array(
    'render element' => 'form',
  );
  return $theme;
}

4) You do all of the points above. What happens?