Problem/Motivation

Similarly to #2034995: Test taxonomy term formatter RDFa output and other fields, we need to support RDFa for the link field formatter. The field formatter needs modification very similar to the taxonomy field formatter (see referenced issue).

Proposed resolution

Update the field formatter and add tests.

Remaining tasks

write patch.

User interface changes

none

API changes

none

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kay_v’s picture

Assigned: Unassigned » kay_v
Status: Active » Needs review
FileSize
1.93 KB

so far just the change to rdf module to put attributes into the href rather than the parent container

ashepherd’s picture

Reviewed changes. Worked for both link and link_separate field formatters. Wrote test: LinkFieldRdfaTest.php, and it passed locally.

Status: Needs review » Needs work

The last submitted patch, 2: 2188889-2-LinkFieldRdfaTest.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
3.71 KB

Combined the patches and fixed a whitespace error.

Status: Needs review » Needs work

The last submitted patch, 4: 2188889-4-LinkFieldRdfaTest.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
scor’s picture

Status: Needs review » Needs work

Great progress here, Jamie!!

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/LinkFieldRdfaTest.php
@@ -0,0 +1,61 @@
+ * Tests the placement of RDFa in email field formatters.

There is a reference to the email field here, it should say "link field".

We should also test some cases where the link has a title (along with the url value), and various field formatter settings, such as 'Add rel="nofollow" to links' or 'Open link in new window'. These should not impact the RDFa, but we should test them anyways. It should be trivial to test with the help of #2203065: Adjust assertFormatterRdfa() parameters to allow for more advanced testing. Feel free to include that patch in this issue for now (commit it locally in your issue branch) while devising the test for the link formatter, once #2203065 is committed, we can remove and RTBC this link formatter issue.

chrisfromredfin’s picture

This incorporates the above comment feedback, as well as the feedback to test using different formatter settings. (In fact, that allowed us to discover that the URL as plain text version of the formatter was not displaying correct RDF, so that is also fixed with this patch).

NOTE: This patch incorporates as part of it the latest patch from #2203065 as well (#6 at the time of this writing), which I manually re-applied.

chrisfromredfin’s picture

Status: Needs work » Needs review

Trigger test bot. Still there is probably comment cleanup to do, etc.

Status: Needs review » Needs work

The last submitted patch, 8: 2188889-8-LinkFieldRdfaTest.patch, failed testing.

chrisfromredfin’s picture

OK, just looks like I'll need to pull in some of the parallel work being done with the date formatter.

chrisfromredfin’s picture

Status: Needs work » Needs review
FileSize
14.91 KB

The tests failed because $item->_attributes isn't necessarily always set, so we need to check to make sure it's non-empty (i.e. we have a mapping defined) before merging in our attributes.

chrisfromredfin’s picture

Status: Needs review » Needs work

The last submitted patch, 12: 2188889-12-LinkFieldRdfaTest.patch, failed testing.

chrisfromredfin’s picture

Status: Needs work » Needs review
FileSize
15.22 KB

Re-rolled, flinging to testbot.

Status: Needs review » Needs work

The last submitted patch, 15: 2188889-15.patch, failed testing.

chrisfromredfin’s picture

FileSize
4.42 KB

Work in progress...

chrisfromredfin’s picture

Status: Needs work » Needs review
FileSize
9.21 KB

More progress, and I know this fails the Link Formatter for internal links, but want to see why...

Status: Needs review » Needs work

The last submitted patch, 18: 2188889-18.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
9 KB

reroll.

lokapujya’s picture

FileSize
9.26 KB
3.51 KB

This should get the tests working. Still needs improvement; Probably the expected value should be set up inside the testAllFormattersX() functions.
Why does assertFormatterRdfa() expect an absolute URL when we pass in a relative URL?

The last submitted patch, 20: 2188889-20.patch, failed testing.

