Problem/Motivation
HTML5 has the <time>
element for marking up dates and times. When datetimes are annotated this way, it makes it easier to build jQuery scripts and applications that do cool things. We want to output all of our $submitted variables (and other datetimes) with this element.
The <time>
element has a datetime attribute which provides a machine processable version of the date. This can be given in a number of different formats. For example:
<time datetime="2011-12">Decemeber 2011</time>
<time datetime="01-01">New Years Day</time>
Proposed resolution
Create a theme_datetime function that takes:
- A 'text' variable to use between the
<time>
tags. This ischeck_plain
ed by default, but a flag can be set to let it pass through in case it contains HTML. - Either a formated date string to use for the datetime attribute, or a timestamp which is processed into the full datetime string
Remaining tasks
Patch needs review. A notable change is the introduction of new built-in datetime formats.
Original report by Everett Zufelt
Split from #1094324: Add new theme_functions for consistency - Discussion to add a theme_datetime() function.
From @Jeff Burnz in #1094324-1: Add new theme_functions for consistency - Discussion
The first one that really strikes me is our formatting of dates and times.
Our current use of times and dates is rather inconsistent in core, for example lets compare just a couple:
Node:
$variables['date'] = format_date($node->created);
Eventually this is printed in node tpl via $submitted and no markup is added anywhere - its naked and unless RDFa is enabled this carries zero semantics.Aggregator:
$variables['feed_age'] = t('%age old', array('%age' => format_interval(REQUEST_TIME - $item->timestamp)));
$feed_age is printed in aggregator-summary-item.tpl.php wrapped in span's with a class: <span class="age"><?php print $feed_age; ?></span>There are quite a few other instances of this sort of inconstancy in aggregator, and of course comment tpl does essentially the same thing as node.
I think we have a use case here for a theme_datetime() function. This would provide a simple wrapper with attributes. This will allow much easier global
override of markup for dates and times, for example if we want to use XHTML we can use a span, if we want to use HTML5 we can use time + the time attributes
such as pubdate, datetime etc.In HTML5 we can use the <time> element.
Here's a bit of a write up about it
and if you're really hard core
read the spec.
Comment | File | Size | Author |
---|---|---|---|
#209 | drupal8.datetime.209.patch | 9.22 KB | sun |
#196 | drupal8.datetime.196.patch | 10.64 KB | sun |
#191 | drupal8.datetime.191.patch | 10.03 KB | sun |
#188 | 1183250-theme_datetime-188.patch | 9.48 KB | aspilicious |
#186 | 1183250-theme_datetime-186.patch | 9.7 KB | aspilicious |
Comments
Comment #1
Everett Zufelt CreditAttribution: Everett Zufelt commentedPrototype
Comment #2
scor CreditAttribution: scor commenteddon't we want to specify a time zone in the datetime attribute? or is $variables['timestamp'] a UTC date? The 'c' date format character will generate an ISO 8601 date.
Powered by Dreditor.
Comment #3
Everett Zufelt CreditAttribution: Everett Zufelt commented@scor
$variables['timestamp'] should be a unix timestamp.
I am open to suggestions for better formatting of the datetime attribute. The datetime attribute must be:
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedComing in late on this, but is there a reason we wouldn't want to use date_iso8601() as RDFa does?
HTML5 doctor lists that kind of formatting as one of the valid ones.
For example, here is the code from HTML5 doctor:
And here is the code from a page that uses date_iso8601:
The only difference I see is that date_iso8601 includes seconds.
Comment #5
scor CreditAttribution: scor commentedyes, I suggested that to remain in line with what we use in D7, namely date_iso8601(). Uploading patch to reflect that.
Other fixes:
- reordered parameters in drupal_common_theme() to be in line with the order in the docs
- capitalized constants NULL and FALSE
- fixed doxygen
Comment #6
jessebeach CreditAttribution: jessebeach commentedIt's succinct and clear. I say we set to RTBC. I'd love to start using this in the template update work.
Comment #7
JacineHey guys, wouldn't it make sense to have a generic 'attributes' variable here, so that if some random module out there is using it, they can plug classes or id's in, if needed? I don't think we should populate any of those by default, but I think it'd be good to have, because who knows what it will end up being used for.
Thoughts?
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedYeah, having a generic attributes definitely makes sense here... you never know what attributes you're going to need to insert 3 years down the road (or which attributes someone else is going to need to insert tomorrow).
I had a big problem with theme_image_formatter because it doesn't pass attributes through to theme_image. This makes it hard to place RDFa as Google wants it for Rich Snippets.
So yeah, attributes++
Comment #9
jessebeach CreditAttribution: jessebeach commentedAttributes are plumbed through in the updated patch.
As I was working on #1189804: Convert aggregator-feed-source.html.twig to HTML5, I realized that theme_datetime needs to be more robust in terms of how it displays the time element text. One might simply want a well-formatted timestamp with format_date. Just as likely, one might also want a well-formatted time interval with format_interval, as we display for feed source update times.
But once you give a mouse that cookie, he's going to want labels. So I rigged up some default labels (e.g. "ago" and "to go") and plumbed overrides through in the $variables array.
And the feed source timestamp can also be a link, so I added an href property to the $variables array that let's you print the time element text as a link. So, I qualified my RTBC with the desire to get this theme function into template update work and it turns out that once I did, I realized it needed a lot more options.
Comment #10
aspilicious CreditAttribution: aspilicious commentedtrailing whitespace
trailing whitespace
trailing whitespace
trailing whitespace
20 days to next Drupal core point release.
Minor but it has to be fixed
Comment #11
Everett Zufelt CreditAttribution: Everett Zufelt commentedThis is looking really good. I didn't catch up with all of the comments, but I wanted to note that the datetime attribute does not call for a iso8601 format. I'm not sure if this has been considered and dealt with in the current patch.
The spec states:
Comment #12
jessebeach CreditAttribution: jessebeach commented@aspilicious, had you meant to post an updated patch?
Comment #13
jessebeach CreditAttribution: jessebeach commented@Everett Zufelt, I think iso8601 is acceptable here. From the doc you linked to:
Comment #14
Everett Zufelt CreditAttribution: Everett Zufelt commented@jessebeach
After quickly looking at http://tools.ietf.org/html/rfc3339#section-5.6 it appears that the date('c', $date) formatter is an acceptable value for the datetime attribute. Thanks for pointing this out.
I'd still be happy for someone else to take a quick look at that RFC just to be on the safe side.
Comment #15
Jeff Burnz CreditAttribution: Jeff Burnz commentedI looked into this a bit myself and found some interesting stuff.
http://en.wikipedia.org/wiki/ISO_8601 shows an example of ISO 8601 with date time, however this looks different to date times generated using PHP "c".
I did some tests:
Produces:
The sitepoint variant is the only one the I can see that matches the examples given on Wikipedia (I may have missed something?).
Wonder if this is a problem, I don't know nearly enough about these things tbh.
Comment #16
JacineHey guys, maybe this is a dumb question (so I apologize in advance if it is) but can we use or improve format_interval() for this?
http://api.drupal.org/api/function/format_interval/7
Comment #17
jessebeach CreditAttribution: jessebeach commentedRemoved whitespace as noted by aspilicious.
No other changes from the v6 patch.
Comment #18
jessebeach CreditAttribution: jessebeach commentedjacine, the updated patch does use format_interval(). Were you thinking we use it in a different way?
Comment #19
jessebeach CreditAttribution: jessebeach commentedJeff, my understanding of Z is that it represents Zulu time, or -00:00 i.e. Greenwich Mean Time. Rather than produce a server time with a UTC offset, it just prints out the GMT timestamp for the Unix time number.
Now that I'm thinking about this more, one would want gmdate() for the datetime attribute (so machines always get the same number) and format_date()/format_interval() for humans so they see the timestamp in the context of their timezone.
Am I thinking about this right?
What we're outputing here is an html document that is going to get crawled and indexed. We want an attribute timestamp value that doesn't fluctuate with the location of the server that requested the document so the indexed document has a constant datetime value. The time element contents can vary because they're intended for humans to use.
I updated the date_iso8601() function to gmdate() in this patch. No other changes.
Comment #20
JacineOy, I'm so sorry, Jesse! Jeez. I was looking at the patch before, and looking specifically for format_interval() and totally missed it. I think I'm loosing it. :/
Plz don't shoot me for being so nit picky, but I think pubdate and datetime should go into $variables['attributes'], like the way it's done in theme_image().
One other thing I'm not sure about here, is if there are any variables in there (like $past_label, $future_label, $human_readable) that we would want to modify or add to, in a preprocess function. If so, they'd be better off in $variables so we can. And... certain patch reviewers (not me) are gonna nitpick because the comments don't have periods at the end of the sentences.
/me ducks and runs away now.
Comment #21
jessebeach CreditAttribution: jessebeach commentedNot nit picky at all. I feel like a theme function should get a thorough going-through.
I moved pubdate to the attributes array - that seems clear.
timestamp is a little different. Yes, the timestamp is used to provide a value for the datetime attribute. But it's also used to derive the human-readable text in the time element. I'm conflicted here. The timestamp variable feels more primary than a property of the attributes array. Anyone else have an opinion? I could go either way, but I'm leaning to leave this outside of attributes.
All labels are properties of $variables['labels'], so they're available to preprocess functions. I tested these all out to make sure they're overridable. Nothing frustrates me more than hard-coded text!
Comments end in a period.
Comment #22
JacineCool. Thanks ;) Sounds fine to me.
Comment #23
Everett Zufelt CreditAttribution: Everett Zufelt commentedI'm definitely in favour of $timestamp staying out of attributes, it isn't an attribute, it is the actual value that is being themed. Other than that, can we get an example of the result of putting a timestamp through gmdate()? This seems like it is just about ready to be RTBC.
Comment #24
aspilicious CreditAttribution: aspilicious commentedDreditor tells me these lines passes 80 characters.
Trailing whitespace
Not going to mark this needs work yet, but it needs to be fixed. Srry! :D
19 days to next Drupal core point release.
Comment #25
Everett Zufelt CreditAttribution: Everett Zufelt commented+ * - format_type (optional): The date format type to be used for the displayed date or
+ * time, e.g. 'short', 'medium'.
What is the default format?
+ * - attributes (optional) An associative array containing the optional properties.
+ * Of note for the time element is:
+ * - pubdate: (optional) A boolean value for the pubdate attribute, defaults
+ * to FALSE.
Actually it appears to default to unset.
+ * - href (optional): A string that contains a URL or URL fragment to pass to the l()
+ * function to render the time element text as a link.
Is this necessary? Can the time element be placed within a link? I guess I am asking here if there is really a good use-case for href in theme_datetime(), and if we might rather have authors use the value returned from theme_datetime() as the text of a link. I am not sure if <time> can be rendered in an anchor, but my guess (without looking) is yes.
Comment #26
Jeff Burnz CreditAttribution: Jeff Burnz commented@25, I'd prefer the time element contained the anchor. I think you're more likely to want the text to link off to something and rest of the element is meaningless (the actual time element is not really relevant). For example I've seen plenty of sites linking the publishing date of comments as a permalink. Either way you could still do what you want, so having this in gives the option.
Comment #27
jessebeach CreditAttribution: jessebeach commentedI agree with Jeff. Having the link inside the <time> element is more elegant.
Everett, in HTML5 the <a> tag can now contain block level content.
Comment #28
Everett Zufelt CreditAttribution: Everett Zufelt commentedI am wondering if this theme function might be getting too complex. Perhaps it should only require $timestamp (for calculating datetime), pubdate, and value (the markup to be displayed to the end user?
It seems to me like there is a lot of logic and duplication of the functionality of format_date and format_interval in this function that really is of little benefit. That being said, I am not a themer.
Comment #29
jessebeach CreditAttribution: jessebeach commentedEverett, I share the sentiment.
I added the extra complexity while working on template updates for the aggregator module. Without this complexity, this theme function would be useless in the very first place I tried to use it. And instead of duplicating the format_interval() stuff in the theme function, I'd need to put that code in N preprocess functions that call theme_datetime.
At its most basic, you pass theme_datetime a timestamp and it works, no need to engage the complexity.
Comment #30
jessebeach CreditAttribution: jessebeach commentedaspilicious and Everett, this latest update takes your comments from #24 and #25 into account.
Everett, the default type passed into the format_date function by the theme_datetime function is NULL. The format_date function defaults to 'medium' when the type parameter is NULL.
Comment #31
jessebeach CreditAttribution: jessebeach commentedI just noticed that I have two comments over 80 characters. Now that I know the standard, I'm trying to be proactive!
I just broke two longs comments from a single line to two lines.
Comment #32
JacineFixing the tag. Was wondering what happened to this. ;)
Comment #33
JacineOh, and sorry for the inbox spam, but un-assigning Everett. Jesse, feel free to assign yourself if you'd like. ;)
Comment #34
jessebeach CreditAttribution: jessebeach commentedI wish we had something like a multi-value field for people that participated in an issue.
Comment #35
Everett Zufelt CreditAttribution: Everett Zufelt commented+ * - pubdate: (optional) A boolean value for the pubdate attribute, defaults
+ * to unset.
I am thinking that the pubdate should default to "false". However, it isn't rendered as "false", or as "true" if set, but as "pubdate". So, perhaps it is okay to have it default to unset. In the case that we leave it default as unset, it should not be listed as boolean, but the help text should say to set it to "pubdate". IMO attribute keys in the attributes array should render the value set, to reduce confusion. Boolean attributes are confusing in html5.
Alternatively we can remove pubdbate from attributes and put it back in variables, but we then run into potential clashes if someone sets attributes['pubdate'] on their own. Thoughts?
+ if (!empty($variables['attributes']['pubdate'])) {
+ $output .= ' pubdate="pubdate"';
+ }
...
+ $output .= drupal_attributes($variables['attributes']) . '>';
From what I see here pubdate, if set to true, will be rendered twice. Once as pubdate="pubdate" and once as pubdate="1". perhaps pubdate should be unset after it is rendered the first time?
Comment #36
Anonymous (not verified) CreditAttribution: Anonymous commentedpubdate isn't a boolean value but a boolean attribute... I had to look up what this meant for microdata a while ago.
Common Microsyntaxes spec
I have to look more closely at the patch, but it feels to me like it should use drupal_attributes to render the attributes instead of processing them into strings. Also, would it make sense to return a render array instead of an HTML string?
Comment #37
Everett Zufelt CreditAttribution: Everett Zufelt commented@linclark
I think returning a string is reasonable.
Although the presence of a boolean attribute represents the true value
<time pubdate>
, and is valid html5, it is not valid xhtml5. So, for best practice we should always be doing boolean attributes as attributename="attributename" e.g.<time pubdate="pubdate">
Other than this tweak, however we decide to make it, I think the patch is looking very good. Most important is to make sure that pubdate isn't rendered twice.
Comment #38
jessebeach CreditAttribution: jessebeach commentedAgreed the output should be
<time pubdate="pubdate">
.I'll get this patch updated today, incorporating #36 and #37.
Comment #39
jessebeach CreditAttribution: jessebeach commentedSince pubdate is just an attribute, rather than treat it special by checking for it, I'd rather leave it up to the user to pass the attribute in e.g.
I removed mention of pubdate from the patch.
Comment #40
jessebeach CreditAttribution: jessebeach commentedJust realized that the comments exceed 80 characters and I had one line with a trailing whitespace character. Fixed in this new patch.
Comment #41
scor CreditAttribution: scor commentedI'm not sure it's a good idea to drop the timezone in the machine readable version of the date. Machine data can end up being reused for the human user. Imagine a scenario where a user script extract the data from the page and displays it in a widget: this script would have to translate the GMT date back to the user timezone to match the value she reads elsewhere on the page. Doesn't it make more sense to leave the job of converting to GMT to the interface if it is really needed?
Comment #42
Everett Zufelt CreditAttribution: Everett Zufelt commented+ 'datetime' => array(
+ 'variables' => array('timestamp' => NULL, 'offset' => NULL, 'format_type' => NULL, 'custom' => NULL, 'pubdate' => FALSE, 'attributes' => array(),
'labels' => array(), 'href' => ''),
+ ),
@pubdate is still in $variables.
+ * - format_type (optional): The date format type to be used for the displayed
+ * date or time, e.g. 'short', 'medium'.
We should have a (default) here. I know that format defaults to medium, but implementers might not.
'attributes' isn't listed in the doc block.
+ // Print attributes
+ $output .= drupal_attributes($variables['attributes']) . '>';
Probably don't need the comment here. Plus we aren't 'printing' anything.
IMO there is still way too much logic here for a theme function, but I'm not a themer, so I'll leave that up to others to decide.
Comment #43
Everett Zufelt CreditAttribution: Everett Zufelt commentedAlso href isn't listed in $variables = array(..), but is in the doc block.
Comment #44
jessebeach CreditAttribution: jessebeach commentedscor, re:#41, we went back and forth a bit on this issue. I agree that the timezone can be useful. Defaulting to the UTC Z timezone seems a good baseline to start from. The definition of the time elements datetime attribute from the W3C specification is
Since the time and timezone are optional and to ensure that a developer can alter this formatter for any reason, I made the gmdate format string configurable and defaulted it to Y-m-d\TH:i:s\Z
This change will be reflected in an upcoming patch. I just need to incorporate Everett's feedback as well.
Comment #45
jessebeach CreditAttribution: jessebeach commentedAfter hours of searching, I finally found the git patch formatting docs: http://drupal.org/node/1054616
I renamed the patch file to conform to the standard.
Everett, the issues you raised in #42 are addressed. I couldn't find where href was missing in variables from comment #43. It seems to be there at the end of the array in common.inc.
The bulk of the complication in this theme function is the offset display option. Rather than just displaying a formatted timestamp, this theme function allows the time to be displayed as {$time} since or {$time} until. I really think this flexibility is valuable. Take it out and the theme function just prints the datetime attribute, the rest of the attributes variable, a formatted timestamp and wraps it in a link optionally.
Comment #46
Everett Zufelt CreditAttribution: Everett Zufelt commentedLooks good to me, let's see what the bots think.
Comment #47
Everett Zufelt CreditAttribution: Everett Zufelt commentedI'm also wondering if we need to add any style rules to this in Core? Do browsers apply any default styling to this?
Comment #48
jessebeach CreditAttribution: jessebeach commentedChrome (to pick one at random) has some interesting HTML5 styling declarations that I wasn't expecting, but nothing about <time>. I think we're safe there with this element.
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebK...
Comment #49
Dave ReidSubscribing for review. Woah nelly this function is doing a lot! :)
Comment #50
Lars Toomre CreditAttribution: Lars Toomre commentedFollowing up on #49, this function should include some unit tests demonstrating the formatting complexity works now and going forward.
Comment #51
jessebeach CreditAttribution: jessebeach commentedLars, I've never written a unit test for Drupal. Can you recommend a doc or example that would help me get started?
Comment #52
skottler CreditAttribution: skottler commented@jessebeach, this doc page is a great jumping-off point - http://drupal.org/simpletest. Looking at, or even copying and modifying existing theme function tests, will also be super helpful.
Comment #53
stevetweeddale CreditAttribution: stevetweeddale commentedTagging for the current sprint
Comment #54
Anonymous (not verified) CreditAttribution: Anonymous commentedjessebeach, I can help with Simpletests next week. If you'd like me to give a quick intro for you, let me know. They are pretty easy once you get the hang of them.
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commentedI couldn't figure out an example of a theme function being directly tested... but in doing issue queue triage, I stumbled upon the patch for this issue, which has an example that's pretty spot on for what we need: #952772: theme_html_tag optional value not really optional
Let me know if you have time to tackle this for the next HTML5 meeting... if you don't, I can take a crack :)
Comment #56
JacineHere's another example that might be useful: #98696-47: Various bugs in theme_links()
Comment #57
jessebeach CreditAttribution: jessebeach commentedChanging to needs work because we still need to write tests.
Comment #58
JacineTagging for the current sprint.
Comment #59
Everett Zufelt CreditAttribution: Everett Zufelt commentedIs it possible to move some of the logic out of the theme_datetime() function and into a template_process_datetime() function to clean up the function that themers see?
Comment #60
jessebeach CreditAttribution: jessebeach commentedWhat do you propose we move out?
Comment #61
Everett Zufelt CreditAttribution: Everett Zufelt commentedEverything associated with determining interval / interval text.
Comment #62
jessebeach CreditAttribution: jessebeach commentedMy fear is that this type of code will end up getting duplicated in several preprocess functions: node, aggregator, and comment as examples. Why maintain it in different places?
Like I mentioned in #29 above, without the interval logic, this theme function is useless in the first place I would use it, the aggregator module templates.
So I keep coming back to this thought: The interval portion of this function is completely optional. At its very basic, if you pass this function a UNIX timestamp, it passes back a
<time>
element with a pretty date-formatted presentation string, formatted by the default formatter. It's totally simple. With a little more code, you can get the presentation string formatted as an interval.Maybe you're falsely assuming I know more than I really do about the theming layer :) Is there a choice preprocess function that you think we should move the interval logic to? Perhaps I'm just missing some obvious solution.
Comment #63
Anonymous (not verified) CreditAttribution: Anonymous commentedI think he was suggesting a template_preprocess_datetime function. This would move a lot of the processing to that function and just leave it to the theme function to place the variables. This would mean that any overriding theme function has access to the processed variables and doesn't have to recreate the logic. It also means that you can alter those variables without overriding the theme function itself.
I think it makes a lot of sense to do it this way. For an example, you could look at template_preprocess_username.
Comment #64
jessebeach CreditAttribution: jessebeach commented@linclark, brilliant! Thanks, I was really wondering what I was missing here. I know Everett has been doing this Drupal stuff a lot longer than me and I didn't want to outright discount the suggestion, but I really had no idea where to go with it.
My stay-cation starts tomorrow (whoop!) and I plan to spend some quality time with this issue in the coming week. Stay tuned!
Comment #65
Anonymous (not verified) CreditAttribution: Anonymous commentedThere is an issue on HTML5 that we should be aware of: Consider replacing <time> with <data>. Since that issue is relevant to the HTML Data Task Force's work on creating a mapping from microdata to RDF and I'm advising on that, I'll probably hear when that issue gets resolved and can report back.
I don't think that this should hold up any of the rest of the work here, though, since the preprocess function and tests will still be relevant even if the element changes.
Comment #66
jessebeach CreditAttribution: jessebeach commentedI'm still working on this, but my time to invest has been limited, so I'm unassigning myself in order not to discourage others from offering up patches.
Comment #67
jessebeach CreditAttribution: jessebeach commentedFrom the constant encouragement of Everett and some insights from linclark, I pulled most of the complexity out of theme_datetime and moved it over to template_process_datetime. I really like the setup of the patch now. theme_datetime feels lightweight and simple.
I'm working on the tests now.
Comment #68
jessebeach CreditAttribution: jessebeach commentedI added five tests and made changes to theme_datetime. I hadn't made all of the parameters to format_date available to configure through $variables. Now all four parameters can be altered: type, format, timezone and langcode.
The tests exercise the basic theme_datetime call; a call with a time; a call with an offset in the past and the label changed; a call with the offset in the future and the label changed; and a call with a link wrapped around the text.
I'm marking it as needs review to get the bots involved.
Comment #69
scor CreditAttribution: scor commentedgreat progress here @jessebeach!
to make this comment more useful for themers, you might want to mention the hook that themers can implement to alter this text.
isn't that something that could be moved outside the theme function? or did you have a reason for leaving it here?
(optional) to be moved after the colon and before the key description. (same for text, format_options and href, and more in template_process_datetime() too).
Comment #70
jessebeach CreditAttribution: jessebeach commented#68: introduce_theme_datetime-1183250-68.patch queued for re-testing.
Comment #71
jessebeach CreditAttribution: jessebeach commented@scor
re: part 1 of comment #69: I put a reference to
template_process_datetime
as an @see for that comment.re: part 2 of comment #69: I went back and forth with where to include these defaults. At first I thought that they could be included as defaults of
theme_datetime
in common.inc. But they're not really parameters oftheme_datetime
; they're parameters offormat_date
. My thinking was to maketheme_datetime
less complex by only listing the necessary parameters for it in common.inc. What's your opinion about this? I'm a bit agnostic.re: part 3 of comment #69: I moved the
(option)
parts in the code as suggested.Comment #72
jessebeach CreditAttribution: jessebeach commentedFound extra whitespace and theme.text was missing a new line at the end of the file.
Comment #74
JacineThanks for all the work on this @jessebeach. Getting close!
Comment #75
Anonymous (not verified) CreditAttribution: Anonymous commentedGreat progress, jesse!
template_process_datetime
totemplate_preprocess_datetime
. template_process was introduced in D7 as a supplement to template_preprocess in special cases where the preprocessing needed to be taken further after all modules had added what they needed to.Using template_preprocess is far more common in core. The only two modules that use template_process in D7 are field and overlay. Overlay needs to get a part of the render array that other modules may have added to after its preprocess ran, and field needs to do things that would otherwise be done in the generic template_process function, but that isn't called because theme_field isn't implemented as a template.
theme('hook')
, so don't have any additional parameters listed.Comment #76
jessebeach CreditAttribution: jessebeach commentedre: 1, my thinking with _process was to give all other modules a pass at changing the label text before core went and declared it so. But you've made me think that I'm conceptualizing the call order
correctlyincorrectly. _preprocess should work just fine.re: 2, I'll move the declaration of these variables to preprocess.
But not tonight. sleep calls. I'll try to get this patch posted tomorrow night.
Comment #77
ericduran CreditAttribution: ericduran commentedHere are some observations
I don't think we need to check if its empty. Being that the default is an array.
This can be set up as an attribute.
So then all this can be simplified into
-16 days to next Drupal core point release.
Comment #78
ericduran CreditAttribution: ericduran commented#72: introduce_theme_datetime-1183250-72.patch queued for re-testing.
Comment #79
jessebeach CreditAttribution: jessebeach commented@ericduran, see comment #21 about moving the datetime attribute into the attributes array.
$variables['timestamp']
is not just used for thedatetime
attribute; it's also used to generate the human-readable text for the<time>
element.Plus,
$variables['timestamp']
needs to be run throughgmdate()
to be formatted correctly. We can't leave that up to the caller to do before providing the timestamp value to the theme function because it would need to be undone for the human-readable text.Agreed on not needing the
!emtpy()
check.Comment #81
sunI don't understand why this is called "datetime", if the effective result is a "time". "datetime" is an attribute on the element.
We only have two patterns in core:
1) [HTML-element-name] à la "select"; i.e., #type === HTML element name.
2) [name + "field"] á la "textfield".
Since the inception of Field API, we're trying to move away from 2) though, as it's very confusing. (And yes, there's even an issue to change "textfield" into "text".)
I'd suggest to use "time" here.
If we keep this enforced text pattern, then the variables should be moved onto the top-level and their defaults should be declared in drupal_common_theme().
1) "HTML"
2) "for a date / time": Even when removing the "date" part, this summary ultimately doesn't really hold water. It returns HTML for a TIME element.
This construct can (too) easily lead to duplicate "datetime" attributes.
We should add in the datetime attribute, if necessary, to 'attributes', and just drupal_attributes() the attributes afterwards.
According to http://html5doctor.com/the-time-element/, this element can hold kind arbitrary inline elements inside of it.
Therefore, I don't understand why we're enforcing plain-text.
Hence, in terms of render arrays, this should be a #theme_wrappers, so any child elements within are rendered as usual upfront, and then provided as #children to the theme function. -- With regard to that, it might make sense to rename the theme variable to 'children', as that means you get the #children for free. :)
I have read the docs. And the code. But I don't understand the purpose of 'offset'. Looks very confusing to me.
At the very very least, shouldn't 'offset' default to REQUEST_TIME...?
The "ago" and "to go" parts need to be expressed in translatable strings. The order/expression is different, especially in RTL languages.
http://api.drupal.org/api/drupal/modules--user--user.admin.inc/function/... uses t('@time ago') for that.
Special-casing the "now" event like this doesn't really make sense, as there's only a time-frame of 1 second in which you may see it.
Popular social networks are displaying "just now" within the first 1-30 seconds, AFAICT. Afterwards, they display "only/some seconds ago" - until a full minute has passed.
Let's remove the custom aligning/indentation of variable values here, please.
'langcode' is defined in 'langcode', not in 'labels']['langcode'.
Not space before the colon.
I don't understand why abs() is needed here.
There should be a blank line between switch cases, unless a case falls through to the next.
No need for type-agnostic comparison here.
The default is unnecessary, since all conditions are covered.
-17 days to next Drupal core point release.
Comment #82
jessebeach CreditAttribution: jessebeach commented@sun, thank you so much for taking the time to do this extensive review. I'll need to spend a few hours digesting the comments. I should have something posted tomorrow.
Comment #83
jessebeach CreditAttribution: jessebeach commented@sun, if we morph this into a #theme_wrappers, then isn't it nothing more than a theme_container that prints a
<time>
tag instead of a<div>
tag?Comment #84
jessebeach CreditAttribution: jessebeach commentedAll of @sun's comments in #81 have been addressed except:
time
element as an interval e.g. 2 months, 3 days ago. I removed the now distinction so the interval is either ago or to go.<time>
element. It could well be that I just don't understand how to employ the render arrays well enough to write this patch. I welcome input on this issue.Comment #85
jessebeach CreditAttribution: jessebeach commentedArgh! Whitespace! I'm using Netbeans. Anyone know of a good way to check for whitespace quickly before posting a patch?
Comment #87
scor CreditAttribution: scor commentedgit diff can show it if you configure it to do so. see my .gitconfig for example. (you should see a red block when running git diff).
Comment #88
Anonymous (not verified) CreditAttribution: Anonymous commentedI didn't notice this before, but we're having the attributes array pass in a timestamp value to be further processed into an HTML ready value and then put back into the attributes array.
I think we should have the timestamp as a separate variable, not in the attributes array. We should still have an attributes array to pass in other attributes though, like microdata's itemprop which must be placed within the time element for time values.
I actually don't see the way that this can be overridden. This variable my have been taken out in an earlier patch.
These are using the Drupal 6 function signature for t().I'm also not sure about using a function call within t()... I'm still confused by all that translation function stuff and how it works.This code could probably be clearer by not creating an array, but just creating variables and then pulling values for that variable from the passed-in array.
Switch to single quotes.
--
Since we want to get a patch up for today's review at BADCamp and I have the time zone edge on this, I'm going to post a patch with these changes.
Comment #89
jessebeach CreditAttribution: jessebeach commented@lin, for the datetime attribute vs. timestamp variable, see comments #21 and #81. I originally advocated for a
$timestamp
variable and this was in place. @sun and @Everett lobbied for just including it in the datetime attribute because, as @sun rightly points out, we could run into conflicts with a user passing in both a $timestamp variable and a datetime attribute, in which case the $timestamp attribute would trample the datetime attribute value.My inclination is to just use the attributes variable and set a default as the code does now with REQUEST_TIME.
I'm tempted to just remove the interval stuff and the labels from this function. It's not going to satisfy all cases. Last night after posting the patch I started thinking that a function, like format_interval, might be a better way to address formatting a time interval so that we're not limiting this common formatting to
theme_time
. I could call this function in something like aggregator_preprocess_time and satisfy the need I have there to format the output as an interval.In this case, by removing the interval formatting code, the
template_preprocess_time
function because much less complex. What do you think?Comment #90
Anonymous (not verified) CreditAttribution: Anonymous commentedRE: the timestamp variable vs. attributes['datetime']... I feel that we shouldn't put any values in an attributes array that wouldn't be valid in the HTML output. I think I can avoid the problems that were brought up in the comments (thanks for the pointers to those), so I'll post a patch with that.
This makes a lot of sense to me. It might be that we could have a theme_time_interval, just as theme_image_formatter sets variables to go into theme_image.
Comment #91
catchFor the test failure there are two options:
- make it DrupalWebTestCase
- call theme_time_interval() instead of theme('time_interval') - theme() depends on the theme registry, which depends on the module system/db etc. so any function calling theme() should not be a unit test.
Comment #92
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks catch. It was passing on my local so I thought it was the testbot... didn't realize that there was actually an issue. Changing to theme_time means we'd have to populate the defaults from theme, so I've gone with DrupalWebTestCase.
Jesse's idea to remove the interval logic has really simplified this. We can move all of the interval processing from the previous patch to a theme_time_interval function.
I made the changes I suggested in #88.
Comment #93
jessebeach CreditAttribution: jessebeach commentedlinclark, I love the way you dealt with the datetime attribute and the timestamp. Really elegant.
Last thing, let's make sure the commit message is formatted correctly: Issue #1183250 by jessebeach, linclark, scor, Everett Zufelt: Introduced theme_time to theme.inc so that attribution is given correctly.
Super excited about how this turned out. Let's print it.
Comment #94
skottler CreditAttribution: skottler commentedThis really looks great.
Marking RTBC.
Comment #95
sunCurious, why is this a UNIX timestamp? This means the value is restricted to be later than 0; i.e., start of the UNIX epoch; 1970-01-01.
I think that a generic TIME theme function should be compatible with all kind of dates; i.e., also historic ones.
Hence, I guess we actually need a datetime object here.
The interdependency and/or relation between these is still confusing to me. I'm not sure whether we have such a conditional theme variable pattern in any other theme function. Do you see a way to simplify or remove one of both?
Also, minor: Doxygen directives cannot be used within an directive. In this case, just spell it out; i.e., "See format_date() for more details."
Related to the text and format_options above, it's not really clear to me why a TIME element asks for or supports a "path" to link to. This might be the most confusing part for me currently.
As far as I understood the spec so far, the datetime attribute is actually optional and not required. Why are we enforcing it here?
empty() excludes 0 and "", whereas these values might be intentional. I think we want to use !isset() here.
I don't think it is valid for a theme function to actually produce default content that is the actual point of the theme function itself. I.e., that's like theme_node() producing a default node output if you don't pass one in, which is not the case.
In other words, I think we want and need to remove this timestamp default value.
It looks like these fallback values should be the default values for the variables in the theme registry.
With those defaults, these four lines can be removed entirely and you can pass the $variables directly to format_date().
-19 days to next Drupal core point release.
Comment #96
skottler CreditAttribution: skottler commentedThis is a really great point. @jessebeach, do you have an idea in mind as to how we handle this?
Comment #97
scor CreditAttribution: scor commentedhttp://php.net/manual/en/function.gmdate.php says that the timestamp is optional, and will "default to the current local time if a timestamp is not given. In other words, it defaults to the value of time()."
If REQUEST_TIME is the same as the local time, then we don't need to have this code.
-19 days to next Drupal core point release.
Comment #98
JacineChanging the title.
Comment #99
skottler CreditAttribution: skottler commentedI am working on this now at the Core sprint.
Comment #100
skottler CreditAttribution: skottler commentedComment #101
skottler CreditAttribution: skottler commentedComment #102
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for the thorough review, sun :) It's a little late in Ireland, so just responding to two items that stuck out to me:
The time element itself will possibly/probably be removed from the HTML spec and replaced with a data element. This has been stated by Hixie on the HTML bugtracker. At that point, whatever attribute is the equivalent of datetime will be the only way of specifying that this is indeed a time value.
0 and "" would not be valid values for this attribute. From the spec: "If the element needs a date, and the datetime attribute is present, then the attribute's value must be a valid date string with optional time."
Comment #103
Everett Zufelt CreditAttribution: Everett Zufelt commented@linclark
I posted a comment on the html5 bug to request that Hixie clarify his statement about "m...moving away from the <time>..." element. ( http://www.w3.org/Bugs/Public/show_bug.cgi?id=12942#c3 )
Comment #104
scor CreditAttribution: scor commented@Everett Zufelt the main issue is that one: http://www.w3.org/Bugs/Public/show_bug.cgi?id=13240
Comment #105
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks Everett, I also asked what the current thinking was in #whatwg, but didn't get a concrete answer from anyone. Hopefully we'll get an idea soon. However, as I mentioned in #65, I don't think that this should hold up work.
We should enforce the datetime attribute because:
time
will probably change to<data>
, at which point in time we will need this attribute to provide the time value.There is no downside to using the datetime attribute, and if we make it optional, then we place the burden on the caller to check whether the text content is going to be a machine readable date value. It's not worth the cognitive load to expose it as optional.
Comment #106
Everett Zufelt CreditAttribution: Everett Zufelt commentedAgreed w/ @linclark in #105
Comment #107
Anonymous (not verified) CreditAttribution: Anonymous commentedUsing historic dates is more difficult, but possible with the current implementation. If a developer passes in a value for $variables['attributes']['datetime'], then it will supersede the value generated by using the timestamp. Dates before 1970 and after 2038 would have to be processed into a valid date string with optional time by the module code before calling theme_time.
On the other hand, if we optimize to make it easier for this edge case, then we have to change from using gmdate with a timestamp (which are commonly used in Drupal for values such as created and updated) to using the DateTime object. The DateTime object can take a timestamp, but it must be prepended with an @ sign. We would have the following options (as far as I can see):
--or--
--or--
I don't think any of these options are better than our current solution. Most use cases are working solely with dates between 1970 and 2038 and most are pulling timestamps out of the database already. I believe we should optimize for these use cases and make the edge case difficult but possible using the datetime attribute.
Comment #108
jessebeach CreditAttribution: jessebeach commentedthanks for summing that up linclark.
Comment #109
Anonymous (not verified) CreditAttribution: Anonymous commentedI've updated the patch to take into account the feedback.
I discuss this in the comment above. However, the patch now goes a bit further than it did. If
$variables['attributes']['datetime']
is set, it is used to process the text variable. This might address your concern.Agreed. If users really want to configure this and understand how to configure
format_date
's parameter, then they should just callformat_date
and pass in the configured value as$variables['text']
. We can then useformat_date
's defaults when there is no$variables['text']
set.By this logic, we should also remove the configuration variable for gmdate. If someone wants a customized attribute for the datetime format, they will have to call
gmdate
to get the value and place it in$variables['attributes']['datetime']
.Agreed. Anyone who wants to link the time element can call l() with the output and $options['html'] set to TRUE, as theme_image_formatter does.
Addressed in #105. I feel strongly that we should place the datetime automatically. However, removing the timestamp default (as I do below) means that datetime is no longer placed in certain circumstances (if neither $variables['timestamp'] nor $variables['attributes']['datetime'] are set). See the tests for an example.
Addressed in #102.
Agreed. I don't see a precedent for this.
No longer necessary since $format_options were removed.
Comment #110
jessebeach CreditAttribution: jessebeach commentedLove it. Let's print it.
Comment #111
Everett Zufelt CreditAttribution: Everett Zufelt commentedhttp://html5.org/tools/web-apps-tracker?from=6782&to=6783
Looks like <time> is gone. I can't read diffs like this with any usefulness, so can someone else please confirm?
Comment #112
aspilicious CreditAttribution: aspilicious commentedWell I read the entire issue and yes it's gone... BUT it's possible they will revert the removal
They use Data now. Not everybody in the issue is happy with it, so I'm not sure what will happen in the future...
They use a lot of examples in combination with schema.org
examples:
It *seems* that schema.org (aka microdata) is being tightly coupled to the html5 spec. Not sure if it's a good thing. Anyway if this continue http://drupal.org/project/microdata will be part of D8.
Comment #113
Everett Zufelt CreditAttribution: Everett Zufelt commentedhttp://www.w3.org/Bugs/Public/show_bug.cgi?id=13240#c49
This has been escalated to an HTML WG tracker issue. There will likely either be a request for a reversion or a request for change proposals.
Comment #114
scor CreditAttribution: scor commentedI'd wait a bit before switching time to data until a final decision is made in the HTML WG. As for the microdata vs RDFa, the HTML Data Task Force is actively looking at evaluating the two and producing guidelines for authors.
Comment #115
skottler CreditAttribution: skottler commentedComment #116
Anonymous (not verified) CreditAttribution: Anonymous commentedI don't think that we need to postpone this issue.
I think that we want to have a theme_time in Drupal 8. It's pretty clear that there will be a solution for time in the HTML5 spec. Whether or not we have to spit out a time element or spit out a data element is a small detail and we can easily file a new issue if the spec changes back. We should just make sure that no tests outside of the unit tests for theme_time rely on the specific markup.
This doesn't require microdata. While the itemprop microdata attribute is used in an example in the current spec, it is only used to say this is a pubdate, and we don't have that functionality in the current patch. For the functionality we currently have, the only attribute we need is the value attribute, which is part of HTML5 itself, not microdata. Therefore, this doesn't rely on whether or not microdata module is in core.
We want other patches to be able to rely on having theme_time, so I think we should move forward with whatever the current spec is. If there is a spec change later, we change to reflect it. The impact will be localized and shouldn't affect calling code at all.
Comment #117
aspilicious CreditAttribution: aspilicious commentedThnx for the clarification linclark :)
Comment #118
skottler CreditAttribution: skottler commentedComment #119
Anonymous (not verified) CreditAttribution: Anonymous commentedI updated the patch to use
<data>
with the value attribute.I also added back in a little bit of configurable-ness. Now, you can use any of the system's defined date format types to format the text content or the value attribute. See the tests for examples of how this would work.
Comment #120
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #122
Anonymous (not verified) CreditAttribution: Anonymous commentedNeeded to update my code base. As posted before:
I updated the patch to use
<data>
with the value attribute.I also added back in a little bit of configurable-ness. Now, you can use any of the system's defined date format types to format the text content or the value attribute. See the tests for examples of how this would work.
Comment #123
Anonymous (not verified) CreditAttribution: Anonymous commentedGah! no new line at end of file.
Comment #124
Anonymous (not verified) CreditAttribution: Anonymous commentedThis needs a little bit of work. The format_types array's defaults should be merged in at the beginning of the template_preprocess function. That way, a format type can be set by the caller without having to set both of them.
Comment #125
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch outputs a
<data>
element with a value attribute. For example:$variables['timestamp']
or a valid date string with optional time in$variables['attributes']['value']
.value
attribute from the timestamp if$variables['attributes']['value']
wasn't passed in. Defaults to datetime formatting, but callers can switch this to output a simple date using$variables['value_type']
.$variables['text_type']
, which is a Drupal date format type. Defaults to medium. This text content can be overridden with$variables['text']
in circumstances where the text isn't really date output, like "Dries's birthday".Comment #126
JacineThanks Lin :)
There's too many to list, so I'll just summarize. All of the @see comments and the text of each test case still refers to theme_time(). I assume you probably left those in temporarily pending the function name change here. ;)
Minor nitpick, but most everywhere else in this patch 'value' is in quotes except here.
This is another minor nitpick, but In most cases, this is done inline. It's not done consistently, so I guess this is fine, but I also found that when $variables['x'] is used (most other cases are $elements), the variable it uses it named 'whatever_array'. Any reason we are not using attributes_array in the preprocess function?
This is totally fancy. I've never seen try catch used in a preprocess or theme function, let alone logging something to watchdog. I'm not really sure if this is a good or a bad thing right now. My gut is telling me most of this should be a function, but I trust you know better than me and that this is probably the most efficient way to code this. :D
Should rename this.
The documentation is looking really good though, and I agree that it's easier to grok than previous patches.
Comment #127
ericduran CreditAttribution: ericduran commentedThe $variables['text_type']; variable is declared as optional, but we're not doing any checking before accessing it.
-1 days to next Drupal core point release.
Comment #128
Anonymous (not verified) CreditAttribution: Anonymous commentedThat's because a default is set in declaration of the theme function. If the user doesn't set it, it defaults to 'medium'.
Comment #129
ericduran CreditAttribution: ericduran commented@linclark ha, yea. I see that now.
Comment #130
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks Jacine and ericduran for the reviews.
attributes_array
. Moved it inline as well.$variables['attributes']['value']
and$variables['text']
. This is a very limited use case so I don't think that is a problem.Eric brought up over IRC that there might be cases where there is no text value, in which case we should possibly output a self-closing data element, i.e.
<data value="1978-11-19" />
.Comment #131
Anonymous (not verified) CreditAttribution: Anonymous commentedPatch updated for core directory, hope it worked properly.
Comment #132
webchickI offered to try and review this. However, it could really use an issue summary. :) Tagging.
Comment #133
Anonymous (not verified) CreditAttribution: Anonymous commentedWebchick, I added an issue summary. Also, this is what the patch does
This patch outputs a
<data>
element with a value attribute. For example:$variables['timestamp']
or a valid date string with optional time in$variables['attributes']['value']
.$variables['attributes']['value']
wasn't passed in. Defaults to datetime formatting, but callers can switch this to output a simple date using$variables['value_type']
.$variables['text_type']
, which is a Drupal date format type. Defaults to medium. This text content can be overridden with$variables['text']
in circumstances where the text isn't really date output, like "Dries's birthday", or where a timestamp wasn't passed in because$variables['attribtues']['value']
was set directly.Comment #134
JacineAnd we're back to
<time>
! http://lists.w3.org/Archives/Public/public-html/2011Nov/0011.htmlComment #135
Everett Zufelt CreditAttribution: Everett Zufelt commented@Jacine et al.
It is still possible that <time> will be removed from HTML5. If so, it will be done through the standard HTML WG consensus process. I don't think, however, that the dangling fate of <time> is any reason to delay this issue.
Comment #136
JacineTBH, I think we should use
<time>
for this regardless, but agree there's no point in holding this up. It's still in the W3C spec, so it's not exactly a rogue move and many are suggesting to do just that. Anyway, that's my 2 cents.Comment #137
jessebeach CreditAttribution: jessebeach commentedI rerolled the patch for the element.
I also took liberties with it to reduce the functionality down to 4 cases. It had been 5 with the 'text_type' variable that allowed a user to change the format_date format type.
These seem the most basic use cases for this theme function. Anything beyond this will need to be applied through preprocess and process functions.
The reason I took out text_type is that format_date actually has 4 options: the timestamp, the format, the timezone and the language. I felt we should either provide hooks for all 4 or just use the defaults for the 3 formatting parameters beyond the timestamp. The format of the date text can be altered in a preprocess function if further configuration is needed.
@linclark, are you ok with these changes? Do they make sense?
Comment #138
jessebeach CreditAttribution: jessebeach commentedOh, @Jacine and @linclark, I should also mention that I removed attributes_array array and just pass through the attributes array. After looking into this some more, the attributes_array is really only used in template_preprocess and template_process.
These hookless functions are only called when templates are employed (see details below). The pattern seems a bit odd to me and simply setting $variables['attributes'] to $variables['attributes_array'] doesn't add any value and threatens to add a little misdirection, since one might think the change was made in order to support some further processing, when really, it's just a name change for the sake of a loose pattern. Make sense? For this, I'll conjure the acronym KISR (Keep It Simple Rockstar!).
Details
In _theme_process_registry() see the code associated with the following comment (begins at line 583 of theme.inc):
Comment #139
JacineIt's extremely odd in this case because "attributes" is one of the arguments. We had discussed putting the attributes in an 'options' key, like it's done for
theme_link()
in IRC, but I think it's more trouble than it's worth at this point. I completely agree the way it had to be coded makes it confusing and wouldn't work for this. It really bugs me because we have a preprocess function here and a hook (whether or not it's output via a template or function it's still a theme hook), so consistently working with attributes in preprocess functions is something that should just work (without the name changing nonsense that was needed here). But in testing this I realize that it doesn't and that's a far bigger problem and not related to this patch, so it'll have to be dealt with in a separate issue.Anyway, this patch looks really good to me. It's much simpler now, which is a good thing IMO. If @linclark is all good with it, I'd dare to say it's RTBC. :P
Comment #140
Everett Zufelt CreditAttribution: Everett Zufelt commentedAgreed w/ @Jacine that this is looking very good.
What is a 'valid date string'? Can we provide a URL to the W3C definition?
Can we add that this is a valid date string, i.e. why are we using this particular format?
I didn't see pubdate mentioned anywhere. It is really only an attribute, but important enough that we might what to add it to the theme_datetime() doc block.
Comment #141
Anonymous (not verified) CreditAttribution: Anonymous commented@jessebeach Thanks for updating this :)
One thing webchick mentioned in an informal review over IRC was that the function needs to escape
$variables['text']
. Before we hadcheck_plain
, but sun pointed out that<time>
can contain more than just plaintext, so probably should usecheck_markup
.The comment for
$variables['text']
needs to change based on the code changes. Something likeI typed a few arguments in favor of the types and then realized that I think I agree with you... though I think that maybe the
datetime_type
should still be included to switch between'datetime'
format and'date'
format.On
attributes_array
, I think that we should still have it as anattributes_array
variable in the preprocess function for consistency with other preprocess functions. Since it isattributes_array
in all other preprocess functions, that is the variable that people will be used to adding attributes to in every hook_preprocess implementation.... but.....I think we might want to file an issue to clean this up across the theming system though. Is there any reason why
'attributes_array'
shouldn't just be called'attributes'
to start with? I understand that'attributes_array'
makes clear that it still needs to go throughdrupal_attributes
, but I think that's also clear when you try to print an array. @Jacine, I'll defer to your opinion on that.Here's the W3C url: http://www.w3.org/TR/html5/common-microsyntaxes.html#valid-date-string-w...
Agree that the comment should mention this.
Comment #142
webchickThanks, Lin. I never did get back to this the other day. :( Apologies.
But regarding the attributes_array thing... in looking through, my observation about attributes_array is the same as Jesse's. It's not a preprocess function thing, it's a preprocess function for a template file thing that's the pattern. So this function is perfectly fine just making it a standard array called 'attributes' IMO. Makes the code much cleaner, too.
Comment #143
JacineYeah...So, this is actually a much bigger issue. I was under the impression that we had implemented preprocess functions for theme functions in D7 in a sane and consistent way, and that attributes_array would be defined in template_preprocess() and flattened in template_process(). Turns out that it's not true, and I was mislead by horribly buggy documentation: #1333122: Fix documentation in theme() and template_preprocess(). I'm very, very frustrated about it, and I'm writing up a post which I hope to share soon. As soon as there is an issue posted, I will reference it here.
In the meantime, let's just ignore that part, use 'attributes' and get this done. ;)
Comment #144
sunWhat is a "valid date string"? Valid in which context or for what?
"a date string with optional time in $... valid for gmdate(), for example '...'."
Let's move the preprocess function right above the theme function in the code.
Should be !isset()
Comment #145
jessebeach CreditAttribution: jessebeach commented@Everett (#140)
I addressed your comments.
@linclark (#141)
<time>
is again removed and replaced with<data>
we should then put the datetime/date switch back in. Until then,<time>
doesn't support the date attribute.<p>
around the text. filter_xss just strips common xss attacks. I tested one just to be sure. All the values in the attributes array are already passed through check_plain.@sun (#144)
I addressed your comments. I used empty() rather than !isset() so that empty strings would return FALSE rather than TRUE. Is it Drupal practice to consider an empty string as valid and intentional string content in a case like this?
Comment #146
jessebeach CreditAttribution: jessebeach commentedI moved the filter_xss call from template_preprocess_datetime to theme_datetime. Otherwise this patch is the same as #145.
Comment #147
scor CreditAttribution: scor commentedfilter_xss is somewhat arbitrary and cumbersome (it strips span tags for example). Please check_plain() $variables['text'] by default, and introduce a $variables['html'] for those who want to display custom HTML. This is for example the behavior of l() and to a certain extent drupal_set_title().
Comment #148
jessebeach CreditAttribution: jessebeach commentedAdded $variables['html'] which defaults to FALSE. check_plain is now called in theme_datetime on $variables['text'] until $variables['html'] is TRUE, in which case, $variables['text'] is printed without filter.
Comment #149
Everett Zufelt CreditAttribution: Everett Zufelt commentedLooks good to me. Great work.
Comment #150
Anonymous (not verified) CreditAttribution: Anonymous commentedUgh, so I'm now realizing that we don't actually want the datetime value to be set to UTC, but instead want the datetime value to use the offset from UTC. Otherwise, times won't display properly in event Rich Snippets in Google.
For example, the start time for DrupalCamp Ohio would currently be output as
However, we would want it to be output as:
The
datetime
attribute takes a valid date with optional time. The spec says that if the "datetime attribute is present, then the attribute's value must be a valid date string with optional time." A valid date string is:A valid date with time is
Both are allowed with the datetime attribute. The link I posted in 141 is the definition of 'valid date string with optional time.' The code never added a
date
attribute (which wouldn't be valid on a<data>
element either AFAIK).Comment #151
jessebeach CreditAttribution: jessebeach commentedAh, my mistake for not understanding that. I'll work it back in.
RE: the timezone offset, do you think we should just use the install's configured timezone as a default?
Comment #152
webchickHm. That's tricky. The answer really depends on context.
Take groups.drupal.org, for example. The site-wide default time zone is set to UTC. For something like a post date on a discussion in the Views Developers working group, that's likely the right timezone to be using. However, the node to announce the DrupalCamp Ohio event should output the time in US Eastern Standard Time.
I wonder though if this discussion isn't beyond the scope of what this theme function needs to be concerned with. Presumably, the Date module would handle preparing the string to pass to theme_datetime() based on the particular Date field settings in question (which in the case of events, lets the person creating a node specify the time zone, which is then stored alongside the field value).
So I would actually prefer if theme_datetime() didn't make any assumptions about timezones, if possible.
Comment #153
jessebeach CreditAttribution: jessebeach commented@webchick, this is the concern I brought up in #19. If we print different content (a different value for datetime) based on the server that serves the request, then we run into fluctuating values for content.
The human-readable
<time>
value runs through format_date and will be formatted according to the configured timezone.What I don't understand @linclark, is what is the need for Rich Snippets in Google? Why is formatting the timestamp with UTC without a timezone offset going to confound Rich Snippets? Can you point me to some docs?
Comment #154
Anonymous (not verified) CreditAttribution: Anonymous commentedwebchick's right, this should be handled by the date field settings.
@jessebeach Google doesn't really document their Rich Snippets super well, so there's no docs page to point to on this AFAIK.
However, I have coded an example. I've changed the starttime to be 7pm EST on Dec 3, which is 12am the next day in UTC. In the Rich Snippets Testing Tool, this shows up as Sun, Dec 4. It really should show up as Sat, Dec 3.
Comment #155
Anonymous (not verified) CreditAttribution: Anonymous commentedThe time element has a new specification as of Friday night... with commit the <time> element is dead, long live the <time> element.
The new time element has 9 valid date string formats:
I believe that we want to optionalize this so that people using the function don't have to know how to create the HTML date formats but can instead just say "I want to use month format" or "I want to use duration format".
I see two ways to optionalize this. We could have a flag passed in to the theme function (like the value_type I had in earlier patches) or we could have something like a format_html_time which takes a timestamp and a flag and returns an appropriately formatted string.
I'm going to sign on to office hours to discuss this.
Comment #156
Everett Zufelt CreditAttribution: Everett Zufelt commentedThe SVN log / diff isn't super easy for me to read. Do you know if this is in any of the W3C drafts?
Regardless, I'd recommend letting this issue be postponed for a couple weeks until the spec for <time> seems more stable. Alternatively we can keep trying to play catch up with the spec.
Comment #157
Anonymous (not verified) CreditAttribution: Anonymous commentedIt's not in the W3C draft, but is in the WHATWG draft. I'm not sure, but hopefully that is easier to read.
So far, all of the changes in the spec have just resulted in simple, superficial changes to the patch. The most recent change has broadened the number of options for datetime formatting, but (as Jacine mentioned) the datetime formatting probably should have been in a helper function in the previous patches anyways. It isn't really the change to the spec that is motivating the change in approach. There were already two options for the datetime formatting and we were going to have to find the best way to optionalize those anyway.
Since the changes are superficial, we can keep working on figuring out the best approach without the changes in the spec really affecting anything. I would really like to get the approach pretty much finalized while it's still fresh in people's minds rather than waiting a few months and rehashing the whole thread.
Comment #158
Anonymous (not verified) CreditAttribution: Anonymous commentedI added #1347648: Change date_iso8601 to format the 9 HTML datetimes to create a helper function for this. That helper function addresses the issues I brought up regarding the date/datetime value types and the timezone offset.
Comment #159
Everett Zufelt CreditAttribution: Everett Zufelt commentedI agree that this issue needs to be fixed, and am grateful for all of the work that everyone has put into it.
I will not support the patch if it is based on WHATWG spec language that is not in the W3C spec.
I * strongly * think that the issue should be postponed for a couple of weeks to track the status of <time> in the W3C spec before any other decisions are made.
Comment #160
Everett Zufelt CreditAttribution: Everett Zufelt commentedApart from trying to implement an unstable piece of the HTML5 spec(s), I think that this function is a bit over complicated.
From what I recall my original intent was to provide a method to theme a datetime.
At the very least the function should accept:
- timestamp
- Optional date as string
- optional pubdate (in attributes aray)
The timestamp should be converted into a valid @datetime attribute. The text string should be used for element content. If the text string is not present then @datetime shouldalso be used as element content.
I also like allowing @datetime to be set in attributes array, overriding the need to generate @datetime from timestamp (so I guess that timestamp might also be optional.
Comment #161
Anonymous (not verified) CreditAttribution: Anonymous commentedWhich patch are you referring to? Because you said "Looks good to me. Great work." in response to the last patch posted on this thread.
I don't think it's ready for a commit and am not advocating that, but I'm unclear on how the function has changed since you last reviewed it.
Comment #162
Everett Zufelt CreditAttribution: Everett Zufelt commented@linclark
I've changed my mind. I have spoken to more than one themer who has stated that the function is over complicated. but, I wasn't very clear about that in my last comment.
What is the use case that this function is trying to satisfy? That is something that I didn't fully specify when I opened the issue. That might help us to finish up development here. I still do feel like postponing for a couple of weeks, so we quite chasing the spec, is the best idea.
Comment #163
jessebeach CreditAttribution: jessebeach commentedThe use case I had in mind when working on this patch is this:
"I have a date and/or time. I want to render this date/time as an HTML time element. I don't know what the correct time format for this element is, nor do I really care that much. The formatting of the time element should simply be standard and correct."
More and more it seems that this element is really complicated and anything beyond a basic use case is going to require more doc than code.
Comment #164
jessebeach CreditAttribution: jessebeach commentedI'm setting this issue to postpone. I feel like we're stuck in a loop.
Comment #165
Anonymous (not verified) CreditAttribution: Anonymous commentedAs talked about with jessebeach on IRC, we would both like this to move forward. In particular, jesse brought up the need to have this function in place for changes to aggregator templates.
I have attached a patch that hopefully simplifies the function. It makes these changes:
l()
convention so should be a pattern developers are used to. This might also help with the attributes inconsistency we talked about around comment 143 or so.Because #1347648: Change date_iso8601 to format the 9 HTML datetimes hasn't been committed, this patch will not pass tests. It should be reviewed solely for DX.
Comment #166
Anonymous (not verified) CreditAttribution: Anonymous commentedAs discussed in IRC, we should be able to add these as built-in types for format_date (I didn't realize that the built-in dates weren't returned by
system_get_date_types
).This negates the need for format_html_datetime. However, we would need to be careful about format names. Modules currently are aware that 'small', 'medium', 'large', and 'custom' are built-in format names. Modules might unintentionally collide with the following:
Comment #167
Anonymous (not verified) CreditAttribution: Anonymous commentedBumping this back down to needs work, will be posting another patch today.
Comment #168
jessebeach CreditAttribution: jessebeach commentedYay! I'll make time to review.
Comment #169
Anonymous (not verified) CreditAttribution: Anonymous commentedThe attached patch adds the following formats to format_date():
I prefixed them with 'html_' so that it would 1), be easy to keep them grouped together, 2) be clearer what their purpose is, and 3) decrease the likelihood of unintentional conflicts with module defined formats.
I don't include
html_duration
because we will probably want to refactorformat_interval()
to handle that one. We can file another issue for that support.Comment #169.0
Anonymous (not verified) CreditAttribution: Anonymous commentedIssue summary for reviewer.
Comment #169.1
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdate for patch at #169.
Comment #170
msonnabaum CreditAttribution: msonnabaum commentedThis mostly looks good, except I agree with sun that many of these empty()'s really should be !isset(). IMO it's not relevant whether or not 0 or "" is a valid value in this scenario, but it's more about intention. In almost all of these cases an array is being checked for the presence of a key, it's not validating the value if the key exists.
Comment #171
tim.plunkettLooking at http://dev.w3.org/html5/markup/datatypes.html#common.data.datetime and http://www.php.net/manual/en/function.date.php, I think we should be using 'Y-m-d\TH:i:sP'. O = +0200, P = +02:00
This would become 2007-03-25T17:00:00-07:00
all of the +0000 in these tests would be +00:00
Borrowing the link from #141, http://www.w3.org/TR/html5/common-microsyntaxes.html#valid-global-date-a... also says to use the colon.
Also of note, just in case everyone wasn't already confused by date types, Date module refers to the ISO format as "Date", while "Datetime" refers to "database's native date format (YYYY-MM-DD HH:MM:SS)". But HTML uses "Datetime" to mean ISO. Yay!
14 days to next Drupal core point release.
Comment #172
theborg CreditAttribution: theborg commentedMinor thing: some comment lines are over 80 char long:
// See http://www.w3.org/TR/html5-author/the-time-element.html#attr-time-datetime
Comment #173
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks everyone for the reviews, glad that this is almost there.
@msonnabaum
Ok, I'll make that change.
@tim.plunkett
Hixie has been taking feedback about the time element into account and has been making a number of changes (and I believe all of these changes are not contested except in minor details). Basically, the reasoning was if we're going to have a time element, the spec should reflect how people actually thought they were supposed to use it since no one was using it as it was spec-ed.
Both of those timezone offset formats are valid (both as ISO8601 and in the editors version of HTML5)... but if we want to change to go with the old spec, that's fine by me.
@theborg
I think it's better to keep the URLs all on one line rather than split them, but if you have examples of split URLs in core, I'll follow precedent.
Comment #174
jessebeach CreditAttribution: jessebeach commentedAgreed with @linclark's proposed changes.
Comment #175
tim.plunkettI'm sold.
Comment #176
Anonymous (not verified) CreditAttribution: Anonymous commentedOk, so this replaces three
empty
's with!isset
and also removes the duplicatecheck_plain
that catch mentioned to jacine... and by-golly, I think that might just be it :)Comment #177
tim.plunkettAwesome! Looks great, meets the spec, passes tests, makes Drupal 8 better.
Comment #178
skottler CreditAttribution: skottler commentedThis is great - looks good to me.
Comment #179
webchickthat looks like an oops. :)
Comment #180
Anonymous (not verified) CreditAttribution: Anonymous commentedis that debug call meant to still be in there?
Comment #181
Anonymous (not verified) CreditAttribution: Anonymous commentedDARNIT!!!
New patch, hopefully no oopsies this time.
Comment #182
JacineYay! Thank you, thank you everyone that helped (especially linclark, jessebeach and Everett Zufelt) for all the work you put into getting this patch ready. I know it was rough. I can't wait to finally see this fixed! Wooo :D
Comment #183
catchStill there ;)
Comment #184
aspilicious CreditAttribution: aspilicious commentedI. Don't. Want. To. Steal. Credit
Comment #186
aspilicious CreditAttribution: aspilicious commentedEvil issue...
Not sure I fixed something...
Comment #188
aspilicious CreditAttribution: aspilicious commentedAAAAAAH found it, windows endings...
Comment #189
msonnabaum CreditAttribution: msonnabaum commented#188 is identical to #181 without the debug, so moving back to RTBC.
Comment #190
jessebeach CreditAttribution: jessebeach commentedI'm hoping for this Christmas miracle.
Comment #191
sunSorry, but a plain theme function that doesn't account for the render system at all is not enough when targeting Drupal 8, IMHO.
Attached patch extends the test to verify that this new theme function can be reasonably used in a render element. The test will fail.
It's only one of multiple possible options to structure the render element, and merely was the most natural choice based on the existing assertions.
(Note: Quite possibly, I've mistaken #attributes][html with #html -- #options doesn't really make sense here.)
In addition to that, I'd still like to see a special condition in the preprocessor along the lines of:
Comment #193
JacineIt really sucks to get this kind of feedback #193 comments in. This is the kind of review and direction I was hoping for when I asked back in October.
Is this really required to "reasonably" use this function in a render array? If so, that's pretty unfortunate.
Regarding "targeting Drupal 8" and theme functions, I didn't get the memo, so if this is something that needs to generally happen, then documenting why and how would be a good start. If we have to go through this on every theme functions we touch, I guarantee you that half the team working on HTML5 will give up, and I wouldn't blame them.
Comment #194
jessebeach CreditAttribution: jessebeach commented@Jacine, agreed. #191 has demoralized me a bit.
We can improve D8 in iterative commits. Each commit need not be perfect. It should merely add value. What we have so far adds value. Addressing support for render arrays adds value as well and that step can be taken in a subsequent issue. Progress in small steps keeps us moving forward.
Comment #195
Anonymous (not verified) CreditAttribution: Anonymous commentedAm I missing something, or is part of why this failed because there isn't a theme_timestamp?
6 days to next Drupal core point release.
Comment #196
sunComment #197
skottler CreditAttribution: skottler commentedNot sure why this wasn't architected this way to begin with...
But it looks good to me (and miraculously passes tests) so RTBC.
Comment #198
JacineJumping in at the 11th hour and taking over the patch that people have been working on for ages is just meh.
This is what happens when there is no documentation and an API that was implemented in a ridiculously inconsistent way.
I'm not going to change the status because I just want this issue to be over with, but the thought of having a children argument in every friggin theme function makes me want to hurl. I think I officially hate Render API. That is all.
Comment #199
Anonymous (not verified) CreditAttribution: Anonymous commentedWe REALLY need to think about this.
theme_link
does functionally the same thing astheme_datetime
. They both routinely output an element from the HTML spec,<a>
in the case of the former,<time>
in the case of the later.Then why should
theme_link
take both 'attributes' and 'html' as part of an 'options' element, buttheme_datetime
not?To my mind, they should both work the same way.
Comment #200
sunThat is, because theme_link() is not invoked normally; it needs to be explicitly "enabled" first, as documented. (could be more clear, I'd agree; but separate issue)
theme_link() was designed to be compatible with (and constrained to) the function signature of l(), which takes exactly those arguments: $text, $path, $options. In turn, $path and $options are the literal arguments for url().
Therefore, theme_link() should not be taken as an example to follow in any way.
Comment #201
sunThat said, if you'd really want to follow l()/url()/theme_link()'s example, then the direct equivalent to l()/url() would be format_date().
In turn, that would require the following theme variables:
$timestamp, $type = 'medium', $format = '', $timezone = NULL, $langcode = NULL
Of course, in addition to the HTML output variables $text, $attributes, and $html.
Comment #202
Anonymous (not verified) CreditAttribution: Anonymous commentedI can't understand why this inconsistency was introduced, I don't see the constraint. The patch in "Make l() themable" issue was actually changed from being consistent with the rest of the theme functions to being inconsistent, with no reason given. Maybe I'm missing something.
But yes, since
theme_link
is a special case, I'm fine for going back to having 'attributes' and 'html' as variables as they were in #148. I introduced 'options' get feedback in #165 because Everett Zufelt said the patch in #148 had gotten too complicated for themers.l()
, so I'm guessing it's not because of the function signature. Does anyone know why theme_link doesn't account for children?theme_menu_local_task
?Comment #203
sun'attributes'
and'html'
are common theme variables/arguments; see theme_links() (and others).Let me clarify how render elements are rendered:
#children
property of the parent element.#theme
or#theme_wrappers
) gets all #properties of the render element as theme variables (hence,#text
=>'text'
, and#children
=>'children'
).Accounting for rendered children especially makes sense when the theme function supports a notion of inner HTML, and that inner HTML can be pretty much everything that can be themed or rendered in Drupal. Thus, the actual content of the TIME element may be text, a link, but in reality actually anything that is valid HTML within the TIME element (and which can be generated through a plethora of available render elements and theme functions throughout core and contrib).
theme_link() is called too often to account for any #children instead of #text. Unless this is baked into the theme/render system and extremely optimized for high-performance, this won't be possible for theme_link().
The only difference in theme_menu_local_task() to this implementation is that it is using a
'render element'
instead of theme'variables'
in hook_theme(). I already attempted that approach, but it made the preprocessor and theme function way more complex than it is now.Comment #204
Anonymous (not verified) CreditAttribution: Anonymous commentedYes, and since the content model of
<a>
in HTML5 is transparent, it can also contain other HTML elements as long as they aren't interactive.Then shouldn't we do an analysis of how often this theme function will be called and whether there is a performance impact?
Does any other theme function use a 'children' variable? Just because our system can handle it doesn't mean that it provides a consistent API for the developer.
To conclude, I don't think that we should introduce a completely new pattern for this theme function. We should either make it fit an existing pattern, or figure out if it is part of a class of theme functions that do the same thing and make them all work the same way.
Comment #205
Sheldon Rampton CreditAttribution: Sheldon Rampton commentedI just got a request that would require me to change the output generated by Drupal's core format_interval() function. Instead of concatenating interval units by putting a space between them, I was asked to put a comma followed by a space, e.g., "2 weeks, 3 days" instead of "2 weeks 3 days". Right now the only way to do that would require hacking core, so I'm thinking the format_interval function should be themeable. It doesn't seem that the patch shown here to add a theme_datetime() function addresses this. Should I file a separate ticket?
Comment #206
JacineOn second thought, setting this back to RTBC on the patch in #188.
I want to see a separate issue for handling #children and a discussion, and I'm fed up with HMTL5 issues being held hostage to a standard of perfection that does not exist and being bikeshedded to the point where people don't even want to participate (myself included).
Comment #207
jessebeach CreditAttribution: jessebeach commentedI added a task to address the render system update of this theme function once it is committed.
#1400214: Rationalize theme functions to improve consistency, including render elements
I also added the issue to our tracking page:
[#1193054]
Comment #208
sunAside from the 'children' theme variable you're criticizing, #196 contains a full range of improvements and actually fixes, as precisely mentioned in that comment.
Thus, needs work for removing the corresponding lines around 'children' in common.inc and '#children' in theme.inc.
Comment #209
sunRe-rolled without those lines.
Comment #210
aspilicious CreditAttribution: aspilicious commentedOk done, commit, close, followup :)
Comment #211
catchAlright. Committed/pushed to 8.x.
This needs a change notification for the new theme function, if follow-ups need to be opened let's open them too.
Comment #212
jessebeach CreditAttribution: jessebeach commentedAwesome, this frees up work on the aggregator templates. Thanks!
Comment #213
xjmTagging.
Comment #214
nicxvan CreditAttribution: nicxvan commentedComment #215
TransitionKojo CreditAttribution: TransitionKojo commentedChange notification in process at http://drupal.org/node/1441334.
Comment #216
TransitionKojo CreditAttribution: TransitionKojo commentedSun, can you and anyone else who worked on the patch review the Change Notice? It's at http://drupal.org/node/1441334. Also, some code snippets would be helpful.
Comment #217
xjmThe change notification looks good to me so far. An example of how to use the theme function would probably be helpful, and maybe an example using
format_date()
with one of the new formats?Comment #218
Jacine@Transition thanks for writing the change notice! Would you be willing to add an example as @xjm suggested?
Comment #219
xjmWell, actually, when we added the change notice during office hours we were hoping someone who worked on the issue could review the notice and add an example, because we weren't sure what it should be. :)
Comment #220
Anonymous (not verified) CreditAttribution: Anonymous commentedBased on the office hours discussion today, Transition will work on this and I will review it.
Comment #221
sunRevised. Fixed.
Comment #222
sunmeh.
Comment #223
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #224
JacineRemoving the sprint tag :)
Comment #225.0
(not verified) CreditAttribution: commentedReordered lis