Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Issue #1896060 by shanethehat, Cottser, joelpittet, disasm, ParisLiakos, Floydm, stevector, jenlampton, c4rl, mr.baileys, thedavidmeister, jwilson3: aggregator.module - Convert PHPTemplate templates to Twig.
(as of #74)
Task
Convert aggregator module to use Twig instead of PHPTemplate
Remaining
- Documentation @todos need to be filled out.
Theme function name/template path | Conversion status |
---|---|
core/modules/aggregator/templates/aggregator-feed-source.tpl.php | converted |
core/modules/aggregator/templates/aggregator-item.tpl.php | converted |
core/modules/aggregator/templates/aggregator-summary-items.tpl.php | converted |
core/modules/aggregator/templates/aggregator-wrapper.tpl.php | removed, theme hook was removed in #293318: Convert Aggregator feeds into entities |
User interface changes
n/a
Related Issues
#1987390: aggregator.module - Convert theme_ functions to Twig
#1757550: [Meta] Convert core theme functions to Twig templates
#1938970: Consolidate aggregator-summary-items.html.twig and aggregator-summary-item.html.twig
#1938894: Convert aggregator theme tables to table #type
Follow-up Issues
#1966100: Convert format_rss_channel() and format_rss_item() to twig templates
Comment | File | Size | Author |
---|---|---|---|
#61 | interdiff.txt | 1.15 KB | mr.baileys |
#61 | twig-aggregator-templates-1896060-61.patch | 12.25 KB | mr.baileys |
#56 | interdiff.txt | 891 bytes | shanethehat |
#56 | twig-aggregator-templates-1896060-56.patch | 12.25 KB | shanethehat |
#54 | interdiff.txt | 1.42 KB | shanethehat |
Comments
Comment #0.0
jenlamptonissue summary initiative
Comment #0.1
jenlamptoncleanup
Comment #1
geoffreyr CreditAttribution: geoffreyr commentedStarted preparing some updates in #1912552. As mentioned there, might need to work out a nicer way of spitting out OPML than running it through a theme function.
Comment #2
disasm CreditAttribution: disasm commentedAttached is a patch.
Comment #4
floydm CreditAttribution: floydm commentedAn updated version of the patch from #2 that passes tests and generally appears to work is attached.
I may have reintroduced some cruft in the theme registration (the view_mode args), but it is working as it is.
Comment #5
stevectorThis revised patch gets the failing tests to pass. I haven't done a complete review yet. I just noticed that the original patch removed a lot of the variable defaults from the hook_theme implementation. Doing that meant that those variables don't get set and thus the fails.
Comment #5.0
jenlamptonLink to sandbox
Comment #6
jenlamptonnotes:
- Preprocess functions need to meet new coding standards
aggregator-item.html.twig
- filters in Twig should not be surrounded by spaces
'Categories' | t
should be'Categories'|t
- (I also removed some whitespace/linebreaks in there I thought was too much)
aggregator-summary-items.html.twig
- theme_item_list in doc block needs to be updated to reference item-list.html.twig
- filters in Twig should not be surrounded by spaces
aggregator-summary-item.html.twig
- missing?
aggregator-wrapper.html.twig
- theme_pager in doc block needs to be updated to reference pager.html.twig
- @see reference to theme function needs to be removed
aggregator-feed-source.html.twig
- theme_feed_icon in doc block needs to be updated to reference feed-icon.html.twig
- filters in Twig should not be surrounded by spaces
aggregator-page-opml.html.twig
- missing?
aggregator_categorize_items.html.twig
- missing?
aggregator_page_rss.html.twig
- missing?
I'll reroll this.
Also, follow-up: #1938970: Consolidate aggregator-summary-items.html.twig and aggregator-summary-item.html.twig
Comment #7
star-szrRe-adding tag.
Comment #8
jenlamptonI need to work on other things today, but I took a first stab at the updates above. aggregator-page-rss.html.twig still needs to be converted.
Comment #9
star-szr@jenlampton - Last patch was empty, please repost if you can!
Edit: interdiff is not empty, I'll see what I can do.
Comment #10
star-szrOkay, rerolled #5 along with the changes in #8.
The only conflict was #1136686: Aggregator Template Needs Context for Accessibility.
Comment #12
star-szrI will post a revised patch in the next day or two to move this along further, looks like there's more than just aggregator-page-rss.html.twig missing here.
Comment #13
star-szrAfter rerolling, I ended up with two template_preprocess_aggregator_summary_item() functions, that's where the PHP syntax error is. I think git was trying to be clever and automerge things, but I could be wrong.
So I will sort that out as well.
Thanks for all your work on this everyone!
Comment #14
joelpittet#1938894: Convert aggregator theme tables to table #type don't need to convert
theme_aggregator_categorize_items
I know you haven't but just in case someone get's the inkling.
Comment #14.0
joelpittetfollow up
Comment #15
star-szrShould have a new patch up within 48 hours.
Comment #16
star-szrOkay, I think I sorted out the two template_preprocess_aggregator_summary_item() functions.
#293318: Convert Aggregator feeds into entities removed aggregator-wrapper (but not the .tpl.php file) and also changed 'item' to 'aggregator_item' for a couple items in aggregator_theme().
Rerolled for #1938894: Convert aggregator theme tables to table #type as well.
Still needs docs updates and a couple conversions are missing:
theme_aggregator_block_item()
theme_aggregator_page_rss()
(Looks like I'm 3 minutes late :))
Comment #17
star-szrComment #18
joelpittettheme_aggregator_page_rss seemed to need a bit of API work. Ended up removing
format_rss_channel
andformat_rss_item
because they generated most of the markup that the RSS needed and I wanted no more tags in PHP strings. I left in 'format_xml_elements
' because it's more generic.Also translated node_feed() to use aggregator_page_rss because that's what it does and it was uing those functions and duplicated the logic.
Comment #18.0
joelpittetAdded #type=>table related entry
Comment #18.1
star-szrUpdate summary to add conversion status
Comment #18.2
star-szrTweaks
Comment #20
star-szrUnassigning, want to trade me for #1898416: filter.module - Convert theme_ functions to Twig @joelpittet? :)
Comment #21
joelpittet@Cottser, trade accepted:)
Comment #22
joelpittetThis will fix at least one, the other error I am a bit puzzled by.
taxonomy_term_feed() doesn't seem to get called for some reason...
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedsilently watching here, but i really wonder how we are calling an aggregator's theme function from node module?aggregator is a non standard module, many sites dont have it enabled..will node_feed still work if aggregator is disabled?
Comment #25
star-szrTagging.
Comment #25.0
star-szrRemove header
Comment #26
joelpittet@rootatwc totally right, that may have been a tangent... I made a kind of follow-up issue to possibly convert those format_ functions to twig.
This patch takes a middle ground. Removes
format_rss_channel()
from use but leavesformat_rss_item()
. Rolled back the node_feed and format_ function changes and deletions.Fixed up some whitespace stuff, missing last_checked variable.
RSS page summary Read more link needs a manual test.
Comment #26.0
joelpittetremove the conversion of the format_ functions
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commentedthats better, thanks!
introducing whitespace here
i believe we dont need check_plain here.t takes care of it for @ placeholders
Comment #28
star-szrThanks @joelpittet, took a quick look and found this tweak:
.html.twig, and the first one looks like it should be aggregator-block-item.
Comment #29
joelpittetAnybody can re-roll this with #27 and #28
Comment #30
shanethehat CreditAttribution: shanethehat commentedComment #31
star-szrChanges look good, thanks @shanethehat :)
This should end in 'templates' per #1913208: [policy] Standardize template preprocess function documentation, consider an extra line of description below the summary and before 'Default template:' to explain what this is for.
Comment #32
shanethehat CreditAttribution: shanethehat commentedComment #33
shanethehat CreditAttribution: shanethehat commentedComment #34
star-szrThanks @shanethehat! Better, but I think the summary line should still mention 'block', since there's also aggregator-item.html.twig to get this confused with.
Comment #35
shanethehat CreditAttribution: shanethehat commentedIt feels to me like once the block is mentioned in the summary, the second line is redundant.
Comment #36
steveoliver CreditAttribution: steveoliver commentedI agree with #35 -- interdiff looks good.
Comment #36.0
steveoliver CreditAttribution: steveoliver commentedadded followup
Comment #37
star-szrI'm going to split this patch up between here and #1987390: aggregator.module - Convert theme_ functions to Twig.
(See #1757550-40: [Meta] Convert core theme functions to Twig templates)
Comment #37.0
star-szrRemove sandbox link
Comment #38
star-szrAnd here are just the theme function conversions.
Comment #38.0
star-szrRemove .tpl.php conversions from summary
Comment #38.1
star-szrUpdated issue summary.
Comment #39
star-szrComment #40
c4rl CreditAttribution: c4rl commentedWe've reversed the purpose here. Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to templates rather than theme_ functions. See #1987390: aggregator.module - Convert theme_ functions to Twig for theme_ function conversion.
Comment #40.0
c4rl CreditAttribution: c4rl commentedUpdated issue summary.
Comment #41
c4rl CreditAttribution: c4rl commentedComment #42
star-szrHere are just the .tpl.php conversions.
Comment #42.0
star-szrUpdated issue summary.
Comment #43
shanethehat CreditAttribution: shanethehat commentedAll these templates have classes hard-coded rather than being supplied by $variables['attributes']. I think stuff like
<div class="feed-item">
should be
<div{{ attributes }}>
Comment #44
star-szrGood eye @shanethehat! This looks like another exception to #1938430: Don't add a default theme hook class in template_preprocess() which makes the case for removing it stronger :)
Comment #45
c4rl CreditAttribution: c4rl commented#43 @shanethehat Hm, I'm not sure that is such a bad thing. Given our urgency to get this to RTBC, do we really care at this point?
I don't want this to turn into a bikeshed; but if we need to discuss this, we should consult/update http://drupal.org/node/1823416
Comment #46
c4rl CreditAttribution: c4rl commentedAfter chatting with @joelpittet in IRC, the purpose of moving from hard-coded classes to simply {{ attributes }} is that it gives us a better picture of templates that would be likely candidates for eventual consolidation.
I definitely agree with this in principle but don't know that it is necessarily a blocker for our initial set of .tpl.php conversions for Twig -- I would certainly want to avoid us overly nitpicking our tpls to block #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch.
Comment #47
star-szrI think it's fine as is especially for a straight conversion.
Comment #48
c4rl CreditAttribution: c4rl commentedWell, I made those revisions (and some doc cleanup) before @Cottser posted #47. Here it is nevertheless.
Comment #49
star-szr@c4rl - that's fine :) but we shouldn't instantiate the Attribute in preprocess like that now that #1982024: Lazy-load Attribute objects later in the rendering process only if needed is in. Thanks!
Comment #50
shanethehat CreditAttribution: shanethehat commentedComment #51
thedavidmeister CreditAttribution: thedavidmeister commented+ * - source: Information about the feed source.
As evidenced by the Twig template docs, we know a lot more about the structure of "source" than just "Information about". We could say that it's an "Associative array containing information about" for a start and we could even outline the contents of what's expected to be in that array if we were feeling ambitious :)
Other than that, I think this looks pretty good. Definitely ready for someone to be manually testing it.
Comment #52
shanethehat CreditAttribution: shanethehat commentedFirst, a reroll.
Comment #53
shanethehat CreditAttribution: shanethehat commentedDocumentation change from #51: source (and aggregator source in template_preprocess_aggregator_feed_source()) are feed entities, so I've added a @see annotation instead of describing the entity contents.
Comment #54
shanethehat CreditAttribution: shanethehat commentedRevised documentation after IRC feedback
Comment #55
mr.baileysLeft-over debug statement.
Comment #56
shanethehat CreditAttribution: shanethehat commentedOops! Thanks @mr.baileys
Comment #57
shanethehat CreditAttribution: shanethehat commentedComment #58
jwilson3Do we need a twig file to replace
aggregator-wrapper.tpl.php
?EDIT: to answer myself, no we dont need it, it appears this theme function was removed in a separate issue, but the template is still there.
Comment #59
jwilson3I couldn't find anything else wrong with this from a visual review. Still needs manual testing.
Comment #60
mr.baileysAssigning to myself for manual testing a patch review.
Comment #61
mr.baileys#293318: Convert Aggregator feeds into entities removed the aggregator_wrapper theme entry but neglected to remove the template, so agreed that aggregator-wrapper.tpl.php can just be removed.
value should not be emphasized.
Should actually be "feed item", since it renders an individual story from the feed.
Updated patch.
Comment #62
mr.baileysComment #63
joelpittetThanks @mr.baileys! Changes look good thanks for fixing that misplaced
Here's profiles @mr.baileys and I did together, hope it's up to par:
Scenario 1
Path: /aggregator/sources/1
BBC Feeds 82 articles, 20 per page shown.
Scenario 2
Path: /aggregator/categories
1 category showing 4 articles tagged, showing 3 most recent.
Comment #64
thedavidmeister CreditAttribution: thedavidmeister commentedwow.. Twig is a little faster here. that's nice :)
Comment #65
mr.baileysRe-did the profiling since our run yesterday did not include the "twig-node" on the profiled pages, so Twig engine loading was included in the numbers.
Scenario 1
Path: /aggregator/sources/1
BBC Feeds 82 articles, 20 per page shown.
Scenario 2
Path: /aggregator/categories
1 category showing 4 articles tagged, showing 3 most recent.
Comment #66
alexpottCan we explain the 1% slow down in scenario 1?
Comment #67
joelpittetGoing to run again and try to explain.
Comment #68
joelpittetOk ran through this again, made sure it's stark, with a "view block full content node" on the page and more of my apps closed. These results were fairly consistent over 5 runs each scenario.
Scenario 1
Path: /aggregator/sources/1
BBC Feeds 83 articles, 20 per page shown.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519af44245f83&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519af44245f83&...
Scenario 2
Path: /aggregator/categories
1 category showing all articles tagged, showing 3 most recent.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519afa8ec2e5c&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519afa8ec2e5c&...
Comment #69
jenlamptonless than 1% slower in one case, and faster in the next! Faster faster let's make drupal faster :)
Comment #70
alexpottWe still have an instance of aggregator wrapper in core...
Comment #71
thedavidmeister CreditAttribution: thedavidmeister commentedkk
Comment #72
mr.baileysWhat was removed in #293318: Convert Aggregator feeds into entities in was the use of the
aggregrator-wrapper
theme entry in aggregator'stheme_hook
. This patch cleans up by actually removing theaggregator-wrapper.tpl.php
file, which is no longer being used.There are three references to
aggregator-wrapper
left in core:But these are classes. They seem to make sense to me, as they are set on containers wrapping aggregator output. Not sure we should remove these, and if we do, not sure we should do so as part of this patch (since we are doing a straight-up conversion of the template files, and removing this class would generate different markup.)
Comment #73
mr.baileysComment #74
alexpottYep ignore #70... as a class I guess it's fine... and it's out of scope to change it here anyway :)
+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #74.0
alexpottUpdated issue summary.
Comment #74.1
ianthomas_ukAdd commit message
Comment #75
alexpottCommitted 4cc09aa and pushed to 8.x. Thanks!
Comment #76.0
(not verified) CreditAttribution: commentedUpdated issue summary (Remove READY from title).