Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

Status: Active » Needs review
FileSize
1.29 KB

And the patch.

Status: Needs review » Needs work

The last submitted patch, 1174756-convert_head_markup_to_html5-1.patch, failed testing.

rupl’s picture

Status: Needs work » Needs review
FileSize
2.19 KB

It seems one of the SimpleTests became inaccurate with the proposed changes. Updated test included.

Status: Needs review » Needs work

The last submitted patch, 1174756-convert_head_markup_to_html5-3.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review

I don't get these fails locally, so let's see if this was a testbot issue.

amateescu’s picture

Issue tags: -html5

Status: Needs review » Needs work
Issue tags: +html5

The last submitted patch, 1174756-convert_head_markup_to_html5-3.patch, failed testing.

iflista’s picture

Subscribing

Jacine’s picture

Issue tags: +HTML5 Sprint: July 2011 - 2

Tagging. Would be nice to make some progress on this one by next week. :)

Anonymous’s picture

I may be able to help debug the test, subscribing for now and hopefully will have a chance later on today to debug.

Anonymous’s picture

Just wanted to confirm that this does fail on my local machine. Seeing if I can debug now.

Anonymous’s picture

+++ b/includes/common.incundefined

@@ -310,8 +310,7 @@ function _drupal_default_html_head() {
     '#type' => 'html_tag',
     '#tag' => 'meta',
     '#attributes' => array(
-      'http-equiv' => 'Content-Type',
-      'content' => 'text/html; charset=utf-8',
+      'charset' => 'utf-8',
     ),

This is the part that is causing the fail.

Anonymous’s picture

It'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

Anonymous’s picture

Status: Needs work » Needs review
FileSize
2.95 KB

This dirty hack should do the trick...

+++ b/modules/simpletest/drupal_web_test_case.php
@@ -1677,7 +1677,7 @@ class DrupalWebTestCase extends DrupalTestCase {
       // DOM can load HTML soup. But, HTML soup can throw warnings, suppress
       // them.
       $htmlDom = new DOMDocument();
-      @$htmlDom->loadHTML($this->drupalGetContent());
+      @$htmlDom->loadHTML('<?xml encoding="UTF-8">' . $this->drupalGetContent());
bleen’s picture

Status: Needs review » Needs work
+++ b/includes/common.incundefined
@@ -4138,9 +4133,7 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
-    '#attributes' => array(
-      'type' => 'text/javascript',
-    ),

Nit-picky, but cant we just get rid of the '#attributes here'

8 days to next Drupal core point release.

Anonymous’s picture

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

bleen’s picture

linclark - sorry, my code snippit got cut off a bit so I think you misunderstood my meaning...

+++ b/includes/common.incundefined
@@ -4138,9 +4133,7 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
     '#value' => '',
-    '#attributes' => array(
-      'type' => 'text/javascript',
-    ),
+    '#attributes' => array(),

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.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
2.92 KB

Oooh, 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...

Status: Needs review » Needs work

The last submitted patch, 1174756-18-convert_head_markup_to_html5.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
2.95 KB

Ok, in checking this manually, we totally need attributes there... otherwise theme_html_tag goes batty.

Reposting the previous patch.

Jacine’s picture

Issue tags: +HTML5 Sprint: August 2011 - 1
Jacine’s picture

Issue tags: -HTML5 Sprint: August 2011 - 1 +HTML5 Sprint: August 2011 - 2

Still needs to be reviewed and tested. Updating to current sprint.

rupl’s picture

+++ b/includes/common.incundefined
@@ -3223,16 +3222,12 @@ function drupal_pre_render_styles($elements) {
   $style_element_defaults = array(
     '#type' => 'html_tag',
     '#tag' => 'style',
-    '#attributes' => array(
-      'type' => 'text/css',
-    ),
   );

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

Jacine’s picture

Assigned: amateescu » Unassigned
Status: Needs review » Needs work
Issue tags: -HTML5 Sprint: July 2011 - 2, -HTML5 Sprint: August 2011 - 2

One minor thing I noticed was that

doesn't retain the empty #attributes array in the same manner as .
Hmm. If we have '#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.
ericduran’s picture

Status: Needs work » Needs review

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

ericduran’s picture

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Novice

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

Jacine’s picture

Issue tags: +sprint

Tagging this for the next sprint.

karschsp’s picture

Status: Needs work » Needs review
FileSize
3 KB

Here's a re-roll for the /core change.

Status: Needs review » Needs work
Issue tags: -Novice, -html5, -sprint

The last submitted patch, 1174756-14-convert_head_markup_to_html5_29.patch, failed testing.

karschsp’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Novice, +html5, +sprint

The last submitted patch, 1174756-14-convert_head_markup_to_html5_29.patch, failed testing.

xjm’s picture

Can you verify whether this patch applies locally? Testbot is unable to apply it.

karschsp’s picture

Status: Needs work » Needs review

It didn't. Here's a re-roll that should apply cleanly.

karschsp’s picture

Status: Needs review » Needs work
FileSize
2.59 KB

Yeah, and the patch. TGIF!

rupl’s picture

Status: Needs work » Needs review

Setting to needs review for testbot.

cosmicdreams’s picture

testbot liked it, is this ready to test?

xjm’s picture

Edit: Editing comments is cheating. ;) I'd say test away!

cosmicdreams’s picture

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

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

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

chx’s picture

Whoa, what.

-      'http-equiv' => 'Content-Type',
-      'content' => 'text/html; charset=utf-8',

why? (http://diveintohtml5.org/semantics.html#encoding is not available)

amateescu’s picture

Because of the line that follows those deletions :)

+++ b/core/includes/common.inc
@@ -299,8 +299,7 @@ function _drupal_default_html_head() {
-      'http-equiv' => 'Content-Type',
-      'content' => 'text/html; charset=utf-8',
+      'charset' => 'utf-8',

http://www.w3.org/TR/html5-diff/#syntax

rupl’s picture

@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" />

cosmicdreams’s picture

Good. Then all concerns have been addressed?

chx’s picture

Status: Reviewed & tested by the community » Needs work

Absolutely 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?

catch’s picture

Issue tags: +Needs manual testing
catch’s picture

Issue tags: -Needs manual testing

redacting following discussion in irc.

chx’s picture

Status: Needs work » Reviewed & tested by the community

It'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.

cosmicdreams’s picture

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

Jacine’s picture

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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

Jacine’s picture