Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Easy stuff to do here:
1) get rid of the type
attribute from <script>, <style> and <link> tags.
2) <meta http-equiv="content-type" content="text/html; charset=UTF-8"> should be: <meta charset="utf-8" />
Resources:
http://dev.w3.org/html5/spec/Overview.html#attr-script-type
http://dev.w3.org/html5/spec/Overview.html#attr-style-type
http://dev.w3.org/html5/spec/Overview.html#attr-link-type
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedAnd the patch.
Comment #3
ruplIt seems one of the SimpleTests became inaccurate with the proposed changes. Updated test included.
Comment #5
amateescu CreditAttribution: amateescu commentedI don't get these fails locally, so let's see if this was a testbot issue.
Comment #6
amateescu CreditAttribution: amateescu commented#3: 1174756-convert_head_markup_to_html5-3.patch queued for re-testing.
Comment #8
iflista CreditAttribution: iflista commentedSubscribing
Comment #9
JacineTagging. Would be nice to make some progress on this one by next week. :)
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedI may be able to help debug the test, subscribing for now and hopefully will have a chance later on today to debug.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedJust wanted to confirm that this does fail on my local machine. Seeing if I can debug now.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedThis is the part that is causing the fail.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedIt's failing on Español.
I'm guessing that this is an issue because xpath can't figure out what the character encoding is and is getting mixed up on the special character. DrupalWebTestCase uses PHP's DOMDocument to handle the HTML of the page. It seems that at least some versions of PHP didn't have parsing rules for HTML5 and don't read the
charset
attribute as containing the character encoding (this is probably why it worked on your machine amateescu, you probably have a newer version of PHP installed).If I'm on the right track, then there is some discussion of this issue at http://ie.php.net/manual/en/domdocument.loadhtml.php
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedThis dirty hack should do the trick...
Comment #15
bleen CreditAttribution: bleen commentedNit-picky, but cant we just get rid of the '#attributes here'
8 days to next Drupal core point release.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedI have no strong opinion on the issue. Most user agents already default to "text/javascript" if it isn't explicitly stated, but there could be some that still don't. In light of this, I would lean towards leaving it in, since it doesn't harm anything, but no real preference.
Comment #17
bleen CreditAttribution: bleen commentedlinclark - sorry, my code snippit got cut off a bit so I think you misunderstood my meaning...
I was not suggesting keeping the "type" attribute, I just wasnt sure if we needed to set "#attributes" equal to an empty array. I'm simply suggesting that we remove the attributes as the patch already does without adding the new line seen here:
'#attributes' => array();
7 days to next Drupal core point release.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedOooh, so we already don't do what I was leaning towards....
Yeah, I would think you are correct, no reason to set empty attributes. Rerolling...
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedOk, in checking this manually, we totally need attributes there... otherwise theme_html_tag goes batty.
Reposting the previous patch.
Comment #21
JacineComment #22
JacineStill needs to be reviewed and tested. Updating to current sprint.
Comment #23
ruplNo problems on my local environment! One minor thing I noticed was that
<style>
doesn't retain the empty #attributes array in the same manner as<script>
.Comment #24
Jacine'#attributes' => array()
for the script, then why would we omit it for style tags? Maybe I'm missing something. Setting to needs work for that, until I hear back. Also, un-assigning this from amateescu (feel free to reassign yourself if you're going to work on it) and cleaning up tags.Comment #25
ericduran CreditAttribution: ericduran commented@Jacine is because the style tags has the rel attributes. The script tag technically should be empty but since the html_tag element requires an attributes declaration the empty array() was added.
Switching to needs-review as per this is what was blocking it before.
I didn't actually test this patch.
Comment #26
ericduran CreditAttribution: ericduran commented#20: 1174756-14-convert_head_markup_to_html5.patch queued for re-testing.
Comment #27
xjmNote that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."
Tagging as novice for the task of rerolling the Drupal 8.x patch.
If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.
Comment #28
JacineTagging this for the next sprint.
Comment #29
karschsp CreditAttribution: karschsp commentedHere's a re-roll for the /core change.
Comment #31
karschsp CreditAttribution: karschsp commented#29: 1174756-14-convert_head_markup_to_html5_29.patch queued for re-testing.
Comment #33
xjmCan you verify whether this patch applies locally? Testbot is unable to apply it.
Comment #34
karschsp CreditAttribution: karschsp commentedIt didn't. Here's a re-roll that should apply cleanly.
Comment #35
karschsp CreditAttribution: karschsp commentedYeah, and the patch. TGIF!
Comment #36
ruplSetting to needs review for testbot.
Comment #37
cosmicdreams CreditAttribution: cosmicdreams commentedtestbot liked it, is this ready to test?
Comment #38
xjmEdit: Editing comments is cheating. ;) I'd say test away!
Comment #39
cosmicdreams CreditAttribution: cosmicdreams commentedManually tested,
Everything applies, everything looks good when viewing source code.
This in addition to the unit tests should be enough to mark this a RTBC.
Comment #40
cosmicdreams CreditAttribution: cosmicdreams commentedPerhaps I'm jumping the gun with promoting this issue to RTBC but it looks pretty simple and pretty clean. And I was able to apply the patch and test it manually.
Comment #41
chx CreditAttribution: chx commentedWhoa, what.
why? (http://diveintohtml5.org/semantics.html#encoding is not available)
Comment #42
amateescu CreditAttribution: amateescu commentedBecause of the line that follows those deletions :)
http://www.w3.org/TR/html5-diff/#syntax
Comment #43
rupl@chx that page has been relocated to this URL: http://diveintohtml5.info/semantics.html#encoding
Several paragraphs in, it provides as an example of a valid HTML5 charset meta tag, which the current patch produces:
<meta charset="utf-8" />
Comment #44
cosmicdreams CreditAttribution: cosmicdreams commentedGood. Then all concerns have been addressed?
Comment #45
chx CreditAttribution: chx commentedAbsolutely not. The link @rupl provided says "Both of these techniques still work in HTML5. " meaning the HTTP header and the http-equiv meta we used so far. Did anyone test this with IE? This is not some "meh, it will look garbled, but who cares" issue. If a page has only ASCII data and so no way for IE to detect a charset and the user provides some messed up Windows charset are we going to put broken data into the database? What's the point of the change if we know the old one will still work and basically no idea (at least not in the issue) whether it can lead to serious problems under IE?
Comment #46
catchComment #47
catchredacting following discussion in irc.
Comment #48
chx CreditAttribution: chx commentedIt's completely superflous anyways, turns out http://api.drupal.org/api/function/drupal_deliver_html_page/7 sends the HTTP header that the meta provides an equivalent of. Yes, we could go on and on about broken HTTP proxies and whatnot but let's not and go ahead with this. It's just a safety belt on top of a harness anyways.
Comment #49
cosmicdreams CreditAttribution: cosmicdreams commented@chx, let me know which manual tests you'd like to have done for this patch and I'll report back what I find.
I have the full gambit of browsers that I can send them through. I tested this patch with all of them and didn't see anything wrong at first glance.
Comment #50
Jacine@cosmicdreams No testing is needed (the tag was added and then removed). The issue was marked back to RTBC after a discussion in IRC. It has been and still is ready, so we're just waiting for a commit.
Comment #51
catchCommitted/pushed to 8.x, thanks!
Comment #53
JacineHere's a related issue: #1362974: Remove type="text/javascript" from <script> tags.