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 is check_plained 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.

Files: 
CommentFileSizeAuthor
#209 drupal8.datetime.209.patch9.22 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,117 pass(es).
[ View ]
#196 drupal8.datetime.196.patch10.64 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,380 pass(es).
[ View ]
#191 drupal8.datetime.191.patch10.03 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,251 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#188 1183250-theme_datetime-188.patch9.48 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 34,253 pass(es).
[ View ]
#186 1183250-theme_datetime-186.patch9.7 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 1183250-theme_datetime-186.patch.
[ View ]
#184 1183250-theme_datetime-184.patch9.73 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 1183250-theme_datetime-184.patch.
[ View ]
#181 1183250-theme_datetime-181.patch9.69 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 34,174 pass(es).
[ View ]
#176 1183250-theme_datetime-176.patch9.74 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 34,180 pass(es).
[ View ]
#169 1183250-theme_datetime-168.patch9.76 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 34,338 pass(es).
[ View ]
#165 introduce_theme_datetime-1183250-164-D8.patch5.9 KBlinclark
#148 introduce_theme_datetime-1183250-148.patch6.87 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 33,799 pass(es).
[ View ]
#146 introduce_theme_datetime-1183250-146.patch6.4 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 33,809 pass(es).
[ View ]
#145 introduce_theme_datetime-1183250-145.patch6.49 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 33,805 pass(es).
[ View ]
#137 introduce_theme_datetime-1183250-137.patch5.92 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 33,805 pass(es).
[ View ]
#131 introduce_theme_datetime-1183250-131.patch6.54 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 33,761 pass(es).
[ View ]
#130 introduce_theme_time-1183250-130.patch6.48 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 33,754 pass(es).
[ View ]
#125 introduce_theme_time-1183250-125.patch7.17 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 33,774 pass(es).
[ View ]
#123 introduce_theme_time-1183250-122.patch7.71 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 33,766 pass(es).
[ View ]
#122 introduce_theme_time-1183250-122.patch7.74 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 33,754 pass(es).
[ View ]
#119 introduce_theme_time-1183250-119.patch7.64 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch introduce_theme_time-1183250-119_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#109 introduce_theme_time-1183250-108.patch5.39 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 33,658 pass(es).
[ View ]
#92 introduce_theme_time-1183250-92.patch6.06 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 33,626 pass(es).
[ View ]
#85 introduce_theme_time-1183250-85.patch10.19 KBjessebeach
FAILED: [[SimpleTest]]: [MySQL] 33,615 pass(es), 0 fail(s), and 1 exception(es).
[ View ]
#84 introduce_theme_time-1183250-84.patch10.2 KBjessebeach
FAILED: [[SimpleTest]]: [MySQL] 33,628 pass(es), 0 fail(s), and 1 exception(es).
[ View ]
#72 introduce_theme_datetime-1183250-72.patch9.67 KBjessebeach
FAILED: [[SimpleTest]]: [MySQL] 33,434 pass(es), 0 fail(s), and 1 exception(es).
[ View ]
#71 introduce_theme_datetime-1183250-71.patch9.7 KBjessebeach
FAILED: [[SimpleTest]]: [MySQL] 33,316 pass(es), 0 fail(s), and 1 exception(es).
[ View ]
#68 introduce_theme_datetime-1183250-68.patch9.76 KBjessebeach
FAILED: [[SimpleTest]]: [MySQL] 33,321 pass(es), 0 fail(s), and 1 exception(es).
[ View ]
#67 introduce_theme_datetime-1183250-67.patch6.12 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 33,315 pass(es).
[ View ]
#45 theme_datetime-1183250-14.patch5.1 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 32,926 pass(es).
[ View ]
#40 1183250_13_theme_datetime.patch4.6 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 32,947 pass(es).
[ View ]
#39 1183250_12_theme_datetime.patch4.56 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 32,935 pass(es).
[ View ]
#31 1183250_11_theme_datetime.patch4.92 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 33,651 pass(es).
[ View ]
#30 1183250_10_theme_datetime.patch4.91 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 29,936 pass(es).
[ View ]
#21 1183250_9_theme_datetime.patch4.87 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 33,646 pass(es).
[ View ]
#19 1183250_8_theme_datetime.patch4.74 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 33,636 pass(es).
[ View ]
#17 1183250_7_theme_datetime.patch4.52 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 33,638 pass(es).
[ View ]
#9 1183250_6_theme_datetime.patch4.52 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 33,653 pass(es).
[ View ]
#5 1183250_5_theme_datetime.patch1.75 KBscor
PASSED: [[SimpleTest]]: [MySQL] 33,575 pass(es).
[ View ]
#1 theme_datetime-1.patch1.73 KBEverett Zufelt
PASSED: [[SimpleTest]]: [MySQL] 30,008 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.73 KB
PASSED: [[SimpleTest]]: [MySQL] 30,008 pass(es).
[ View ]

Prototype

