Problem/Motivation

The date field currently hard-codes a dc:date in core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldFormatter/DatetimeDefaultFormatter.php:126. We need to support the RDF mappings defined by core.

Proposed resolution

Change core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldFormatter/DatetimeDefaultFormatter.php to use the proper mappings of core. A good example is the taxonomy field formatter at #1778122-113: Enable modules to inject attributes into field formatters, so that RDF attributes get output.

Remaining tasks

write patch

User interface changes

none

API changes

none

CommentFileSizeAuthor
#17 interdiff.txt962 bytesashepherd
#17 2139551-datime_rdfa-17.patch4.65 KBashepherd
#15 2139551-diff-13-15.txt767 bytesvijaycs85
#15 2139551-datime_rdfa-15.patch4.43 KBvijaycs85
#13 rdf-datetime_rdfa_support-2139551-13.patch4.32 KBchrisfromredfin
#11 rdf-datetime_rdfa_support-2139551-11.patch1.05 KBchrisfromredfin
#9 datetime-rdfa-support-2139551-9.patch1.05 KBdjevans
#5 interdiff-2139551-2-5.txt952 bytesdjevans
#5 datetime-rdfa-support-2139551-5.patch1.07 KBdjevans
#2 datetime-rdfa-support-2139551-2.patch737 bytesdjevans

Comments

djevans’s picture

djevans commented

The simplest thing seems to be to remove the hardcoded reference to dc:date. Adding RDF mappings to datetime fields then adds the @property attribute to the wrapper <div> of each individual <time> element.
This also shouldn't affect any of the property mappings that already exist in core.

djevans’s picture

djevans commented
StatusFileSize
new datetime-rdfa-support-2139551-2.patch737 bytes
scor’s picture

scor commented

I'd rather add the @property inside the time element since the new field formatters in D8 allow that. See how the taxonomy field formatter does it.

djevans’s picture

djevans commented

OK, @property now belongs to the inner <time> element, using a similar approach to the Taxonomy module's LinkFormatter. The comments in template_preprocess_field() also helped.

djevans’s picture

djevans commented
StatusFileSize
new datetime-rdfa-support-2139551-5.patch1.07 KB
new interdiff-2139551-2-5.txt952 bytes
djevans’s picture

djevans commented
Status: Active » Needs review

The last submitted patch, 2: datetime-rdfa-support-2139551-2.patch, failed testing.

The last submitted patch, 5: datetime-rdfa-support-2139551-5.patch, failed testing.

djevans’s picture

djevans commented
StatusFileSize
new datetime-rdfa-support-2139551-9.patch1.05 KB
scor’s picture

scor commented
Issue tags: +RDF code sprint
chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
StatusFileSize
new rdf-datetime_rdfa_support-2139551-11.patch1.05 KB

patch re-rolled... seems like the filename just changed.

Is there other work to do on this in terms of the actual solution, or is the formatter acceptable as-is?

scor’s picture

scor commented
Status: Needs review » Needs work

We will need test for this one as well, much like the other field formatters.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Needs work » Needs review
StatusFileSize
new rdf-datetime_rdfa_support-2139551-13.patch4.32 KB

This patch is the above patch, plus a new test for the datetime_default formatter. TBD is whether or not we need to test the datetime_plain formatter. As of right now it doesn't support RDFa at all, so we're not sure if either (a) the formatter should go away, (b) the formatter should stay as is, and not support RDFa, or (c) we should include support for RDFa in the plain formatter.

Note that it also includes a change to the signature for FieldRdfaTestBase::assertFormatterRdfa() by adding a fifth parameter for $datatype. Probably a better approach here is to pass in $expected_value as an array, rather than take three values in the function signature, but I will leave that to another issue. For now, this is preferred because it won't break other tests.

vijaycs85’s picture

vijaycs85
Location London, UK
commented

13: rdf-datetime_rdfa_support-2139551-13.patch queued for re-testing.

vijaycs85’s picture

vijaycs85
Location London, UK
commented
StatusFileSize
new 2139551-datime_rdfa-15.patch4.43 KB
new 2139551-diff-13-15.txt767 bytes
  1. Not sure how the default case(e.g. dc:date) covered.
  2. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/FieldRdfaTestBase.php
@@ -66,7 +66,7 @@ public function setUp() {
    * @param string $object_type
    *   The object's type, either 'uri' or 'literal'.
    */

    missing new param in docblock. Adding in this patch.

scor’s picture

scor commented
Status: Needs review » Needs work

Not sure how the default case(e.g. dc:date) covered.

The dc:date came from contrib when the date module was first introduced in core. It should be removed since we now use the attributes that come from config (like this patch already does). The following line also need to be removed:

      // @todo How should RDFa attributes be added to this?
      ...
      'datatype' => 'xsd:dateTime',
TBD is whether or not we need to test the datetime_plain formatter. As of right now it doesn't support RDFa at all, so we're not sure if either (a) the formatter should go away, (b) the formatter should stay as is, and not support RDFa, or (c) we should include support for RDFa in the plain formatter.

I've mulled over this for the last couple of weeks and I'm convinced we shouldn't support/worry about this field formatter, I'm not even sure why someone would want to use this field formatter and have RDFa at the same time. I'm not even sure what the purpose of this field formatter is on its own (but that's beyond this issue). Moreover, if someone really want to have RDFa with a similar output as the plain formatter, they could register a custom date format and choose it in the settings of the default date formatter, and get RDFa for free via the time HTML5 element.

ashepherd’s picture

ashepherd commented
Status: Needs work » Needs review
StatusFileSize
new 2139551-datime_rdfa-17.patch4.65 KB
new interdiff.txt962 bytes

Addressed comment #16. Run the datetime test and is passing locally.

scor’s picture

scor commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks Adam, djevans, cwells, vijaycs85 for your work on this patch!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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