$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.
Comment | File | Size | Author |
---|---|---|---|
#27 | theme_html_tag-952772-27.patch | 2.09 KB | oriol_e9g |
#22 | theme_html_tag-952772-21.patch | 2.49 KB | oriol_e9g |
#21 | theme_html_tag-952772-21.patch | 2.49 KB | idflood |
#16 | theme_html_tag-952772-16.patch | 2.49 KB | mdupont |
#14 | theme_html_tag-952772-14.patch | 2.18 KB | oriol_e9g |
Comments
Comment #1
bleen CreditAttribution: bleen commentedtagging
Comment #2
bleen CreditAttribution: bleen commentedComment #3
k_zoltan CreditAttribution: k_zoltan commentedReviewed 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.
Comment #4
catchThis could use tests.
Comment #5
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #6
oriol_e9gtheme_html_tag.patch queued for re-testing.
Comment #8
mdupontAnother go at a patch.
Comment #9
oriol_e9gOk! 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
Comment #10
oriol_e9gOps!! The last patch includes some debug code. This is the correct!
Comment #11
bleen CreditAttribution: bleen commentedsame patch as #9 without whitespace issue
Comment #13
oriol_e9gOps again!
Comment #14
oriol_e9gWhitespaces!!! And 14 it's a better number than 13 :D
Comment #15
klausido not use a fully blown DrupalWebTestCase here, as this can be tested with a DrupalUnitTest equally.
Comment #16
mdupontTrying to use DrupalUnitTestCase for tests... Will it work?
Comment #17
droplet CreditAttribution: droplet commentedcan anyone explain why 1st line has \n and 2nd one hasn't.
13 days to next Drupal core point release.
Comment #18
oriol_e9g@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
Comment #19
mdupontIndeed, this is the original behavior of the unpatched code, see http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_html...
Comment #20
Everett Zufelt CreditAttribution: Everett Zufelt commented+/**
+ * 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.
Comment #21
idflood CreditAttribution: idflood commentedSimple reroll of #16 with modification proposed in #20
Comment #22
oriol_e9gSolved #20... maybe RTBC? One more review?
Comment #23
oriol_e9gOps! 2 minuts slow than @idflood :D
Comment #24
droplet CreditAttribution: droplet commentedI appreciate both your guys. :)
Comment #25
bleen CreditAttribution: bleen commentedSince 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.
Comment #26
catchThis 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.
Comment #27
oriol_e9gNo problem catch!!! New patch with tests with title and meta tags! Testbot, are you happy?
Comment #28
droplet CreditAttribution: droplet commentedah
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.
Comment #29
catchUsing 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.
Comment #30
Dries CreditAttribution: Dries commentedCommitted to 7.x and 8.x. Thanks.