$t not defined in form_pre_render_conditional_form_element()

This needs to use

$t = get_t();

typically done as first line of the function.

No need for a comment, so #5 is RTBC.

This seems to have spotted a hole in our test suite. Could we adjust the tests so this function gets hit?

The test does not seem to fully trigger the error for some reason. I don't see a fatal error in the testbot results?

Trying to invoke a function name in a undefined string should yield a fatal error:

> php -r "print $t('foo'); print 'bar';"

Notice: Undefined variable: t in Command line code on line 1

Call Stack:
    0.0003     317704   1. {main}() Command line code:0

Fatal error: Function name must be a string in Command line code on line 1

Call Stack:
    0.0003     317704   1. {main}() Command line code:0

Please disregard #11, we see the PHP notice in the tests. The fatal error happens in the child site. We'd only see the fatal error if the test itself would attempt to render the form element in the parent site.

That patch was rolled back, so we should be good to go here.

Yep. And #1475666: Rollback - PHP error because of typo in form.inc line 3025 was major. (Not sure if it should have been.)

Lets get this done ... here's a review:

+++ b/core/includes/form.incundefined
@@ -2753,6 +2753,9 @@ function theme_radios($variables) {
+  if (!empty($element['#attributes']['title'])) {

Shouldn't we use isset() here? Otherwise '0' couldn't be used as a title, could it?

+++ b/core/includes/form.incundefined
@@ -3014,6 +3017,9 @@ function theme_checkboxes($variables) {
+  if (!empty($element['#attributes']['title'])) {

Same here. We also wouldn't need the negation.

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -664,6 +664,12 @@ class FormsElementsLabelsTestCase extends DrupalWebTestCase {
+    $this->assertEqual($elements[0]['title'], t('Checkboxes test') . ' (' . t('Required') . ')', t('Title attribute found.'));

The assertion message shouldn't have a t().

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -664,6 +664,12 @@ class FormsElementsLabelsTestCase extends DrupalWebTestCase {
+    $this->assertEqual($elements[0]['title'], t('Radios test') . ' (' . t('Required') . ')', t('Title attribute found.'));

Neither this one. (Yes, t() in assertion messages are all around, but cleaning that up is another issue.)

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -752,10 +752,31 @@ function form_label_test_form() {
-  // Textfield test for title set not to display
+  // Textfield test for title set not to display.

Baby-kitten hunk. (Just saying. If it's here anyway, let's get it in.)

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -752,10 +752,31 @@ function form_label_test_form() {
+    '#title' => t('Checkboxes test'),
+    '#options' => array(
+      'first-checkbox' => t('First checkbox'),
+      'second-checkbox' => t('Second checkbox'),

t() in a test again. The rule is not to introduce any *new* translatable strings. So simply remove them.

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -752,10 +752,31 @@ function form_label_test_form() {
+    '#title' => t('Radios test'),
+    '#options' => array(
+      'first-radio' => t('First radio'),
+      'second-radio' => t('Second radio'),

Here, as well.

I don't agree with 4 - very minor point, though, as xjm said. Looks good otherwise. Thank you @andypost!

It introduces no new t()'s because some wrong t()'s are already there. That doesn't count :P

Actually it's about decoupling and stuff. Before were discussing "must we really do this" over and over, here is a slightly adjusted patch. No other changes than this minor stuff.

@andypost asked me to comment on this issue. It's a little messy but I don't think it's that bad, and honestly I can't think of a better way. This patch looks ready to me.

Edit: Didn't mean to take the backport tag off; not sure why that happened.

To clarify, core already has an inconsistent mix of using t() and not in test modules, so I don't think it's worth holding the patch back one way or the other. There are arguments both for and against in this case, but at the end of the day it really doesn't make much difference, and I am not comfortable saying it should be one way or another because there isn't consensus. Edit: we have a brief discussion of this point at http://drupal.org/simpletest-tutorial-drupal7#t and the issue to bikeshed about it in is #500866: [META] remove t() from assert message.

The patch is also fine to backport as-is.

Looks sane to me. Thanks a bunch for the tests!

Committed and pushed to 8.x and 7.x.