+++ b/includes/theme.inc
@@ -1312,6 +1312,36 @@ function theme_disable($theme_list) {
+  $output .= ' datetime="' . format_date($variables['timestamp'], 'custom', 'Y-m-d H:i') . '">';

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

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

A valid date-time as defined in [RFC 3339], with these additional qualifications:

  • the literal letters T and Z in the date/time syntax
    must always be uppercase
  • the date-fullyear production is instead defined as four or more digits representing a number greater than 0

Coming 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:

<time datetime="2009-11-13T20:00+00:00">8pm on my birthday</time>

And here is the code from a page that uses date_iso8601:

<span property="dc:date dc:created" content="2011-07-18T22:53:34+01:00">

The only difference I see is that date_iso8601 includes seconds.

StatusFileSize
new1.75 KB
PASSED: [[SimpleTest]]: [MySQL] 33,575 pass(es).
[ View ]

yes, 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

It's succinct and clear. I say we set to RTBC. I'd love to start using this in the template update work.

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

Status:Needs review» Needs work

Yeah, 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++

StatusFileSize
new4.52 KB
PASSED: [[SimpleTest]]: [MySQL] 33,653 pass(es).
[ View ]

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

+++ b/includes/theme.incundefined
@@ -1312,6 +1312,87 @@ function theme_disable($theme_list) {
+ *   - offset (optional): A Unix style time from or until the timestamp for printing ¶

trailing whitespace

+++ b/includes/theme.incundefined
@@ -1312,6 +1312,87 @@ function theme_disable($theme_list) {
+    // Prepare the interval labels. ¶

trailing whitespace

+++ b/includes/theme.incundefined
@@ -1312,6 +1312,87 @@ function theme_disable($theme_list) {
+  } ¶

trailing whitespace

+++ b/includes/theme.incundefined
@@ -1312,6 +1312,87 @@ function theme_disable($theme_list) {
+  ¶

trailing whitespace

20 days to next Drupal core point release.

Minor but it has to be fixed

This 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:

A valid date-time as defined in [RFC 3339], with these additional qualifications:

• the literal letters T and Z in the date/time syntax must always be uppercase
• the date-fullyear production is instead defined as four or more digits representing a number greater than 0
( http://dev.w3.org/html5/markup/datatypes.html#common.data.date-or-time )

@aspilicious, had you meant to post an updated patch?

@Everett Zufelt, I think iso8601 is acceptable here. From the doc you linked to:

Numeric offsets are calculated as "local time minus UTC". So the equivalent time in UTC can be determined by subtracting the offset from the local time. For example, 18:50:00-04:00 is the same time as 22:50:00Z. (This example shows negative offsets handled by adding the absolute value of the offset.)

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

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

<?php
  $vars
['datetime_rfc'] = date("r" , time()); // RFC2822 PHP "r"
 
$vars['datetime_iso'] = date("c" , time()); // ISO 8601 PHP "c"
 
$vars['datetime_iso2'] = gmdate('Y-m-d\TH:i:s\Z'); // See http://www.sitepoint.com/whats-the-best-date-format/
?>

Produces:

RFC:  Thu, 11 Aug 2011 23:59:56 +0200 // RFC2822 PHP "r"
ISO:  2011-08-11T23:59:56+02:00 // ISO 8601 PHP "c"
ISO2: 2011-08-11T21:59:56Z // sitepoint example

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.

Hey 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

Status:Needs work» Needs review
StatusFileSize
new4.52 KB
PASSED: [[SimpleTest]]: [MySQL] 33,638 pass(es).
[ View ]

Removed whitespace as noted by aspilicious.

No other changes from the v6 patch.

jacine, the updated patch does use format_interval(). Were you thinking we use it in a different way?

StatusFileSize
new4.74 KB
PASSED: [[SimpleTest]]: [MySQL] 33,636 pass(es).
[ View ]

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

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

StatusFileSize
new4.87 KB
PASSED: [[SimpleTest]]: [MySQL] 33,646 pass(es).
[ View ]

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

Cool. Thanks ;) Sounds fine to me.

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

+++ b/includes/theme.incundefined
@@ -1312,6 +1312,92 @@ function theme_disable($theme_list) {
+ *   - offset (optional): A Unix style time from or until the timestamp for printing
+ *     the time element text as an interval.
+ *   - format_type (optional): The date format type to be used for the displayed date or
+ *     time, e.g. 'short', 'medium'.
+ *   - custom (optional): The custom formatter to be used if 'format_type' is set to
+ *     'custom'.
+ *   - 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.
+ *   - labels (optional): An associative array containing the optional properties:

Dreditor tells me these lines passes 80 characters.

+++ b/includes/theme.incundefined
@@ -1312,6 +1312,92 @@ function theme_disable($theme_list) {
+  // We use gmdate() with Z for the UTC value so that the datestamp remains constant ¶

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.

+ * - 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.

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

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

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

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

Issue tags:-html5
StatusFileSize
new4.91 KB
PASSED: [[SimpleTest]]: [MySQL] 29,936 pass(es).
[ View ]

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

StatusFileSize
new4.92 KB
PASSED: [[SimpleTest]]: [MySQL] 33,651 pass(es).
[ View ]

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

Fixing the tag. Was wondering what happened to this. ;)

Assigned:Everett Zufelt» Unassigned

Oh, and sorry for the inbox spam, but un-assigning Everett. Jesse, feel free to assign yourself if you'd like. ;)

Assigned:Unassigned» jessebeach

I wish we had something like a multi-value field for people that participated in an issue.

Status:Needs review» Needs work

+ * - 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?

pubdate isn't a boolean value but a boolean attribute... I had to look up what this meant for microdata a while ago.

A number of attributes are boolean attributes. The presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value.

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?

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

Agreed the output should be <time pubdate="pubdate">.

I'll get this patch updated today, incorporating #36 and #37.

Status:Needs work» Needs review
StatusFileSize
new4.56 KB
PASSED: [[SimpleTest]]: [MySQL] 32,935 pass(es).
[ View ]

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

'attributes' => array(
  'pubdate' => 'pubdate',
),

I removed mention of pubdate from the patch.

StatusFileSize
new4.6 KB
PASSED: [[SimpleTest]]: [MySQL] 32,947 pass(es).
[ View ]

Just realized that the comments exceed 80 characters and I had one line with a trailing whitespace character. Fixed in this new patch.

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.

I'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?

Status:Needs review» Needs work

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

Also href isn't listed in $variables = array(..), but is in the doc block.

scor, 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

The time element represents either a time on a 24 hour clock, or a precise date in the proleptic Gregorian calendar, optionally with a time and a time-zone offset.

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.

StatusFileSize
new5.1 KB
PASSED: [[SimpleTest]]: [MySQL] 32,926 pass(es).
[ View ]

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

Status:Needs work» Needs review

Looks good to me, let's see what the bots think.

I'm also wondering if we need to add any style rules to this in Core? Do browsers apply any default styling to this?

Chrome (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...

Subscribing for review. Woah nelly this function is doing a lot! :)

Following up on #49, this function should include some unit tests demonstrating the formatting complexity works now and going forward.

Lars, I've never written a unit test for Drupal. Can you recommend a doc or example that would help me get started?

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

Tagging for the current sprint

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

I 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 :)

Here's another example that might be useful: #98696-47: Various bugs in theme_links()

Status:Needs review» Needs work

Changing to needs work because we still need to write tests.

Tagging for the current sprint.

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

What do you propose we move out?

Everything associated with determining interval / interval text.

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

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

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

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

Assigned:jessebeach» Unassigned

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

StatusFileSize
new6.12 KB
PASSED: [[SimpleTest]]: [MySQL] 33,315 pass(es).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new9.76 KB
FAILED: [[SimpleTest]]: [MySQL] 33,321 pass(es), 0 fail(s), and 1 exception(es).
[ View ]

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

Status:Needs review» Needs work

great progress here @jessebeach!

+++ b/includes/theme.inc
@@ -1447,6 +1447,62 @@ function theme_disable($theme_list) {
+  // The formatted timestamp for humans. The text variable might be set in
+  // template_process_datetime if a time interval is provided.

to make this comment more useful for themers, you might want to mention the hook that themers can implement to alter this text.

+++ b/includes/theme.inc
@@ -1447,6 +1447,62 @@ function theme_disable($theme_list) {
+  if (empty($variables['text'])) {
+    // Set defaults for the format_date() function
+    $options = $variables['format_options'];
+    $options += array(
+      'type' => '',
+      'format' => '',
+      'timezone' => NULL,
+      'langcode' => NULL,
+    );
+    $variables['text'] = format_date($variables['timestamp'], $options['type'], $options['format'], $options['timezone'], $options['langcode']);

isn't that something that could be moved outside the theme function? or did you have a reason for leaving it here?

+ *   - gmdate_format (optional): The format for the Unix style timestamp passed

(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).

Status:Needs work» Needs review

StatusFileSize
new9.7 KB
FAILED: [[SimpleTest]]: [MySQL] 33,316 pass(es), 0 fail(s), and 1 exception(es).
[ View ]

@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 of theme_datetime; they're parameters of format_date. My thinking was to make theme_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.

Assigned:Unassigned» jessebeach
StatusFileSize
new9.67 KB
FAILED: [[SimpleTest]]: [MySQL] 33,434 pass(es), 0 fail(s), and 1 exception(es).
[ View ]

Found extra whitespace and theme.text was missing a new line at the end of the file.

Status:Needs review» Needs work

The last submitted patch, introduce_theme_datetime-1183250-72.patch, failed testing.

Issue tags:-D8H5+sprint

Thanks for all the work on this @jessebeach. Getting close!

Great progress, jesse!

  1. Unless there is a special reason here, we should switch the function from template_process_datetime to template_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.
  2. RE: scor's point on the options... You don't need to add it as a parameter to theme_datetime if it is added in the preprocess function. The preprocess function will run whenever theme_datetime is called, so you can depend on the defaults being added. Many preprocess functions add variables for the theme function which don't take any input from the call to theme('hook'), so don't have any additional parameters listed.

re: 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 correctly incorrectly. _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.

Here are some observations

+++ b/includes/theme.incundefined
@@ -1447,6 +1447,62 @@ function theme_disable($theme_list) {
+  if (!empty($variables['attributes'])) {
+    $output .= drupal_attributes($variables['attributes']);
+  }
+  $output .= '>';

I don't think we need to check if its empty. Being that the default is an array.

+++ b/includes/theme.incundefined
@@ -1447,6 +1447,62 @@ function theme_disable($theme_list) {
+  $output .= ' datetime="' . gmdate($variables['gmdate_format'], $variables['timestamp']) . '"';

This can be set up as an attribute.

So then all this can be simplified into

'<time' . drupal_attributes($variable['attributes']) . '>';

-16 days to next Drupal core point release.

Status:Needs work» Needs review

@ericduran, see comment #21 about moving the datetime attribute into the attributes array. $variables['timestamp'] is not just used for the datetime attribute; it's also used to generate the human-readable text for the <time> element.

Plus, $variables['timestamp'] needs to be run through gmdate() 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.

Status:Needs review» Needs work

The last submitted patch, introduce_theme_datetime-1183250-72.patch, failed testing.

+++ b/includes/common.inc
@@ -6438,6 +6438,9 @@ function drupal_common_theme() {
+    'datetime' => array(
+++ b/includes/theme.inc
@@ -1447,6 +1447,62 @@ function theme_disable($theme_list) {
+function theme_datetime($variables) {
+  $output = '<time';

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

+++ b/includes/common.inc
@@ -6438,6 +6438,9 @@ function drupal_common_theme() {
+      'variables' => array('timestamp' => NULL, 'gmdate_format' => 'Y-m-d\TH:i:s\Z', 'text' => NULL, 'format_options' => array(), 'attributes' => array(), 'href' => NULL),
+++ b/includes/theme.inc
@@ -1447,6 +1447,62 @@ function theme_disable($theme_list) {
+    $options = $variables['format_options'];
+    $options += array(
+      'type' => '',
+      'format' => '',
+      'timezone' => NULL,
+      'langcode' => NULL,
+    );
+    $variables['text'] = format_date($variables['timestamp'], $options['type'], $options['format'], $options['timezone'], $options['langcode']);

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().

+++ b/includes/theme.inc
@@ -1447,6 +1447,62 @@ function theme_disable($theme_list) {
+ * Returns html for a date / time.

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.

+++ b/includes/theme.inc
@@ -1447,6 +1447,62 @@ function theme_disable($theme_list) {
+  $output .= ' datetime="' . gmdate($variables['gmdate_format'], $variables['timestamp']) . '"';
+  if (!empty($variables['attributes'])) {
+    $output .= drupal_attributes($variables['attributes']);

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.

+++ b/includes/theme.inc
@@ -1447,6 +1447,62 @@ function theme_disable($theme_list) {
+  if (empty($variables['text'])) {

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. :)

+++ b/includes/theme.inc
@@ -2563,6 +2619,56 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+ * The offset value is used to calculate a time interval from the timestamp.
+ * The labels are used to indicate if the interval represents how long in the
+ * past the timestamp event occurred or when in the future the timestamp event
+ * will occur. If the offset and the timestamp are equivalent, the label
+ * indicates that the timestamp has happened now.
...
+ *   - offset (optional): A Unix style time from or until the timestamp for
+ *     printing the time element text as an interval
...
+    // Calculate the time interval.
+    $interval = $variables['timestamp'] - $variables['offset'];

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

+++ b/includes/theme.inc
@@ -2563,6 +2619,56 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+    $past_label =   (!empty($variables['labels']['past'])) ? $variables['labels']['past'] : t('ago');
+    $future_label = (!empty($variables['labels']['future'])) ? $variables['labels']['future'] : t('to go');
...
+        $variables['text'] = format_interval(abs($interval), $granularity, $langcode). ' ' . $past_label;

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.

+++ b/includes/theme.inc
@@ -2563,6 +2619,56 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+    $now_label =    (!empty($variables['labels']['now'])) ? $variables['labels']['now'] : t('just now');
...
+      case ($interval === 0) :
+      default :
+        $variables['text'] = t($now_label);

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.

+++ b/includes/theme.inc
@@ -2563,6 +2619,56 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+    $granularity =  (!empty($variables['labels']['granularity'])) ? $variables['labels']['granularity'] : 2;

Let's remove the custom aligning/indentation of variable values here, please.

+++ b/includes/theme.inc
@@ -2563,6 +2619,56 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+    $langcode =     (!empty($variables['labels']['langcode'])) ? $variables['labels']['langcode'] : NULL;

'langcode' is defined in 'langcode', not in 'labels']['langcode'.

+++ b/includes/theme.inc
@@ -2563,6 +2619,56 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+      case ($interval > 0) :

Not space before the colon.

+++ b/includes/theme.inc
@@ -2563,6 +2619,56 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+        $variables['text'] = format_interval(abs($interval), $granularity, $langcode) . ' ' . $future_label;

I don't understand why abs() is needed here.

+++ b/includes/theme.inc
@@ -2563,6 +2619,56 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+        break;
+      case ($interval < 0) :

There should be a blank line between switch cases, unless a case falls through to the next.

+++ b/includes/theme.inc
@@ -2563,6 +2619,56 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+      case ($interval === 0) :

No need for type-agnostic comparison here.

+++ b/includes/theme.inc
@@ -2563,6 +2619,56 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+      default :

The default is unnecessary, since all conditions are covered.

-17 days to next Drupal core point release.

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

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

Status:Needs work» Needs review
StatusFileSize
new10.2 KB
FAILED: [[SimpleTest]]: [MySQL] 33,628 pass(es), 0 fail(s), and 1 exception(es).
[ View ]

All of @sun's comments in #81 have been addressed except:

  1. offset: This should not have a default. The default if offset is null is to print a timestamp of the exact time. When offset is present, format_interval is called, which renders the text of the 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.
  2. #theme_wrappers: this should be discussed more. Going down the render array route saps some of the usefulness of this function that should really just print a timestamp or interval in a <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.

StatusFileSize
new10.19 KB
FAILED: [[SimpleTest]]: [MySQL] 33,615 pass(es), 0 fail(s), and 1 exception(es).
[ View ]

Argh! Whitespace! I'm using Netbeans. Anyone know of a good way to check for whitespace quickly before posting a patch?

Status:Needs review» Needs work

The last submitted patch, introduce_theme_time-1183250-85.patch, failed testing.

Anyone know of a good way to check for whitespace quickly before posting a patch?

git 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).

+++ b/includes/theme.incundefined
@@ -2563,6 +2591,92 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+  // Get the datetime attribute. If none exists, assume REQUEST_TIME;
+  if (empty($variables['attributes']['datetime'])) {
+    $variables['attributes']['datetime'] = REQUEST_TIME;
+  }
+  // Save the datetime attribute to a variable so that modules can reformat
+  // the datetime attribute value in subsequent process functions.
+  $variables['timestamp'] = $variables['attributes']['datetime'];

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

+++ b/includes/theme.incundefined
@@ -2563,6 +2591,92 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+    $past_label = (!empty($variables['labels']['past'])) ? $variables['labels']['past'] : t('ago');
+    $future_label = (!empty($variables['labels']['future'])) ? $variables['labels']['future'] : t('to go');
+    $granularity = (!empty($variables['granularity'])) ? $variables['granularity'] : 2;

I actually don't see the way that this can be overridden. This variable my have been taken out in an earlier patch.

+++ b/includes/theme.incundefined
@@ -2563,6 +2591,92 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+        $variables['text'] = t(format_interval(abs($interval), $granularity, $langcode) . ' ' . $future_label);
...
+        $variables['text'] = t(format_interval(abs($interval), $granularity, $langcode). ' ' . $past_label);

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.

+++ b/includes/theme.incundefined
@@ -2563,6 +2591,92 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+    if (!isset($variables['format_options'])) {
+      $variables['format_options'] = array();
+    }
+    $variables['format_options'] += array(
+      'type' => '',
+      'format' => '',
+      'timezone' => NULL,
+      'langcode' => NULL,
+    );
+    $variables['text'] = format_date($variables['timestamp'], $variables['format_options']['type'], $variables['format_options']['format'], $variables['format_options']['timezone'], $variables['format_options']['langcode']);
+  }

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.

+++ b/modules/simpletest/tests/theme.testundefined
@@ -451,3 +451,86 @@ class ThemeHtmlTag extends DrupalUnitTestCase {
+      'name' => "Theme HTML time tag",

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.

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

RE: 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.

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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new6.06 KB
PASSED: [[SimpleTest]]: [MySQL] 33,626 pass(es).
[ View ]

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

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

Status:Needs review» Reviewed & tested by the community

This really looks great.

Marking RTBC.

Status:Reviewed & tested by the community» Needs work

+++ b/includes/theme.inc
@@ -1447,6 +1447,45 @@ function theme_disable($theme_list) {
+ *   - timestamp: (optional) The UNIX timestamp for the time. If not provided,
+ *     REQUEST_TIME will be used.

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

+++ b/includes/theme.inc
@@ -1447,6 +1447,45 @@ function theme_disable($theme_list) {
+ *   - text: (optional) The text to display in the <time> element. This will
+ *     default to a formatted date of the datetime attribute value.
+ *   - format_options: (optional) Used to format the text content if the text
+ *     variable is not passed in.
+ *     @see format_date()

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

+++ b/includes/theme.inc
@@ -1447,6 +1447,45 @@ function theme_disable($theme_list) {
+ *   - path: (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.

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.

+++ b/includes/theme.inc
@@ -2563,6 +2602,32 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+  // If there isn't a datetime attribute already set, format the datetime
+  // attribute with gmdate().

As far as I understood the spec so far, the datetime attribute is actually optional and not required. Why are we enforcing it here?

+++ b/includes/theme.inc
@@ -2563,6 +2602,32 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+  if (empty($variables['attributes']['datetime'])) {

empty() excludes 0 and "", whereas these values might be intentional. I think we want to use !isset() here.

+++ b/includes/theme.inc
@@ -2563,6 +2602,32 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+    // Get the timestamp. If none exists, assume REQUEST_TIME;
+    if (empty($variables['timestamp'])) {
+      $variables['timestamp'] = REQUEST_TIME;
+    }

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.

+++ b/includes/theme.inc
@@ -2563,6 +2602,32 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+    $type = isset($variables['format_options']['type']) ? $variables['format_options']['type'] : '';
+    $format = isset($variables['format_options']['format']) ? $variables['format_options']['format'] : '';
+    $timezone = isset($variables['format_options']['timezone']) ? $variables['format_options']['timezone'] : NULL;
+    $langcode = isset($variables['format_options']['langcode']) ? $variables['format_options']['langcode'] : NULL;

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.

I think that a generic TIME theme function should be compatible with all kind of dates; i.e., also historic ones.

This is a really great point. @jessebeach, do you have an idea in mind as to how we handle this?

+++ b/includes/theme.inc
@@ -2563,6 +2602,32 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+    // Get the timestamp. If none exists, assume REQUEST_TIME;
+    if (empty($variables['timestamp'])) {
+      $variables['timestamp'] = REQUEST_TIME;
+    }

http://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.

Title:Add a theme_datetime() funtion to consistently theme dates and timesAdd a theme_time() funtion to consistently theme dates and times

Changing the title.

I am working on this now at the Core sprint.

Assigned:jessebeach» skottler

Title:Add a theme_time() funtion to consistently theme dates and timesAdd a theme_time() function to consistently theme dates and times

Thanks for the thorough review, sun :) It's a little late in Ireland, so just responding to two items that stuck out to me:

As far as I understood the spec so far, the datetime attribute is actually optional and not required. Why are we enforcing it here?

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.

empty() excludes 0 and "", whereas these values might be intentional. I think we want to use !isset() here.

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

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

@Everett Zufelt the main issue is that one: http://www.w3.org/Bugs/Public/show_bug.cgi?id=13240

Thanks 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:

  1. time will probably change to <data>, at which point in time we will need this attribute to provide the time value.
  2. Because the datetime attribute is required unless the time element's textContent is itself a valid, machine readable date.

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.

Agreed w/ @linclark in #105

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

Using 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):

  1. make callers prepend '@' to all timestamps, increasing the complexity for the calling code.
  2. --or--

  3. add logic to test whether the value is a timestamp, increasing the complexity for the function. I believe this can fail for 'Ymd' format.
  4. --or--

  5. add variables for 'timestamp' and 'timestring' (which should not be used in conjunction), slightly increasing the complexity of both the calling code and the function.

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.

thanks for summing that up linclark.

Status:Needs work» Needs review
StatusFileSize
new5.39 KB
PASSED: [[SimpleTest]]: [MySQL] 33,658 pass(es).
[ View ]

I've updated the patch to take into account the feedback.

Curious, why is this a UNIX timestamp?

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.

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?

Agreed. If users really want to configure this and understand how to configure format_date's parameter, then they should just call format_date and pass in the configured value as $variables['text']. We can then use format_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'].

it's not really clear to me why a TIME element asks for or supports a "path" to link to.

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.

datetime attribute is actually optional and not required. Why are we enforcing it here?

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.

empty() excludes 0 and "", whereas these values might be intentional. I think we want to use !isset() here.

Addressed in #102.

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.

Agreed. I don't see a precedent for this.

It looks like these fallback values should be the default values for the variables in the theme registry.

No longer necessary since $format_options were removed.

Love it. Let's print it.

Status:Needs review» Needs work

http://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?

Status:Postponed» Needs work

Well 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:

<data itemprop="datePublished" value="2009-10-09">

<data itemprop=""commentTime" value="2009-10-10">">

<data itemprop="birthday" value="2009-05-10">

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.

Status:Needs work» Postponed

http://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.

Status:Needs work» Postponed

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

Title:Add a theme_time() function to consistently theme dates and timesAdd a theme_time() function to consistently theme times

Status:Postponed» Needs work

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

Thnx for the clarification linclark :)

Assigned:skottler» Unassigned

StatusFileSize
new7.64 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch introduce_theme_time-1183250-119_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, introduce_theme_time-1183250-119.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.74 KB
PASSED: [[SimpleTest]]: [MySQL] 33,754 pass(es).
[ View ]

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

StatusFileSize
new7.71 KB
PASSED: [[SimpleTest]]: [MySQL] 33,766 pass(es).
[ View ]

Gah! no new line at end of file.

Assigned:Unassigned» linclark
Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new7.17 KB
PASSED: [[SimpleTest]]: [MySQL] 33,774 pass(es).
[ View ]

This patch outputs a <data> element with a value attribute. For example:

<data value="1978-11-19">Dries's birthday</data>
  1. Allows the user to pass in a UNIX timestamp in $variables['timestamp'] or a valid date string with optional time in $variables['attributes']['value'].
  2. Defines the 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'].
  3. Configures the human-readable text content with $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".

Status:Needs review» Needs work

Thanks 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. ;)

+++ b/includes/theme.incundefined
@@ -2582,6 +2617,41 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+  // A value attribute must always be present on the <data> element. If no

Minor nitpick, but most everywhere else in this patch 'value' is in quotes except here.

+++ b/includes/theme.incundefined
@@ -1460,6 +1460,41 @@ function theme_disable($theme_list) {
+  $attributes = drupal_attributes($variables['attributes']);
+  $text = $variables['text'];
+

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?

$output = '<tag' . drupal_attributes($variables['attributes_array']) . '>...</tag>';

+++ b/includes/theme.incundefined
@@ -2582,6 +2617,41 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+  // If no text for the <data> element was provided, print a human readable
+  // version of the 'value' attribute.
+  if (!isset($variables['text'])) {
+    $text_type = $variables['text_type'];
+    try {
+      $date = new DateTime($variables['attributes']['value']);
+      $variables['text'] = format_date($date->getTimestamp(), $text_type);
+    }
+    catch (Exception $e) {
+      // If the date isn't properly formed, log it and set the text to an
+      // empty string.
+      watchdog_exception('theme', $e);
+      $variables['text'] = '';
+    }

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

+++ b/modules/simpletest/tests/theme.testundefined
@@ -488,3 +488,80 @@ class ThemeHtmlTplPhpAttributesTestCase extends DrupalWebTestCase {
+  function testThemeTime() {

Should rename this.

The documentation is looking really good though, and I agree that it's easier to grok than previous patches.

+++ b/includes/theme.incundefined
@@ -2582,6 +2617,41 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+    $text_type = $variables['text_type'];

The $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.

That's because a default is set in declaration of the theme function. If the user doesn't set it, it defaults to 'medium'.

@linclark ha, yea. I see that now.

Title:Add a theme_time() function to consistently theme timesAdd a theme_datetime() function to consistently theme dates and datetimes
Status:Needs work» Needs review
StatusFileSize
new6.48 KB
PASSED: [[SimpleTest]]: [MySQL] 33,754 pass(es).
[ View ]

Thanks Jacine and ericduran for the reviews.

  1. Fixed the references to theme_time and template_preprocess_time.
  2. Fixed quotes.
  3. Good catch. This should really be attributes_array. Moved it inline as well.
  4. That fancy logic had a silly bug in it anyway, so wasn't doing what I meant it to do. What it was supposed to do was allow us to format a human readable value for dates that can't use timestamps because they are outside the UNIX epoch (1970-2038). I thought about the ways to approach this, but none of them are compelling, so I removed the logic. If a caller wants to use this function to format a date outside the UNIX epoch, they will have to set both $variables['attributes']['value'] and $variables['text']. This is a very limited use case so I don't think that is a problem.
  5. Test renamed.

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

StatusFileSize
new6.54 KB
PASSED: [[SimpleTest]]: [MySQL] 33,761 pass(es).
[ View ]

Patch updated for core directory, hope it worked properly.

I offered to try and review this. However, it could really use an issue summary. :) Tagging.

Webchick, I added an issue summary. Also, this is what the patch does

This patch outputs a <data> element with a value attribute. For example:

<data value="1978-11-19">Dries's birthday</data>
  1. Allows the user to pass in a UNIX timestamp in $variables['timestamp'] or a valid date string with optional time in $variables['attributes']['value'].
  2. Defines the 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'].
  3. Configures the human-readable text content with $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.

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new5.92 KB
PASSED: [[SimpleTest]]: [MySQL] 33,805 pass(es).
[ View ]

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

  1. timestamp
  2. timestamp with text
  3. datetime attribute
  4. datetime attribute with text

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?

Oh, @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):

// Only use non-hook-specific variable processors for theming hooks
// implemented as templates. See theme().

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

Agreed w/ @Jacine that this is looking very good.

+ *   An associative array containing:
+ *   - timestamp: (optional) A UNIX timestamp for the datetime. If the
+ *     datetime cannot be represented as a UNIX timestamp, instead use a valid
+ *     date string with optional time in $variables['attributes']['datetime']

What is a 'valid date string'? Can we provide a URL to the W3C definition?

+      // Format the timestamp as a gmdate.
+      $variables['attributes']['datetime'] = gmdate('Y-m-d\TH:i:s\Z', $variables['timestamp']);

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.

Status:Needs review» Needs work

@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 had check_plain, but sun pointed out that <time> can contain more than just plaintext, so probably should use check_markup.

The comment for $variables['text'] needs to change based on the code changes. Something like

- text: (optional) The text content to display in the <time> element. Use
  this variable to provide a formatted value for the text content (for
  example, using format_date()).

I felt we should either provide hooks for all 4 or just use the defaults for the 3 formatting parameters beyond the timestamp.

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

I removed attributes_array array and just pass through the attributes array.

On attributes_array, I think that we should still have it as an attributes_array variable in the preprocess function for consistency with other preprocess functions. Since it is attributes_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 through drupal_attributes, but I think that's also clear when you try to print an array. @Jacine, I'll defer to your opinion on that.

What is a 'valid date string'? Can we provide a URL to the W3C definition?

Here's the W3C url: http://www.w3.org/TR/html5/common-microsyntaxes.html#valid-date-string-w...

Can we add that this is a valid date string, i.e. why are we using this particular format?

Agree that the comment should mention this.

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

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 through drupal_attributes, but I think that's also clear when you try to print an array. @Jacine, I'll defer to your opinion on that.

Yeah...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. ;)

+++ b/core/includes/theme.inc
@@ -1460,6 +1460,29 @@ function theme_disable($theme_list) {
+ *   - timestamp: (optional) A UNIX timestamp for the datetime. If the
+ *     datetime cannot be represented as a UNIX timestamp, instead use a valid
+ *     date string with optional time in $variables['attributes']['datetime']

What is a "valid date string"? Valid in which context or for what?

"a date string with optional time in $... valid for gmdate(), for example '...'."

+++ b/core/includes/theme.inc
@@ -2582,6 +2605,31 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+function template_preprocess_datetime(&$variables) {

Let's move the preprocess function right above the theme function in the code.

+++ b/core/includes/theme.inc
@@ -2582,6 +2605,31 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+      if (empty($variables['text'])) {
...
+  if (empty($variables['text'])) {

Should be !isset()

Status:Needs work» Needs review
StatusFileSize
new6.49 KB
PASSED: [[SimpleTest]]: [MySQL] 33,805 pass(es).
[ View ]

@Everett (#140)

I addressed your comments.

@linclark (#141)

  1. If <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.
  2. Good catch with the xss check. I added a call to filter_xss on the text variable instead of check_markup. check_markup runs text filters and inserts HTML, such as a <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?

StatusFileSize
new6.4 KB
PASSED: [[SimpleTest]]: [MySQL] 33,809 pass(es).
[ View ]

I moved the filter_xss call from template_preprocess_datetime to theme_datetime. Otherwise this patch is the same as #145.

Status:Needs review» Needs work

I added a call to filter_xss on the text variable instead of check_markup. check_markup runs text filters and inserts HTML, such as a

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.

filter_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().

Status:Needs work» Needs review
StatusFileSize
new6.87 KB
PASSED: [[SimpleTest]]: [MySQL] 33,799 pass(es).
[ View ]

Added $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.

Looks good to me. Great work.

Status:Needs review» Needs work

Ugh, 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

<time itemprop="startDate" datetime="2011-12-03T14:00:00Z">December 3,
9:00am</time>

However, we would want it to be output as:
<time itemprop="startDate" datetime="2011-12-03T09:00:00-05:00">December 3,
9:00am</time>

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

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:

1979-11-19

A valid date with time is
1979-11-19T13:15:30+01:00

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

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 element either AFAIK).

Ah, 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?

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

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

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

The 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:

  • valid month string
  • valid date string
  • valid yearless date string
  • valid time string
  • valid local date and time string
  • valid time-zone offset string
  • valid global date and time string
  • valid week string
  • valid non-negative integer representing a year
  • valid duration string

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.

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

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

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

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

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

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

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

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

Status:Needs work» Postponed

I'm setting this issue to postpone. I feel like we're stuck in a loop.

Status:Postponed» Needs review
StatusFileSize
new5.9 KB

As 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:

  • Uses an 'options' parameter which contains the 'attributes' and 'html' variables, which follows the 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.
  • Uses a format_html_datetime() instead of gmdate() to format the datetime attribute. This format_html_datetime function enumerates the different valid values for the datetime attribute. It takes a timestamp and a type and outputs the valid value. However, the theme function doesn't allow you to pass in the format type that you want. Anyone who wants a specially formatted datetime has to use format_html_datetime before calling theme_datetime and pass in the value in $variables['options']['attributes']['datetime'].

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.

As 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:

  • time: Time in HH:MM:SS format.
  • date: A date in a particular year, i.e. 1978-11-18.
  • yearless_date: A date with no year, i.e. 11-18.
  • week: A week in a particular year, i.e. 1978-W46.
  • month: A month in a particular year, i.e. 1978-11.
  • year: A year, i.e. 1978.
  • duration: A time interval

Status:Needs review» Needs work

Bumping this back down to needs work, will be posting another patch today.

Yay! I'll make time to review.

Status:Needs work» Needs review
StatusFileSize
new9.76 KB
PASSED: [[SimpleTest]]: [MySQL] 34,338 pass(es).
[ View ]

The attached patch adds the following formats to format_date():

  • 'html_datetime',
  • 'html_time'
  • 'html_date'
  • 'html_yearless_date'
  • 'html_week',
  • 'html_month'
  • 'html_year'

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 refactor format_interval() to handle that one. We can file another issue for that support.

Issue summary:View changes

Issue summary for reviewer.

Issue summary:View changes

Update for patch at #169.

Status:Needs review» Needs work

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

+++ b/core/includes/common.incundefined
@@ -1903,6 +1905,34 @@ function format_date($timestamp, $type = 'medium', $format = '', $timezone = NUL
+      $format = variable_get('date_format_html_datetime', 'Y-m-d\TH:i:sO');

Looking 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

+++ b/core/modules/simpletest/tests/common.testundefined
@@ -2361,6 +2361,15 @@ class FormatDateUnitTest extends DrupalWebTestCase {
+    $this->assertIdentical(format_date($timestamp, 'html_datetime'), '2007-03-25T17:00:00-0700', t('Test html_datetime date format.'));

This would become 2007-03-25T17:00:00-07:00

+++ b/core/modules/simpletest/tests/theme.testundefined
@@ -490,3 +490,70 @@ class ThemeHtmlTplPhpAttributesTestCase extends DrupalWebTestCase {
+    $this->assertEqual('<time datetime="1978-11-19T00:00:00+0000">' . $date . '</time>', theme('datetime', $variables), t('Test theme_datetime with timestamp.'));

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.

Minor thing: some comment lines are over 80 char long:
      // See http://www.w3.org/TR/html5-author/the-time-element.html#attr-time-datetime

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

Agreed with @linclark's proposed changes.

I'm sold.

Status:Needs work» Needs review
StatusFileSize
new9.74 KB
PASSED: [[SimpleTest]]: [MySQL] 34,180 pass(es).
[ View ]

Ok, so this replaces three empty's with !isset and also removes the duplicate check_plain that catch mentioned to jacine... and by-golly, I think that might just be it :)

Status:Needs review» Reviewed & tested by the community

Awesome! Looks great, meets the spec, passes tests, makes Drupal 8 better.

This is great - looks good to me.

+    debug(format_date($timestamp, 'html_datetime'));

that looks like an oops. :)

Status:Reviewed & tested by the community» Needs work

<?php
+    debug(theme('datetime', $variables));
?>

is that debug call meant to still be in there?

Status:Needs work» Needs review
StatusFileSize
new9.69 KB
PASSED: [[SimpleTest]]: [MySQL] 34,174 pass(es).
[ View ]

DARNIT!!!

New patch, hopefully no oopsies this time.

Status:Needs review» Reviewed & tested by the community

Yay! 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

Status:Reviewed & tested by the community» Needs work

+    debug(theme('datetime', $variables));

Still there ;)

Status:Needs work» Needs review
StatusFileSize
new9.73 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 1183250-theme_datetime-184.patch.
[ View ]

I. Don't. Want. To. Steal. Credit

Status:Needs review» Needs work

The last submitted patch, 1183250-theme_datetime-184.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.7 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 1183250-theme_datetime-186.patch.
[ View ]

Evil issue...

Not sure I fixed something...

Status:Needs review» Needs work

The last submitted patch, 1183250-theme_datetime-186.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.48 KB
PASSED: [[SimpleTest]]: [MySQL] 34,253 pass(es).
[ View ]

AAAAAAH found it, windows endings...

Status:Needs review» Reviewed & tested by the community

#188 is identical to #181 without the debug, so moving back to RTBC.

I'm hoping for this Christmas miracle.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new10.03 KB
FAILED: [[SimpleTest]]: [MySQL] 34,251 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Sorry, 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:

if (isset($variables['markup']) || isset($variables['children'])) {
  $variables['html'] = TRUE;
  if (isset($variables['markup'])) {
    $variables['text'] = $variables['markup'];
  }
  elseif (!isset($variables['markup']) && isset($variables['children'])) {
    $variables['text'] = $variables['children'];
  }
}

Status:Needs review» Needs work

The last submitted patch, drupal8.datetime.191.patch, failed testing.

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

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

+++ b/core/modules/simpletest/tests/theme.testundefined
@@ -540,3 +540,88 @@ class ThemeHtmlTplPhpAttributesTestCase extends DrupalWebTestCase {
+    $build = array(
+      '#theme' => 'timestamp',
+      '#markup' => check_plain("Dries's birthday"),
+      '#timestamp' => $timestamp,
+      '#attributes' => array(
+        'html' => TRUE,
+      ),
+    );
+    $this->assertEqual('<time datetime="1978-11-19T00:00:00+0000">Dries\'s birthday</time>', drupal_render($build));

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

Status:Needs work» Needs review
StatusFileSize
new10.64 KB
PASSED: [[SimpleTest]]: [MySQL] 34,380 pass(es).
[ View ]
  1. Fixed the render element tests.
  2. Removed the 'options' theme variable.
  3. Incorporated the proposal to account for #children in #191.
  4. Fixed partially outdated phpDoc of theme_datetime(), and also to adhere to coding standards.
  5. Cleaned up the code logic in template_preprocess_datetime().

Status:Needs review» Reviewed & tested by the community

Not sure why this wasn't architected this way to begin with...

But it looks good to me (and miraculously passes tests) so RTBC.

Jumping in at the 11th hour and taking over the patch that people have been working on for ages is just meh.

Not sure why this wasn't architected this way to begin with...

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.

Status:Reviewed & tested by the community» Needs review

We REALLY need to think about this.

theme_link does functionally the same thing as theme_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, but theme_datetime not?

* @param $variables
*   An associative array containing the keys 'text', 'path', and 'options'. See
*   the l() function for information about these variables.
*
* @see l()
*/
function theme_link($variables) {
  return '<a href="' . check_plain(url($variables['path'], $variables['options'])) . '"' . drupal_attributes($variables['options']['attributes']) . '>' . ($variables['options']['html'] ? $variables['text'] : check_plain($variables['text'])) . '</a>';
}

To my mind, they should both work the same way.

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

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

  1. theme_link() was designed to be compatible with (and constrained to) the function signature of l()

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

  2. I assumed that the reason theme_link didn't use the Render API was because of performance, since l() is called a lot. If it were desired, a 'children' parameter could have been added to the function signature of l(), so I'm guessing it's not because of the function signature. Does anyone know why theme_link doesn't account for children?
  3. If it does make sense to account for children here, is a 'children' variable the right way to do this? I don't see a 'children' variable in any of the other theme function declarations in common.inc. In that case, shouldn't we change the whole function so it's the same as the rest of the theme functions that use the render API, like theme_menu_local_task?

  1. Note the actual committed patch, which is identical to the current implementation. Any original/earlier version doesn't matter.

    'attributes' and 'html' are common theme variables/arguments; see theme_links() (and others).

  2. theme_link() doesn't decide on its own whether it is used as part of the render system or not. A module, theme, or themer decides whether and how something is output/themed/rendered. The test in this patch nicely clarifies that; it asserts both usage of the plain theme function but also its usage as part of a render element.

    Let me clarify how render elements are rendered:

    1. Render elements can have sub-elements.
    2. Sub-elements are rendered before the parent element.
    3. The output of sub-elements is assigned to the #children property of the parent element.
    4. The parent element may be themed via theme_datetime().
    5. A theme function (#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().

  3. See reasoning for the name "children" above; the name is hard-coded into the render system.

    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.

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

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

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().

Then shouldn't we do an analysis of how often this theme function will be called and whether there is a performance impact?

See reasoning for the name "children" above; the name is hard-coded into the render system.

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.

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

Status:Needs review» Reviewed & tested by the community

On 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).

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

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new9.22 KB
PASSED: [[SimpleTest]]: [MySQL] 34,117 pass(es).
[ View ]

Re-rolled without those lines.

Status:Needs review» Reviewed & tested by the community

Ok done, commit, close, followup :)

Title:Add a theme_datetime() function to consistently theme dates and datetimesChange notification for: Add a theme_datetime() function to consistently theme dates and datetimes
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active

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

Awesome, this frees up work on the aggregator templates. Thanks!

Issue tags:+Needs change record

Tagging.

Assigned:linclark» nicxvan

Assigned:nicxvan» Transition

Change notification in process at http://drupal.org/node/1441334.

Status:Active» Needs review

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

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

@Transition thanks for writing the change notice! Would you be willing to add an example as @xjm suggested?

Well, 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. :)

Based on the office hours discussion today, Transition will work on this and I will review it.

Status:Needs review» Fixed

Revised. Fixed.

Title:Change notification for: Add a theme_datetime() function to consistently theme dates and datetimesAdd a theme_datetime() function to consistently theme dates and datetimes
Issue tags:-Needs issue summary update, -Needs change record

meh.

Priority:Critical» Normal

Issue tags:-sprint

Removing the sprint tag :)

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

Issue summary:View changes

Reordered lis