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

Files: 
CommentFileSizeAuthor
#27 theme_html_tag-952772-27.patch2.09 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 32,942 pass(es).
[ View ]
#22 theme_html_tag-952772-21.patch2.49 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 32,963 pass(es).
[ View ]
#21 theme_html_tag-952772-21.patch2.49 KBidflood
PASSED: [[SimpleTest]]: [MySQL] 32,952 pass(es).
[ View ]
#16 theme_html_tag-952772-16.patch2.49 KBmdupont
PASSED: [[SimpleTest]]: [MySQL] 32,962 pass(es).
[ View ]
#14 theme_html_tag-952772-14.patch2.18 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 32,970 pass(es).
[ View ]
#13 theme_html_tag-952772-13.patch2.18 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 32,943 pass(es).
[ View ]
#11 theme_html_tag-952772-9.txt2.23 KBbleen18
#10 theme_html_tag-952772-10.patch2.19 KBoriol_e9g
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/simpletest/tests/theme.test.
[ View ]
#9 theme_html_tag-952772-9.patch2.23 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 32,951 pass(es).
[ View ]
#8 theme_html_tag-952772-1.patch825 bytesmdupont
PASSED: [[SimpleTest]]: [MySQL] 32,958 pass(es).
[ View ]
theme_html_tag.patch828 bytesbleen18
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_html_tag.patch. See the log in the details link for more information.
[ View ]

Comments

Issue tags:+Quick fix

tagging

Issue tags:+Novice

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new825 bytes
PASSED: [[SimpleTest]]: [MySQL] 32,958 pass(es).
[ View ]

Another go at a patch.

StatusFileSize
new2.23 KB
PASSED: [[SimpleTest]]: [MySQL] 32,951 pass(es).
[ View ]

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

StatusFileSize
new2.19 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/simpletest/tests/theme.test.
[ View ]

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

StatusFileSize
new2.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.

Status:Needs work» Needs review
StatusFileSize
new2.18 KB
PASSED: [[SimpleTest]]: [MySQL] 32,943 pass(es).
[ View ]

Ops again!

StatusFileSize
new2.18 KB
PASSED: [[SimpleTest]]: [MySQL] 32,970 pass(es).
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new2.49 KB
PASSED: [[SimpleTest]]: [MySQL] 32,962 pass(es).
[ View ]

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

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

@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;
  }
}

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

+/**
+ * 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.

StatusFileSize
new2.49 KB
PASSED: [[SimpleTest]]: [MySQL] 32,952 pass(es).
[ View ]

Simple reroll of #16 with modification proposed in #20

StatusFileSize
new2.49 KB
PASSED: [[SimpleTest]]: [MySQL] 32,963 pass(es).
[ View ]

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

Ops! 2 minuts slow than @idflood :D

I appreciate both your guys. :)

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.

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.

StatusFileSize
new2.09 KB
PASSED: [[SimpleTest]]: [MySQL] 32,942 pass(es).
[ View ]

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

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.

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.

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.