Envision the main tasks here being:
- doctype change.
- html element attribute changes in response to HTML5+RDFa 1.1 requirements.
Assuming the output of $head such as meta charset changes will be covered in separate issues.
| Comment | File | Size | Author |
|---|---|---|---|
| #75 | convert_html-tpl-php_to-HTML5-1077566-75.patch | 675 bytes | scor |
| #69 | convert_html-tpl-php_to-HTML5-1077566-69.patch | 11.55 KB | scor |
| #67 | convert_html-tpl-php_to-HTML5-1077566-67.patch | 11.56 KB | scor |
| #61 | convert_html-tpl-php_to-HTML5-1077566-61.patch | 9.53 KB | scor |
| #60 | convert_html-tpl-php_to-HTML5-1077566-60.patch | 9.48 KB | scor |
Comments
Comment #1
Jeff Burnz commentedRelated #1174756: Convert <head> markup to HTML5
Comment #2
jessebeach commentedsubscribe
Comment #3
jacineWhat we need to do here is change the doctype to HTML5 and stop hard coding the RDFa parts. When the RDF module is disabled, we should just have a plain jane HTML5 doctype. The rest of the file looks okay. This is the code in question:
We could do something similar to what @Jeff Burnz did in contrib, basically making the doctype a variable. See template and preprocess.
Regarding the
<head profile="<?php print $grddl_profile; ?>">, Lin Clark recommends not using the profile attribute:Comment #4
skottler commentedsubscribe
Comment #5
karschsp commentedHere's a patch that basically implements what Jacine outlined in #3.
Comment #7
karschsp commentedHmmm...not sure why the tests failed. It passed locally.
Comment #8
skottler commentedI believe you may need to alter the test suite because it expects the doctype to be of a certain type so changing this causes a failure.
Comment #9
jessebeach commentedskottler, where is the test suite we should look at?
Comment #10
skottler commented@jessebeach, {docroot}/modules/simpletest/tests/
Comment #11
skottler commented@jessebeach, I believe that most of the markup testing and element checking is in common.test.
Comment #12
robloachCan't we have an alter hook in the RDF module to add these variables in? This just seems wrong.
-1 days to next Drupal core point release.
Comment #13
jessebeach commentedAgreed. The
<html>attributes should be printed with drupal_attributes() so modules can modify them. That way the RDf module can push it's attributes to the template without theme.inc knowing anything about it. I'll look at rerolling this.Comment #14
jessebeach commentedI modified @karschsp's patch for this one.
1. The RDF-specific variables are now defined in rdf_preprocess_html instead of template_preprocess_html in theme.inc.
2. I moved attributes from the html.tpl.php file into arrays in preprocess functions so they are printed with drupal_attributes().
3. I deleted the drupal_get_rdf_namespaces function from common.inc. It was used only once in core, to pull a string of namespaces into the html.tpl.php file. This is better achieved through the hook system and attribute arrays in the rdf module so these values can be modified by other modules before rendering.
I ran all the RDF tests on this patch and everything passed.
I'm leaving this as needs work because I'm anticipating some churn on this before it's ready to commit.
Comment #15
robloachLike! Like! Like! I understand leaving it in needs work, but I'd like to see what the testbot says :-).
Should this be $variables['head_attributes'] and $head_attributes instead of $title_attributes?
-2 days to next Drupal core point release.
Comment #16
robloachComment #17
robloachHow do I forget to change the Status twice?
Comment #19
karschsp commentedThis one just cleans up a couple whitespace issues in #14. Still CNW.
Comment #20
Everett Zufelt commentedComments against #19
'...as long as the headers...'
Not part of this patch, but might just as well be corrected.
Agreed that we should create a new $variables]'head_attributes'] = array(); title_attributes is intended to be used for things like node / block / other 'titleish' things, in head this would most appropriately be <title>, which needs no attributes.
+ <body class="<?php print $classes; ?>" <?php print $content_attributes;?>>Classes should go in $variables['content_attributes']['class']; At its best the classes_array should be used for classes on the wrapper, only printed where attributes_array is printed, at its worst classes_array should be purged from Core :)
Comment #21
Jeff Burnz commentedCompletely off topic but +1000 to that!
Sorry, real review coming :)
Comment #22
skottler commentedInitiating the test bot.
Comment #24
karschsp commentedPatch adds a new head_attributes array. Tests pass locally, but lets see what the bot thinks.
Comment #25
Everett Zufelt commentedbots
Comment #27
Everett Zufelt commentedSince head_attributes_array is only used in the html template, it should likely be defined in template_preprocess_html() and not in _template_preprocess_default_variables(). It then then be flattened in template_process_html(). This will still allow rdf_preprocess_html() to add to the array before flattening.
Comment #28
Everett Zufelt commentedx-post
Comment #29
karschsp commentedsomething like this?
Comment #30
Anonymous (not verified) commentedI can take a look at this to see where the RDF test is failing and debug/rewrite the test if necessary.
Just a couple quick style things I noticed with a quick look with Dreditor...
I'm going to leave this at NR though so the test will run.
0 days to next Drupal core point release.
Comment #32
karschsp commentedAttached cleans up the whitespace and \n issues noted by @linclark in #30.
Comment #33
Everett Zufelt commented@@ -2167,7 +2167,8 @@ function template_process(&$variables, $hook) {
// Flatten out classes.
$variables['classes'] = implode(' ', $variables['classes_array']);
- // Flatten out attributes, title_attributes, and content_attributes.
+ // Flatten out attributes, head_attributes, title_attributes, and
+ // content_attributes.
// Because this function can be called very often, and often with empty
* Doc change needs to be removed, no longer touching this func.
+
+ $head_attributes_array = array();
+ $variables['head_attributes'] = $head_attributes_array ? drupal_attributes($head_attributes_array) : '';
* You have this in template_preprocess_html
You should have:
$variables['head_attributes_array'] = array();
and then in template_process_html()
$variables['head_attributes'] = $variables['head_attributes_array'] ? drupal_attributes($variables['head_attributes_array']) : '';
* Essentially we create the array in template_preprocess_html() so that rdf_preprocess_html() or any other preprocess hooks can alter it, and then we flatten in the next step in template_process_html().
+ // GRDDL provides mechanisms for extraction of this RDF content via XSLT
+ // transformation using an associated GRDDL profile.
+ $variables['title_attributes_array'] = array(
+ 'profile' => 'http://www.w3.org/1999/xhtml/vocab',
+ );
* 1. $title_attributes doesn't seem to be used in the template (not sure if you were looking for head_attributes_array, or something else here.
2. By setting the array you are overwritting anything else stored in title_attributes_array, so you need to do something like
$variables['foo']['profile'] = 'http://www.w3.org/1999/xhtml/vocab';
Comment #34
Anonymous (not verified) commentedI don't think we need to add a mention to the head attributes here.
This stuff in rdf_process needs some work.
prefix="dbr: http://dbpedia.org/resource/".Comment #35
robloachAdding the Framework Initiative tag since this helps clean up some of the cruft from the RDFa system that should be in the RDF module itself. I'll hopefully get some time over the coming week to have a closer look.
Comment #36
scor commentedAlthough I'm in the RDFa WG, this is just my personal opinion. RDFa 1.1 encourages the use of the RDFa doctype and @version to help parsers understand what version of RDFa they use for the parsing. But in the case of Drupal, I don't think we should include them. We should keep the doctype neutral, since it is not mandatory to include the RDFa doctype or @version to support RDFa. That's the way I've done it so far in the themes I've built in HTML5/RDFa 1.1.
The @prefix should be there however. A few weeks back Manu and Ivan raised an issue re. charset detection failing on some browsers when the @prefix contains too many characters, but since Drupal declares the UTF8 charset in the HTTP header, this is not an issue for us.
Just to make it clear in the case of several prefix declaration, they should all be in the same prefix attribute (not like the xmlns case) like so:
Comment #37
Jeff Burnz commentedscor, just to be absolutely clear on the doctype, you're saying only use
<!DOCTYPE html>all the time, correct?Comment #38
scor commentedJeff: yes, just the regular doctype you would use in HTML5.
Comment #39
jessebeach commentedThis is related to #1036452: HTML5ify html.tpl.php [Doctype].
Comment #40
jacineTagging for the next sprint.
Comment #41
dcmouyard commentedHow do people feel about using IE conditional comments for the <html> element, like HTML5 Boilerplate?
Comment #42
jacineI actually hate the idea, TBH. I think this is something a theme should decide to do. I don't use this technique personally.
Adding the sprint tags back with the right month this time ;)
Comment #43
jessebeach commentedAgreed that such a tactic (#41) should be used only when required i.e. in a template override for a specific site or project. Using a scoping class at the hmtl root level is killer for performance. Having the classes there, all shiny and pretty, is a temptation all too unavoidable for module developers looking for a quick fix.
It's so sad that we need to expend so much time and energy on one browser.
Thanks for the suggestion @dcmouyard. Keep pinging the queues with these kinds of ideas. I think we're going to be somewhat conservative on what goes into core templates, but that doesn't mean techniques like these shouldn't be considered.
Comment #44
scor commentedI've got a patch coming soon for fixing tests and aligning with RDFa 1.1.
Comment #45
scor commentedThis patch:
- removes the variable doctype and uses a fixed HTML5 doctype instead
- serializes RDF namespace prefix bindings into the prefix attribute according to RDFa 1.1
- removes the head_attributes variable from the html.tpl.php (not needed for RDFa, and I'm not sure we need it at all). there might be some left over in the docs
- fixed the tests to work with the prefix attribute
Comment #46
scor commentedmoving tests from common.test to rdf.test
Comment #47
ericduran commentedCould we name the html attribute html_attributes instead of attributes?
Zen and html5_tools both use html_attributes.
I'm confused as to where content_attributes is coming from. Did I miss something?
Comment #48
Everett Zufelt commented* This comment modification is unnecessary. We should remove all references to head_attributes if not used.
* $variables['attributes_array'] already exists, it is created by template_preprocess() and hooks may have already been fired that modify it, so it is best practice not to overwrite.
+?><!DOCTYPE html>Are we certain that we don't want a variable for this. Makes it easier to modify. Although, I suppose that anyone changing it might likely change the tpl anyway.
+ <body class="<?php print $classes; ?>" <?php print $content_attributes;?>>* class should be a key in content_attributes_array. We shouldn't be using classes_array here.
There actually is nothing ,that I know of, in content_attributes at this point, but it is useful to print it just in case it is altered, the whole point of the array is that it can be altered by any theme hook, that is why it is created by template_preprocess.
Comment #49
Jeff Burnz commentedIs removing or no longer using classes_array's something we are officially doing now, that feels like a follow up issue that we just do everywhere all in one go, if that is what is happening.
Comment #50
Everett Zufelt commented@Jeff
Regardless of what we do with classes_array as a whole, it doesn't belong paired with content_attributes. classes_array is meant, afaik, to be paired with attributes_array. If we are going to keep classes_array, it needs to be implemented consistently.
Comment #51
Jeff Burnz commentedWell, I rather see consistency as being $classes, $title_classes, $content_classes because in reality everyone is abusing attributes arrays to push in classes. I don't think anyone actually wants $title_classes and $content_classes at all, and I personally prefer to keep up the abuse to the point of simply doing everything in attributes arrays in core - pave the cowpaths so to speak.
I can see the point of moving attributes to html element and using content_attributes on the body element, so for consistency we should consider title_attributes for the page title (another patch, another issue, just noting it), however is it just me or does "content_attributes" feel weird on the body, and honestly I prefer Eric Durans suggestion of special casing an html_attributes variable.
I'll shut up now. Great work scor.
Comment #52
jacineI think we need to special case this. I agree
content_attributesis not going to work. We need to be able to count on that being an actual content wrapper intemplate_preprocess()for example. I'm fine with usinghtml_attributeshere if that's what everyone else wants to do. Let's try to move this forward.The
classes_arraydiscussion needs to happen separately. Is there already an issue open for this? If not can someone create one?Comment #53
Anonymous (not verified) commentedThis issue was posted about the classes array #1290694: Provide consistency for attributes and classes arrays provided by template_preprocess().
Also, there is an issue where effulgensia describes classes_array and how you aren't supposed to add classes to the attributes array from 2009, but I can't find it atm... and I don't think that old post is really valid anymore, since there is an example in core that uses an attributes array to place a class.
EDIT: Here is the issue I was talking about #569362: Document $attributes, $title_attributes, and $content_attributes template variables. Thinking about classes arrays seems to have moved on since then, though.
Comment #55
jessebeach commented@linclark, I don't know if I'm totally grokking what you've suggested. Is this summary correct?
1. Stop using classes_array
2. Start using drupal_attributes()
Maybe I'm just reading what I want to read. It seems creating a parallel method of adding an element attribute, i.e. a @class attribute, is getting messy.
Comment #56
Anonymous (not verified) commentedEveret posted the issue, but I agree with him (and with you and Jeff Burnz) that having two ways of adding attributes is too confusing and we should use the attributes array instead.
But in the short term, for the purpose of this patch I think we should continue using classes_array as Jacine suggests. Then we can address the entire classes_array issue as a whole in the other issue, making the change to all the theme functions and templates at the same time.
Comment #57
jessebeach commentedAgreed. Solutions in small steps.
Comment #58
scor commentedThis patch fixes the two first remarks in #48
Exactly, I thought those who need to override the doctype might also override html.tpl.php. that's a fairly advanced use case IMO.
re content_attributes naming, I think folks are talking about different things (or I'm confused!). @ericduran mentioned
html_attributesin the context of the html tag, and I agree with him. so it would like like:what @Everett Zufelt brought up was in the context of the body tag. (I'm ignoring the class issue here). would everyone be happy with
body_attributes? like thisComment #59
Anonymous (not verified) commentedThe patch still has references to $head_attributes even though they aren't used, which Everett mentioned in his review.
I agree with your thinking on html_attributes and body_attributes, how about another patch with those added?
Comment #60
scor commentedremoved remainings of $head_attributes and implemented the html_attributes and body_attributes variables.
Comment #61
scor commentedadded better documentation for template_process_html()
Comment #63
ericduran commented#61: convert_html-tpl-php_to-HTML5-1077566-61.patch queued for re-testing.
Comment #64
ericduran commented#61: convert_html-tpl-php_to-HTML5-1077566-61.patch queued for re-testing.
Comment #65
ericduran commentedAh, this is so much nicer than whats in there now.
We need test for testing the attributes.
Comment #66
ericduran commentedShouldn't this be Implements hook_process_HOOK().
We don't need to check, we could just change it to drupal_attributes($variables['html_attattributes_array']
-19 days to next Drupal core point release.
Comment #67
scor commentedThanks Eric for the review!
I refactored this function so that it makes more sense to be in preprocess, by using an array instead of imploding directly: drupal_attributes() takes care of that. also fixed the second remark.
re tests, there does not seem to be a place where the generic attributes_array, content_attributes_array, etc. are tested, so I created a test of my own for the html.tpl.php attributes.
Comment #68
ericduran commentedThis comment feels out of place to me. Maybe I'm missing something.
I'm still confused if this is the right comment block for this hook. Shouldn't it be hook_preprocess_HOOK.
The rest seems good to me.
-22 days to next Drupal core point release.
Comment #69
scor commentedFixed documentation issues pointed out by @ericduran.
Comment #70
jacineThis looks good to me. Thank you @scor!
Comment #71
catchThis looks great, nice to see the RDF cleanup included. Committed/pushed to 8.x.
Do we have a change notification for any of this yet? It'd be good to get one started up if not.
Comment #72
scor commentedThanks @catch! did you also mean to patch number.module?
Comment #73
webchickOH YEAH!!!!!!!!!!!!!!!!! GREAT work on this folks!!!!!!!
Comment #74
tim.plunkettShouldn't this line come out? Follow-up issue or in here? It's the only remaining mention of grddl left in D8.
Comment #75
scor commentedgood point @tim.plunkett. I'm also removing the $rdf_namespaces from the docs since it's handled up stream now...
Comment #76
tim.plunkettI only caught that because pronouncing GRDDL always made me chuckle. Griddle? Girdle?
Comment #77
catch@scor, that was #1275978: The thousand_separator for numeric fields should default to '' (nothing) instead of ' ' (space). which I'd intended to commit as well last night, but obviously not all at once..
Committed the followup.
Back to active for the change request.
Comment #78
xjmShame on whoever snuck in this comment cleanup. You broke my patch and gave me a merge conflict. :)
Comment #79
ruplChange notice has been created at http://drupal.org/node/1328756
Comment #80
catchComment #81
scor commentedadding tag to track RDFa 1.1 issues.
Comment #82
catchComment #83
gábor hojtsyNoticed this patch did not convert xml:lang to lang on the way (but reading the HTML 5 spec sounds like it should have done that). So opened a followup (now bug) report for this: #1330922: Drupal 8 HTML 5 page template should use lang not xml:lang.
Comment #85
larjohn commented@scor:
this does not validate on W3C validator, because of the "\n"
If you replace it with a regular space, it works.
Is that in par with the specification, or we should report it there?
Comment #86
scor commented@larjohn please file a new issue so we can look into it there.