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.
Comment | File | Size | Author |
---|---|---|---|
#53 | interdiff.txt | 962 bytes | kgoel |
#53 | 2456701-53.patch | 17.28 KB | kgoel |
#51 | interdiff.txt | 1.63 KB | kgoel |
#51 | 2456701-50.patch | 16.34 KB | kgoel |
#50 | interdiff.txt | 1.63 KB | kgoel |
Comments
Comment #1
star-szrI'll try this one.
Comment #2
star-szrHere's a start, I hope :)
Comment #4
jhodgdonSee #2456713-7: Custom taxonomy field views handler needs to be replaced with generic Field API handler for some suggestions that may help get this working.
Comment #5
star-szrNot 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:
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.
Comment #6
star-szrOh it helps if you attach files.
Edit: And the last one was a crosspost, thanks @jhodgdon!
Comment #7
jhodgdonIt 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.
Comment #8
star-szrGotcha. That makes sense I think, thanks! :)
Comment #10
dawehnerWell, 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.
Comment #11
star-szr@dawehner indeed, as long as we are okay with the potential of slightly breaking people's D8 sites. Otherwise I like that idea too.
Comment #12
dawehnerEven 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.
Comment #13
jhodgdonBy that argument, why do we have the "link to entity" option on any field formatters then? You could solve that using rewrites too.
Comment #14
dawehnerBecause its a really common option compared to it.
Comment #15
jhodgdonIt 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.
Comment #16
dawehnerFair 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.
Comment #17
xjmComment #18
dawehnerSo I guess we need a new formatter for that.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedPer #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.
Comment #20
xjmSee 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?)
Comment #21
dawehnerThis issue is about aggregator.title, the other issue is about aggregator.desription and aggregator.author
Comment #22
xjmDiscussed with @dawehner; I think this is major for the same reasons as #2455149: Aggregator xss fields should be using Field/Entity formatters.
Comment #23
kgoel CreditAttribution: kgoel commentedComment #25
kgoel CreditAttribution: kgoel commentedComment #27
kgoel CreditAttribution: kgoel commentedComment #28
jhodgdonThis 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:
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.
I think this needs to be expanded to work on both 'aggregator_item' and 'aggregator_feed' entity types?
Comment #30
kgoel CreditAttribution: kgoel commentedComment #32
kgoel CreditAttribution: kgoel commentedComment #34
kgoel CreditAttribution: kgoel commentedComment #35
kgoel CreditAttribution: kgoel commentedComment #36
jhodgdonLooks great and passes tests, yeah! A few very minor things to fix:
Minor thing, but this is used for both aggregator item and feed titles, not just items.
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:
and then
And then below you would only turn it into a link if the setting is set and $url is not empty?
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"?
Comment #37
kgoel CreditAttribution: kgoel commentedComment #38
kgoel CreditAttribution: kgoel commentedFound one minor white space issue so not bothering with interdiff for this patch.
Comment #40
jhodgdonAlmost!
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.
Normally we use "Create" not "Creates" in in-line comments.
Nitpick: needs space after //
Comment #41
kgoel CreditAttribution: kgoel commentedComment #42
dawehnerI'd better put parenthesis about the ||
Comment #43
kgoel CreditAttribution: kgoel commentedComment #44
jhodgdonLooks good now. Good catch @dawehner. Assuming bot still agrees, this should be RTBC now. Thanks @kgoel for all the work!
Comment #45
kgoel CreditAttribution: kgoel at Forum One commentedComment #47
kgoel CreditAttribution: kgoel at Forum One commentedReroll
Comment #49
dawehnerSo that particular table mapping has to change form 'aggregator_item_title' to 'title' for example.
Comment #50
kgoel CreditAttribution: kgoel at Forum One commentedComment #51
kgoel CreditAttribution: kgoel at Forum One commentedcorrect patch this time.
Comment #52
dawehnerTwice 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.Comment #53
kgoel CreditAttribution: kgoel at Forum One commentedComment #54
dawehnerAlright, this looks fine for me now. We now have test coverage, which is what we really want.
Comment #58
kgoel CreditAttribution: kgoel at Forum One commentedComment #59
star-szrWell it's green again, back to #54's RTBC :)
Comment #60
alexpottUsing 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!