$variables['element']['#attributes'] is documented (correctly) as being optional, but if you dont include it you get a big fat "Undefined index: #attributes in theme_html_tag()" error.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bleen’s picture

Issue tags: +Quick fix

tagging

bleen’s picture

Issue tags: +Novice
k_zoltan’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed and tested
without applying the patch the error I got:
"Recoverable fatal error: Argument 1 passed to drupal_attributes() must be an array, null given, called in C:\wamp\www\workshop\drupal\includes\theme.inc on line 1896 and defined in drupal_attributes() (line 2257 of C:\wamp\www\workshop\drupal\includes\common.inc)."

After applying the patch the bug disappeared.

catch’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +Needs backport to D7

This could use tests.

pillarsdotnet’s picture

oriol_e9g’s picture

Status: Needs work » Needs review
Issue tags: -Quick fix, -Needs tests, -Novice, -Needs backport to D7

theme_html_tag.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Quick fix, +Needs tests, +Novice, +Needs backport to D7

The last submitted patch, theme_html_tag.patch, failed testing.

mdupont’s picture

Status: Needs work » Needs review
FileSize
825 bytes

Another go at a patch.

oriol_e9g’s picture

Ok! Go for a patch with tests.

With the actual theme_html_tag() function > new theme tests: 12 exceptions
With the patched theme_html_tag() function > new theme tests: 0 exceptions

oriol_e9g’s picture

Ops!! The last patch includes some debug code. This is the correct!

bleen’s picture

FileSize
2.23 KB

same patch as #9 without whitespace issue

Status: Needs review » Needs work

The last submitted patch, theme_html_tag-952772-10.patch, failed testing.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
2.18 KB

Ops again!

oriol_e9g’s picture

Whitespaces!!! And 14 it's a better number than 13 :D

klausi’s picture

Status: Needs review » Needs work
+++ b/modules/simpletest/tests/theme.test
+++ b/modules/simpletest/tests/theme.test
@@ -101,6 +101,27 @@ class ThemeUnitTest extends DrupalWebTestCase {

do not use a fully blown DrupalWebTestCase here, as this can be tested with a DrupalUnitTest equally.

mdupont’s picture

Status: Needs work » Needs review
FileSize
2.49 KB

Trying to use DrupalUnitTestCase for tests... Will it work?

droplet’s picture

+++ b/includes/theme.incundefined
@@ -1902,11 +1902,12 @@ function theme_feed_icon($variables) {
+    return '<' . $element['#tag'] . $attributes . " />\n";
...
+    $output = '<' . $element['#tag'] . $attributes . '>';

can anyone explain why 1st line has \n and 2nd one hasn't.

13 days to next Drupal core point release.

oriol_e9g’s picture

@droplet Because the \n is more down in the code and you cannot see in the patch :D Both finish the tag generation with a \n

function theme_html_tag($variables) {
  $element = $variables['element'];
  $attributes = isset($element['#attributes']) ? drupal_attributes($element['#attributes']) : '';
  if (!isset($element['#value'])) {
    return '<' . $element['#tag'] . $attributes . " />\n"; // <-- SEE HERE!!!!!!
  }
  else {
    $output = '<' . $element['#tag'] . $attributes . '>';
    if (isset($element['#value_prefix'])) {
      $output .= $element['#value_prefix'];
    }
    $output .= $element['#value'];
    if (isset($element['#value_suffix'])) {
      $output .= $element['#value_suffix'];
    }
    $output .= '</' . $element['#tag'] . ">\n"; // <-- SEE HERE!!!!!!
    return $output;
  }
}
mdupont’s picture

Indeed, this is the original behavior of the unpatched code, see http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_html...

Everett Zufelt’s picture

+/**
+ * Unit tests for the theme_html_tag().
+ */

A little picky but 'the' isn't necessary in the above comment.

Other than that this looks good to me.

idflood’s picture

Simple reroll of #16 with modification proposed in #20

oriol_e9g’s picture

Solved #20... maybe RTBC? One more review?

oriol_e9g’s picture

Ops! 2 minuts slow than @idflood :D

droplet’s picture

I appreciate both your guys. :)

bleen’s picture

Status: Needs review » Reviewed & tested by the community

Since the original part of the patch that I wrote was already marked RTBC (#3), and now the tests look nice and neat I'm comfortable marking this RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs review

This is very picky but the theme function was meant to be reserved for tags inside head, not used for p/h2 etc due to performance reasons. Worries me a bit using those as an example in the tests but might just be me.

oriol_e9g’s picture

No problem catch!!! New patch with tests with title and meta tags! Testbot, are you happy?

droplet’s picture

Status: Needs review » Needs work

ah

+++ b/modules/simpletest/tests/theme.testundefined
@@ -354,3 +354,29 @@ class ThemeFastTestCase extends DrupalWebTestCase {
+    $this->assertEqual('<meta name="description" content="Drupal test" />'."\n", theme_html_tag($tag), t('Test auto-closure meta tag generation.'));
...
+    $this->assertEqual('<title>title test</title>'."\n", theme_html_tag($tag), t('Test title tag generation.'));

ahh. just seen it. why theme_html_tag($tag) call in this way ??

it should be

theme('html_tag', $tag).....

12 days to next Drupal core point release.

catch’s picture

Status: Needs work » Reviewed & tested by the community

Using theme_html_tag() directly means we can use real unit tests for this.

theme('html_tag') hits the database, could break if the tested site's theme implemented THEME_html_tag() etc. - in other words that would include a test of the theme system itself no just this function. So it actually makes sense to call it directly here.

Going to mark this back to RTBC, if we had more real unit tests for theme functions then this would not look so strange.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix, -Needs tests, -Novice, -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.