Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Aggregator's rendering is still stuck to pre-entity conversion
Proposed resolution
Instead of using custom logic, templates etc, move everything to hook_entity_extra_field_info()
and field formatters
Remaining tasks
Review
Commit
User interface changes
- Field UI is now enabled for feed entities under
admin/config/services/aggregator
- Two view modes are available under the "Manage display" tab: "Default" and "Summary"
API changes
aggregator_feed_source
theming function is renamed toaggregator_feed
to match what usually happens with other entities in core..eg nodeaggregator_summary_items
andaggregator_summary
theming functions removed
Comment | File | Size | Author |
---|---|---|---|
#41 | interdiff-2261425.txt | 3.13 KB | ParisLiakos |
#41 | drupal-aggregator_render-2261425-40.patch | 38.96 KB | ParisLiakos |
#37 | interdiff-2261425.txt | 1.1 KB | ParisLiakos |
#37 | drupal-aggregator_render-2261425-37.patch | 38.07 KB | ParisLiakos |
#34 | interdiff-2261425.txt | 12.58 KB | ParisLiakos |
Comments
Comment #1
ParisLiakos CreditAttribution: ParisLiakos commentedI am not entirely sure about
hook_entity_extra_field_info()
tbh..but this is a quite old patch, i just splitted it out from #2154781: Convert aggregator/sources and aggregator/opml to viewsI am now studying #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title), it seems that there might be no need to use it
Comment #2
ParisLiakos CreditAttribution: ParisLiakos commentedOk, i got it:) great stuff happening in D8 :)
Anyway, i fixed the form ones, i would prefer to switch to formatters in a followup, preferebaly after the Node conversion, because many susefull stuff aggregator needs are added there..eg timestamp formatter
Comment #5
ParisLiakos CreditAttribution: ParisLiakos commentedWell i have no clue why
ConfigImportAllTest
fails and running it locally it just takes forever and ends withMaximum execution time of 240 seconds exceeded
Geez, why we keep creating tests like that..any help appreciated
Comment #7
BerdirYeah, we only need extra fields for stuff that we can't handle with formatters/widgets. Thanks for working on this, on my review list :)
Comment #8
BerdirI think that test fail means it's verifying that there no config changes left to sync at the end, looks like aggregator view display configuration has updated itself during the import, we should make sure that the provide the correct file as an import...
Will ask @alexpott to verify that...
Comment #9
alexpottLooking into it...
... to catch stuff like this.
Comment #10
alexpottSo the issue is with
entity.view_display.aggregator_feed.aggregator_feed.default
the order is being swapped around for some reason.And this is because checked and items have the same weight.
Comment #11
BerdirThis is added by default in EntityManager::getAllBundleInfo(). Wondering if we should make the feed the bundle of items, but we don't need to do this here.
As mentioned above, it shouldn't be necessary to provide those files, as this information is build automatically based on base and extra fields.
Can we use _entity_view in the route?
Not sure if we want to go there here, but what we could do is make this is a formatter instead and set it up in the base field definition, similar for others.
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commentedthanks for the review!
All valid points :)
I converted link and author fields to formatters..I added @todos for the rest fields..and for image, well its a special case, will need an issue on its own..(to be filed)
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commenteduhm String::checkPlain
I need to re-export those, now also the module dependency is added
Comment #14
ParisLiakos CreditAttribution: ParisLiakos commentedfixed #13
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedreroll for PSR4
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedreroll, also injecting the date service into the view builders
Comment #17
joelpittetDrive by review and tagging for http://drupaltwig.org/.
There is no hide() in twig. https://www.drupal.org/node/2212845
Needs a space after attributes per standards. @see https://drupal.org/node/1823416
Comment #18
joelpittetIs there any way we can get these tags in template?
The goal is to let these deal with the data and let the templates deal with marking that data up.
I can't think of a way so I thought I'd ask.
Comment #19
Berdir@#18: A lot of that markup will go away when we can switch to formatters for things like the updated and description fields, then those will get the default field template classes/markup. I wouldn't worry too much about that now..
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedthanks @joelpittet for the review! those hide() were old cp's :) updated them to reflect the new way of doing this.
The prefix/suffix tags..yeah, i only put them there to simulate the old markup and to actually have some wrappers..When those are converted to formatters, i will then be able to use the regular field wrappers and we can get rid of them
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedah, date service was renamed
Comment #25
joelpittetThank you @ParisLiakos for fixing those changes. @Berdir it's good to know the plan, is there an issue showing this change to formatters? Maybe it should be a child of this issue or related or something?
Comment #26
ParisLiakos CreditAttribution: ParisLiakos commentedThe node issue is the one is the one that will provide the timestamp formatters i need to build upon..and for the description field there is already an issue
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commentedgot rid many todos. i hope this is ready to go now. Only leftover is the description field which will be resolved in the issue linked above
Comment #28
ParisLiakos CreditAttribution: ParisLiakos commentedComment #30
Berdirinterdiff looks great :)
Comment #31
ParisLiakos CreditAttribution: ParisLiakos commentedoops, too many dots :P
Comment #32
Wim LeersGreat work here! :)
Mostly nitpicks, but also some questions.
Wrong indentation.
I don't see a
protected $dateFormatter
property on this class yet?Doesn't this need to be wrapped in
SafeMarkup::set()
?Mismatch.
I'm no fan of this. But it's definitely a step forward from hardcodedness in templates!
It'd be great if this could document *why* these have to be "extra" fields and can't be "proper" fields.
Shouldn't this be configurable in the Feed's view mode?
And it looks like it is… so … is this if-test a leftover that can be removed? Or what's going on here?
There's no space before the span and a space after, should be vice versa :)
A dependency on itself (here and elsewhere)? Or am I misreading that?
Wrong indentation.
Comma after "mode".
Can't this use
#route_name
and#route_parameters
instead?Comment #33
Berdir5. Some could be formatters and some of those already have @todo's, but extra fields in general is still a valid concept, one example is items. That's a list of something and doesn't exist as a field, so it is "extra".
6. Maybe should be moved from a view mode check to a $display->getComponent() check, which respects the current view mode setting?
7. he visually hidden thing actually makes it tricky, because you don't want a space if it is hidden, so it needs to be inside? Weird but correct I think.
8. You did. Dependency on the view mode, this is the display.
Comment #34
ParisLiakos CreditAttribution: ParisLiakos commentedThanks for the review!
1.Fixed
2.Thanks!!i obviously forgot that
3. No, because it comes from t()
4. Fixed
5. While i was there..i opened an issue to move feed image to formatter..its possible, but i dont want to do it here because its not that simple and straightforward like the other formatters. Also! i realized i could move the "feed" for items into a formatter really easily :))
6. I did what Berdir suggested, which required me to also provide install config for the "default" view mode, so i can get rid of More link.
7. Yes, Berdir is right, its like that in the node module too.
9, 10 Fixed
11. Very good point!! fixed
Comment #36
Wim LeersLooking great! :)
Comment #37
ParisLiakos CreditAttribution: ParisLiakos commentedlets remove some junk. hopefully back to green
Comment #38
BerdirThat's not the right fix. Instead, you need to define the config schema for those types to core.entity.schema.yml, see entity_view_display.field.number_unformatted as an example.
Comment #39
ParisLiakos CreditAttribution: ParisLiakos commentedaah, i see now:) thanks for the tip!
will try to add the necessary fields there
Comment #41
ParisLiakos CreditAttribution: ParisLiakos commentedthat simple?
Comment #42
Wim LeersI think this is RTBC. I'll leave it to Berdir to take one last look and RTBC it :)
Comment #43
BerdirYes, looks great I think.
Comment #44
alexpottWe either need a new change record here - or we need to attach this to [#1905910] and do a massive update for themers.
Comment #45
ParisLiakos CreditAttribution: ParisLiakos commentedThanks Wim and Berdir!
@alexpott: good idea! i updated the existing..i think this will be enough
back to RTBC
Comment #46
alexpottCommitted c554974 and pushed to 8.0.x. Thanks!
Yep I contributed to this patch but only in a very minor way to fix some default configuration.
Removed unused uses.
Comment #48
ParisLiakos CreditAttribution: ParisLiakos commentedyay!
thanks!
Comment #50
znerol CreditAttribution: znerol commentedThis also fixed #397962: Aggregator: link breaks if feed not refreshed yet.