Problem/Motivation

On #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality, we are updating all base entity fields in entity views data so that they are using Field API for formatting rather than using generic Views handlers.

This issue is about the aggregator_title_link formatter, which is currently being used for the entity base fields aggregator_item.title and aggregator_field.title

Proposed resolution

Change these two fields to use the Field API formatter 'field' instead of the custom formatter. Should also be able to remove the custom formatter from the code base completely.

Remaining tasks

User interface changes

None.

API changes

Not really.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Assigned: Unassigned » star-szr

I'll try this one.

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Active » Needs review
FileSize
7.04 KB

Here's a start, I hope :)

Status: Needs review » Needs work

The last submitted patch, 2: 2456701-2.patch, failed testing.

jhodgdon’s picture

star-szr’s picture

Status: Needs work » Needs review

Not sure how to proceed here to restore the functionality of linking the aggregator_item title to its link.

In \Drupal\Core\Field\Plugin\Field\FieldFormatter\StringFormatter::viewElements() there is this bit of code:

    $url = NULL;
    // Add support to link to the entity itself.
    if ($this->getSetting('link_to_entity') && ($entity = $items->getEntity()) && $entity->hasLinkTemplate('canonical')) {
      $url = $entity->urlInfo();
    }

However to me that relies on the entity having a canonical link template defined, which aggregator_item does not have, and it seems like aggregator_item cannot have a canonical link template because these are usually linking offsite.

Either way here is an updated patch after seeing #2456713-7: Custom taxonomy field views handler needs to be replaced with generic Field API handler and trying to figure out some patterns. I'm expecting the exact same fails.

star-szr’s picture

FileSize
7.26 KB
3.61 KB

Oh it helps if you attach files.

Edit: And the last one was a crosspost, thanks @jhodgdon!

jhodgdon’s picture

It may be that we need to have a custom handler for the aggregator item title with link. If so, it needs to derive from the 'field' handler, not from Drupal\views\Plugin\views\field\FieldPluginBase as it is currently, so that it uses the entity system for rendering.

star-szr’s picture

Gotcha. That makes sense I think, thanks! :)

Status: Needs review » Needs work

The last submitted patch, 6: 2456701-4.patch, failed testing.

dawehner’s picture

Well, the question is whether we really should solve this problem using code, rather then the built in features of views itself.
What about let people use the link rewrite feature of views and use the actual stored URL as placeholder.

star-szr’s picture

@dawehner indeed, as long as we are okay with the potential of slightly breaking people's D8 sites. Otherwise I like that idea too.

dawehner’s picture

@dawehner indeed, as long as we are okay with the potential of slightly breaking people's D8 sites. Otherwise I like that idea too.

Even as a site builder I would vote for fixing potential security problems over fixing existing views. Aggregator views are also not that common, if we are honest.

jhodgdon’s picture

By that argument, why do we have the "link to entity" option on any field formatters then? You could solve that using rewrites too.

dawehner’s picture

By that argument, why do we have the "link to entity" option on any field formatters then? You could solve that using rewrites too.

Because its a really common option compared to it.

jhodgdon’s picture

It seems like the 99% use case for making a view from an aggregator item is also that you'd want the title to link to the URL, by default.

dawehner’s picture

Fair point, https://www.drupal.org/planet for example links to the external URL.

#2387019: String field formatters cannot link to their parent entity was the original issue I was referring to.

xjm’s picture

Issue tags: +VDC
dawehner’s picture

So I guess we need a new formatter for that.

effulgentsia’s picture

Issue tags: +Critical Office Hours

Per #2393339-57: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality, tagging for critical office hours, but if there's a reason to not have this particular child issue in that list, please untag it.

xjm’s picture

See discussion in #2455149-8: Aggregator xss fields should be using Field/Entity formatters through 16; does any of that apply here? (Also why are these separate issues; what are the differences between these handlers and those?)