scor’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/rdf/src/Tests/Field/EmailFieldRdfaTest.php
    @@ -53,8 +53,9 @@ public function setUp() {
    -    $this->assertFormatterRdfa(array('type'=>'string'), 'http://schema.org/email', array('value' => $this->testValue));
    +    $this->assertFormatterRdfa(array('type' => 'string'), 'http://schema.org/email', array('value' => $this->testValue));
    +
         // Test the mailto formatter.
    -    $this->assertFormatterRdfa(array('type'=>'email_mailto'), 'http://schema.org/email', array('value' => $this->testValue));
    +    $this->assertFormatterRdfa(array('type' => 'email_mailto'), 'http://schema.org/email', array('value' => $this->testValue));
    

    Those are coding standard fixes, please create a follow up issue to fix those.

  2. +++ b/core/modules/rdf/src/Tests/Field/FieldRdfaTestBase.php
    @@ -74,6 +74,7 @@ public function setUp() {
    +    // Merge in the default value type of "literal" if one wasn't given.
    

    ditto.

  3. +++ b/core/modules/rdf/src/Tests/Field/FieldRdfaTestBase.php
    --- /dev/null
    +++ b/core/modules/rdf/src/Tests/Field/LinkFieldRdfaTest.php
    
    +++ b/core/modules/rdf/src/Tests/Field/LinkFieldRdfaTest.php
    @@ -0,0 +1,175 @@
    +    // change the expected value here to literal. When formatted as plaintext
    +    // then the RDF is expecting a 'literal' not a 'uri'
    

    The coding standards need to be verified throughout LinkFieldRdfaTest.php.

  4. +++ b/core/modules/rdf/src/Tests/Field/LinkFieldRdfaTest.php
    @@ -0,0 +1,175 @@
    +    $this->runTestAllFormatters('External');
    

    Given that those are not UI facing, let's spell the types 'external', 'internal' and 'front' all in lower case.

Why does assertFormatterRdfa() expect an absolute URL when we pass in a relative URL?

The RDFa will still be parsed and extract full URLs, even if those are relative, they will get expanded to match the localhost site.

Probably the expected value should be set up inside the testAllFormattersX() functions.

good idea.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
8.49 KB
6.44 KB

1, 2. Ok, those can be done as part of #1533380: Make RDF module pass Coder Review
3. Adjusted some comments.
4. Done

Moved the expected value to the X() functions.

chrisfromredfin’s picture

Assigned: chrisfromredfin » Unassigned

The patch still applies clean.

scor’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
    @@ -147,6 +147,12 @@ public function viewElements(FieldItemListInterface $items) {
    +        if (!empty($item->_attributes)) {
    +          // Take the fallback behavior of ___ by just setting the
    +          // _attributes directly.
    +          $item->_attributes += array('content' => $item->url);
    

    The comment here should say something like: Piggyback on the metadata attributes which will be placed in the field template wrapper, and set the URL value in a content attribute.

  2. +++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
    @@ -154,6 +160,7 @@ public function viewElements(FieldItemListInterface $items) {
               '#options' => $url->getOptions(),
             );
    +
             if ($url->isExternal()) {
               $element[$delta]['#href'] = $url->getPath();
             }
    

    let's keep the patch small (new line here).

  3. +++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkSeparateFormatter.php
    @@ -77,6 +77,14 @@ public function viewElements(FieldItemListInterface $items) {
    +      if (!empty($item->_attributes)) {
    +        $url->setOption('attributes', $item->_attributes);
    

    Add a comment to say that we're generating a 'a' element here, which is a perfect fit for the RDFa metadata attribute.

  4. +++ b/core/modules/rdf/src/Tests/Field/EmailFieldRdfaTest.php
    @@ -54,6 +54,7 @@ public function setUp() {
       public function testAllFormatters() {
         // Test the plain formatter.
         $this->assertFormatterRdfa(array('type'=>'string'), 'http://schema.org/email', array('value' => $this->testValue));
    +
         // Test the mailto formatter.
         $this->assertFormatterRdfa(array('type'=>'email_mailto'), 'http://schema.org/email', array('value' => $this->testValue));
    

    rogue newline.

  5. +++ b/core/modules/rdf/src/Tests/Field/LinkFieldRdfaTest.php
    @@ -0,0 +1,183 @@
    +    // set up the expected result
    

    s/set/Set and missing period.

chrisfromredfin’s picture

Status: Needs work » Needs review
FileSize
2.41 KB
2.45 KB

Address all review notes in 26.

UPDATE: ignore this, it excludes an entire file.

chrisfromredfin’s picture

FileSize
2.57 KB
7.69 KB

Address all review notes in 26.

scor’s picture

Status: Needs review » Needs work
Issue tags: +Novice
+++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
@@ -147,6 +147,13 @@ public function viewElements(FieldItemListInterface $items) {
+          // Piggyback on the metadata attributes,
+          // which will be placed in the field template wrapper,
+          // and set the URL value in a content attribute.
+          $item->_attributes += array('content' => $item->url);

comments should span to 80 chars

CharuAg’s picture

Status: Needs work » Needs review
FileSize
7.97 KB
3.24 KB

Address comment 29 and other documentation fixes.

scor’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks good now thanks to the recent fixes by Charu. Good to go.

lokapujya’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/rdf/src/Tests/Field/LinkFieldRdfaTest.php
@@ -0,0 +1,197 @@
+    // Test the link formatter: trim at 20, url only (as plaintext.)

The tests are trimming to different lengths. But the url is too short to make a difference.

Also, the assert message is the same for all the tests. If possible, we could change it for each assertion.

CharuAg’s picture

FileSize
8.06 KB
705 bytes

Addressed comment 32

CharuAg’s picture

Status: Needs work » Needs review
scor’s picture

Status: Needs review » Reviewed & tested by the community

URL is now more than 80 charracter long. back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • webchick committed 71e4a64 on 8.0.x
    Issue #2188889 by CharuAg, cwells, lokapujya, ashepherd, kay_v, scor:...
scor’s picture

and congrats @CharuAg for your first patch to core!

CharuAg’s picture

Thanks! :)

Status: Fixed » Closed (fixed)

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