Loosen the dependency between t() and the theming layer

Does this ever need to be themeable? Anyway, this is an improvement.

Hm. I'm not sure if this adequately covers all of our bases. For example, I had an awful lot of fun debugging this problem in #424486-6: Errors when adding external table with unknown column types, which was caused by db_set_active()ing to another database that did not have a system table from which to find the current theme, and then causing watchdog() error with a % placeholder in it.

I'm not suggesting this is the "fix everything that was ever wrong with t() and theme()" issue, but I wonder if there's a way to generalize the fix more than merely checking if we're at full bootstrap, since that wouldn't cover this case, at least afaik.

The fix itself is rather 'meh', though I don't see what other choice we have... but it'll cause inconsistent results if the theme is overriding theme_placeholder to be blinking marquee tags, for example (and whose isn't?)

Also, as per #592008: Don't save theme registry before modules are included, the theme system now works much earlier in the page request than a full bootstrap, so the patch is a bit out of date...

#3 - I do think that messing with t() is beyond the scope we can afford right now.

#4 - That patch is unfortunately broken and at best we will be building a partial theme registry and tossing it in that issue for early theme calls. It is just not worth it here.

Let me throw something out: do we really want a theme_placeholder? Life would be easier (and faster) if we just had a non-themable drupal_placeholder() function (i.e. check_plain()). When I take a critic look at theme_placeholder() it feels a bit silly to me.

Give the em element generated by drupal_placeholder a class, and then themers can still make it do pretty much whatever they like.

Implemented suggestions from #6 and #7. Thanks guys. I'm happy to see this themeable function go. I put the new drupal_placeholder() in bootstrap.inc since some early code might want it and it is only 3 lines of php.

This looks RTBC to me. Marking it as such so people can review it if they want to.

Committed to CVS HEAD. Thanks.

Added to upgrade docs at http://drupal.org/update/modules/6/7#placeholder

I'm afraid this patch has introduced 16 failures in DBLogTestCase.

#666266: HEAD is broken - various test failures

It's a funny story. Here's what happens.

See, when we look at admin/reports/events, the messages are truncated. The test case compensates for this by only checking for a certain portion of the message.

This message contains a placeholder.

The markup for placeholders just got longer by the length of class="placeholder".

This causes it to be truncated earlier.

This causes the test case's hard-coded string portion not to match the page any longer.

=> Fail.

This should be fixed by

1.) adding a truncate_utf8($message, 56, TRUE, TRUE) to the test case.
2.) making truncate_utf8 ignore HTML markup (at least when $wordsafe is set).

The former will fix the test, but I believe the latter will make the events overview more useful.

Edit: Though the latter really goes beyond the scope of this bug. Let's just fix the test failure.

This patch lets dblog.test pass. All is well. :)

That's an evil, evil test we've got there. Nice sleuthing.

Can we put that logic into its own private assertMessageFound() function so we don't have to maintain 10 different lines of truncate_ut8() and when this changes in the future we only have to modify it in one place?

That sounds like a good idea.

And let's call it assertLogMessage().. we don't call it assertTextFound(), just assertText().

Yeah, I just typed what came to mind. :)

not easy patching with a wailing baby

I'm not able to run the tests here locally, cos they hang forever.

When I kill execution I get an AJAX error, and then the results page says 352 passes, 5 fails, and 0 exceptions
The worst seems to be:
The test did not complete due to a fatal error. Completion check dblog.test 404 DBLogTestCase->testFilter()

I get those all the time, which is why I only ever run tests via the scripts/run_tests.sh script now. I'll check if the same thing happens for me in the console script...

I'm afraid I can't reproduce the breakage you're getting, webchick... the tests run through in 1:24 minutes for me both in console and in web mode; 414 passes, no fails. Can you run any of the other test cases, and do you get the same problems when running run_tests.sh?

(/me goes to irc to ask for more people with working test environments to give this a try.)

I confirm the (#28) fails of dblog-truncate-messages-test-601548-22_1.patch (#24)

Looks like more sleuthing. :)

Thanks for the team work on this. Committed to HEAD, with a comment above that weird-ass truncate_utf8() line and a todo to fix it properly by checking the database rather than HTML output.

Let's see if we can turn testing bot back on now.

working again
http://qa.drupal.org/pifr/status

Back to the original issue.

I don't think I understand what bug this patch actually fixed? During the bootstrap, we load includes/common.inc and includes/theme.inc one right after the other, which means that as soon as the t() function is available to be used, the theme system is as well.

Can anyone point to a specific scenario that didn't work before this patch but now works after it? (In Drupal 7, I mean - I know there were tons of problems with this in Drupal 6, but the code changed significantly.)

If there isn't in fact a bug, I think we should reconsider this. It breaks the common pattern where theme() functions are used for HTML output, and therefore makes the surrounding code harder to understand.

Also, I found two problems with the patch:

• There is now an inconsistency between the PHP code and JavaScript, which still uses Drupal.theme('placeholder') and also outputs <em> without a 'placeholder' class.
• There is still a definition for 'placeholder' in hook_theme() .... in particular, in drupal_common_theme().

Those 2 inconsistencies should be fixed. I can take that next week if noone else does. Having me fiddle with javascript is not so advisable though.

We want the error handler to be moved earlier in the bootstrap. This is a step toward that goal. See #325169: Move error/exception handler higher up in the bootstrap process.

Lastly, at least a few of us agreed that hardly anyone ever wanted to theme this so invoking the theme system is useless overhead.

This patch also introduced a bit of ridiculousness. WHY, why why, would you want to pass in an array of variables if we're not passing through the theme layer? The whole mess of passing in variable arrays into theme functions was so they could be preprocessed. If we're not using the theme layer, there's no place to change/extend these variables. And even if you could, you can't override drupal_placeholder(), so these variables would never be used anyway. This should be executed as drupal_placeholder($text) not drupal_placeholder(array('text' => $text)).

agreed. patches welcome. the signature of drupal_placeholder was not really the point of the patch so it was done in a fast way.

+

I think this is all we need for the remaining cleanup. It seems that drupal_placeholder() was changed from taking an array to a string a long time ago, so this just contains the other cleanups mentioned above.

I left Drupal.theme('placeholder') as a "theme function" in the JavaScript since it seems too late in the game to change that, but I did make its output have the placeholder class so it now matches the HTML you get when you call drupal_placeholder() in PHP.

makes sense.

#41: drupal-placeholder-cleanup-601548-41.patch queued for re-testing.

Committed to HEAD. Thanks!

Lest someone else see those - signs next to theme_placeholder in drupal_common_theme() and freak out like I did ;), the original patch removed this function long ago, and so this is just cleaning up the fact that it doesn't exist.