dawehner’s picture

This issue is about aggregator.title, the other issue is about aggregator.desription and aggregator.author

xjm’s picture

Priority: Critical » Major
Issue tags: -Critical Office Hours

Discussed with @dawehner; I think this is major for the same reasons as #2455149: Aggregator xss fields should be using Field/Entity formatters.

kgoel’s picture

Status: Needs work » Needs review
FileSize
9.4 KB

Status: Needs review » Needs work

The last submitted patch, 23: 2456701-23.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
587 bytes
9.98 KB

Status: Needs review » Needs work

The last submitted patch, 25: 2456701-25.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
9.98 KB
640 bytes
jhodgdon’s picture

Status: Needs review » Needs work

This is looking good, and thanks for working on it!

Aside from the test failures (come see me or dawehner if you need help figuring those out), I noticed a couple of other small things that should be fixed:

  1. +++ b/core/modules/aggregator/src/AggregatorFeedViewsData.php
    @@ -35,7 +35,7 @@ public function getViewsData() {
    -    $data['aggregator_feed']['title']['field']['id'] = 'aggregator_title_link';
    +    $data['aggregator_feed']['title']['field']['id'] = 'field';
         $data['aggregator_feed']['argument']['id'] = 'string';
    

    For this line, you can actually just delete the line from AggregatorFeedViewsData (same for the similar line in AggregatorItemViewsData).

    The reason is that the base EntityViewsData class provides a default of 'field' as the ID of any field handler plugin. The line with 'aggregator_title_link' that was there before was overriding the default. So now we can remove the line and use the default.

  2. +++ b/core/modules/aggregator/src/Plugin/Field/FieldFormatter/AggregatorTitleFormatter.php
    @@ -0,0 +1,85 @@
    +    return $field_definition->getTargetEntityTypeId() === 'aggregator_item' && $field_definition->getName() === 'title';
    

    I think this needs to be expanded to work on both 'aggregator_item' and 'aggregator_feed' entity types?

The last submitted patch, 27: 2456701-27.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
12.9 KB
5.69 KB

Status: Needs review » Needs work

The last submitted patch, 30: 2456701-30.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
14.72 KB
4.3 KB

Status: Needs review » Needs work

The last submitted patch, 32: 2456701-32.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
15.14 KB
1.13 KB
kgoel’s picture

Issue tags: +drupaldevdays
jhodgdon’s picture

Status: Needs review » Needs work

Looks great and passes tests, yeah! A few very minor things to fix:

  1. +++ b/core/modules/aggregator/src/Plugin/Field/FieldFormatter/AggregatorTitleFormatter.php
    @@ -0,0 +1,90 @@
    + *   label = @Translation("Aggregator item title"),
    + *   description = @Translation("Formats the aggregator item title with an optional link."),
    + *   field_types = {
    

    Minor thing, but this is used for both aggregator item and feed titles, not just items.

  2. +++ b/core/modules/aggregator/src/Plugin/Field/FieldFormatter/AggregatorTitleFormatter.php
    @@ -0,0 +1,90 @@
    +    if ($items->getEntity()->getEntityTypeId() == 'aggregator_feed') {
    +      $url = Url::fromUri($items->getEntity()->getUrl());
    +    }
    +    else {
    +      $url = Url::fromUri($items->getEntity()->getLink());
    +    }
    +
    

    I'm wondering here if all feed/item entities have URLs defined. If not, this could have a problem. Do we maybe need to check if the URL exists, before calling Url::fromUri()?

    So maybe the code would say:

    if (it's a feed) {
      $url_string = $... entity ... -> getUrl();
    else
       -> getLink();
    

    and then

    if ($url_string) {
      $url = Url::fromUri($url_string);
    }
    

    And then below you would only turn it into a link if the setting is set and $url is not empty?

  3. +++ b/core/modules/aggregator/src/Tests/AggregatorTitleTest.php
    @@ -0,0 +1,89 @@
    +    $aggregator_feed = Feed::create([
    

    Maybe add a // comment saying something like "Create a feed and an item on it." ?

    And then lower down say what we're testing, like "Verify feed title output with and without links", and then "Verify feed item title output with and without links"?

kgoel’s picture

Status: Needs work » Needs review
FileSize
15.38 KB
3.07 KB
kgoel’s picture

FileSize
15.38 KB

Found one minor white space issue so not bothering with interdiff for this patch.

The last submitted patch, 37: 2456701-37.patch, failed testing.

jhodgdon’s picture

Status: Needs review » Needs work

Almost!

  1. +++ b/core/modules/aggregator/src/Plugin/Field/FieldFormatter/AggregatorTitleFormatter.php
    @@ -0,0 +1,90 @@
    + *   label = @Translation("Aggregator item title"),
    + *   description = @Translation("Formats the aggregator item and aggregator feed title with an optional link."),
    + *   field_types = {
    

    Label still needs to be item or feed. I think "and" in the description should also be "or"?

    Hm. So if someone was actually using this, they would know if it was the aggregator item or feed... Hm....

    How about:

    label: Aggregator title
    Description: Formats an aggregator item or feed title with an optional link.

  2. +++ b/core/modules/aggregator/src/Tests/AggregatorTitleTest.php
    @@ -0,0 +1,93 @@
    +    // Creates an aggregator feed.
    

    Normally we use "Create" not "Creates" in in-line comments.

  3. +++ b/core/modules/aggregator/src/Tests/AggregatorTitleTest.php
    @@ -0,0 +1,93 @@
    +    //Verifies aggregator item title with and without links.
    

    Nitpick: needs space after //

kgoel’s picture

Status: Needs work » Needs review
FileSize
15.36 KB
2.51 KB
dawehner’s picture

+++ b/core/modules/aggregator/src/Plugin/Field/FieldFormatter/AggregatorTitleFormatter.php
@@ -0,0 +1,90 @@
+    return ($field_definition->getTargetEntityTypeId() === 'aggregator_item' || $field_definition->getTargetEntityTypeId() === 'aggregator_feed' && $field_definition->getName() === 'title');

I'd better put parenthesis about the ||

kgoel’s picture

FileSize
15.36 KB
985 bytes
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good now. Good catch @dawehner. Assuming bot still agrees, this should be RTBC now. Thanks @kgoel for all the work!

kgoel’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 43: 2456701-43.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
15.19 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, 47: 2456701-47.patch, failed testing.

dawehner’s picture

So that particular table mapping has to change form 'aggregator_item_title' to 'title' for example.

kgoel’s picture

Status: Needs work » Needs review
FileSize
38.69 KB
1.63 KB
kgoel’s picture

FileSize
16.34 KB
1.63 KB

correct patch this time.

dawehner’s picture

Twice a passing test, nice, but at least the later one is small enough.

Remaining tasks: Add 'title' and 'description' to \Drupal\aggregator\Tests\Views\AggregatorItemViewsFieldAccessTest by comment out the title/description test.

kgoel’s picture

FileSize
17.28 KB
962 bytes
dawehner’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Alright, this looks fine for me now. We now have test coverage, which is what we really want.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: 2456701-53.patch, failed testing.

isntall queued 53: 2456701-53.patch for re-testing.

kgoel queued 53: 2456701-53.patch for re-testing.

kgoel’s picture

Status: Needs work » Needs review
star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Well it's green again, back to #54's RTBC :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +D8 upgrade path

Using Field formatters is a major task as per #22. Major tasks are permitted in the beta. Committed 0489c18 and pushed to 8.0.x. Thanks!

  • alexpott committed 0489c18 on 8.0.x
    Issue #2456701 by kgoel, Cottser, dawehner, jhodgdon: Replace...

Status: Fixed » Closed (fixed)

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