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

#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

#1966100: Convert format_rss_channel() and format_rss_item() to twig templates

CommentFileSizeAuthor
#61 interdiff.txt1.15 KBmr.baileys
#61 twig-aggregator-templates-1896060-61.patch12.25 KBmr.baileys
#56 interdiff.txt891 bytesshanethehat
#56 twig-aggregator-templates-1896060-56.patch12.25 KBshanethehat
#54 interdiff.txt1.42 KBshanethehat
#54 twig-aggregator-templates-1896060-54.patch12.4 KBshanethehat
#53 interdiff.txt1.55 KBshanethehat
#53 twig-aggregator-templates-1896060-53.patch12.41 KBshanethehat
#52 twig-aggregator-templates-1896060-52.patch12.29 KBshanethehat
#50 interdiff.txt1.06 KBshanethehat
#50 twig-aggregator-templates-1896060-50.patch12.13 KBshanethehat
#48 drupal-1896060-48.patch12.19 KBc4rl
#48 drupal-1896060-48.patch-interdiff.txt2.95 KBc4rl
#42 1896060-42.patch10.44 KBstar-szr
#38 1896060-38.patch12.74 KBstar-szr
#35 twig-aggregator-1896060-35.patch22.7 KBshanethehat
#35 interdiff.txt858 bytesshanethehat
#33 interdiff.txt715 bytesshanethehat
#33 twig-aggregator-1896060-33.patch22.72 KBshanethehat
#30 interdiff.txt3.22 KBshanethehat
#30 twig-aggregator-1896060-30.patch22.68 KBshanethehat
#26 interdiff.txt11.01 KBjoelpittet
#26 twig-aggregator-1896060-26.patch22.62 KBjoelpittet
#22 twig-aggregator-1896060-22.patch26.43 KBjoelpittet
#22 interdiff.txt1.68 KBjoelpittet
#18 interdiff.txt13.32 KBjoelpittet
#18 twig-aggregator-1896060-18.patch26.79 KBjoelpittet
#16 1896060-16.patch15.26 KBstar-szr
#16 interdiff.txt1.98 KBstar-szr
#10 1896060-10.patch16.63 KBstar-szr
#8 core-convert_aggregator_to_twig-1896060-7.patch0 bytesjenlampton
#8 interdiff.txt9.32 KBjenlampton
#5 drupal-aggregator_twig-1896060-4.patch10.66 KBstevector
#4 drupal-aggregator_twig-1896060-4.patch12.63 KBfloydm
#2 drupal-aggregator_twig-1896060-2.patch10.42 KBdisasm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jenlampton’s picture

Issue summary: View changes

issue summary initiative

jenlampton’s picture

Issue summary: View changes

cleanup

geoffreyr’s picture

Started 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.

disasm’s picture

Assigned: jenlampton » disasm
Status: Active » Needs review
FileSize
10.42 KB

Attached is a patch.

Status: Needs review » Needs work

The last submitted patch, drupal-aggregator_twig-1896060-2.patch, failed testing.

floydm’s picture

Status: Needs work » Needs review
FileSize
12.63 KB

An 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.

stevector’s picture

Assigned: disasm » Unassigned
Issue tags: +SprintWeekend2013
FileSize
10.66 KB

This 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.

jenlampton’s picture

Issue summary: View changes

Link to sandbox

jenlampton’s picture

Assigned: Unassigned » jenlampton
Status: Needs review » Needs work
Issue tags: -SprintWeekend2013

notes:
- 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

star-szr’s picture

Issue tags: +SprintWeekend2013

Re-adding tag.

jenlampton’s picture

I 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.

star-szr’s picture

@jenlampton - Last patch was empty, please repost if you can!

Edit: interdiff is not empty, I'll see what I can do.

star-szr’s picture

Status: Needs work » Needs review
FileSize
16.63 KB

Okay, rerolled #5 along with the changes in #8.

The only conflict was #1136686: Aggregator Template Needs Context for Accessibility.

Status: Needs review » Needs work

The last submitted patch, 1896060-10.patch, failed testing.

star-szr’s picture

Assigned: jenlampton » star-szr

I 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.

star-szr’s picture

After 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!

joelpittet’s picture

#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.

joelpittet’s picture

Issue summary: View changes

follow up

star-szr’s picture

Should have a new patch up within 48 hours.

star-szr’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
15.26 KB

Okay, 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 :))

star-szr’s picture

Status: Needs review » Needs work
joelpittet’s picture

Status: Needs work » Needs review
FileSize
26.79 KB
13.32 KB

theme_aggregator_page_rss seemed to need a bit of API work. Ended up removing
format_rss_channel and format_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.

joelpittet’s picture

Issue summary: View changes

Added #type=>table related entry

star-szr’s picture

Issue summary: View changes

Update summary to add conversion status

star-szr’s picture

Issue summary: View changes

Tweaks

Status: Needs review » Needs work

The last submitted patch, twig-aggregator-1896060-18.patch, failed testing.

star-szr’s picture

Assigned: star-szr » Unassigned

Unassigning, want to trade me for #1898416: filter.module - Convert theme_ functions to Twig @joelpittet? :)

joelpittet’s picture

Assigned: Unassigned » joelpittet

@Cottser, trade accepted:)

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.68 KB
26.43 KB

This 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...

Status: Needs review » Needs work

The last submitted patch, twig-aggregator-1896060-22.patch, failed testing.

ParisLiakos’s picture

silently 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?

star-szr’s picture

Issue tags: +Needs manual testing

Tagging.

star-szr’s picture

Issue summary: View changes

Remove header

joelpittet’s picture

Status: Needs work » Needs review
FileSize
22.62 KB
11.01 KB

@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 leaves format_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.

joelpittet’s picture

Issue summary: View changes

remove the conversion of the format_ functions

ParisLiakos’s picture

thats better, thanks!

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -425,55 +415,77 @@ function aggregator_page_rss() {
-
+  ¶

introducing whitespace here

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -425,55 +415,77 @@ function aggregator_page_rss() {
+  $variables['title'] = check_plain(t('@site_name aggregator', array('@site_name' => $site_name)));

i believe we dont need check_plain here.t takes care of it for @ placeholders

star-szr’s picture

Thanks @joelpittet, took a quick look and found this tweak:

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -519,18 +523,19 @@ function aggregator_category_load($cid) {
+ * Default template: aggregator-item.tpl.php

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -282,24 +282,14 @@ function aggregator_categorize_items_submit($form, &$form_state) {
+ * Default template: aggregator-item.tpl.php

.html.twig, and the first one looks like it should be aggregator-block-item.

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Needs review » Needs work
Issue tags: +Novice

Anybody can re-roll this with #27 and #28

shanethehat’s picture

Status: Needs work » Needs review
FileSize
22.68 KB
3.22 KB
star-szr’s picture

Changes look good, thanks @shanethehat :)

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -523,9 +523,9 @@ function aggregator_category_load($cid) {
+ * Prepares variables for an individual feed item for display in the block.

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.

shanethehat’s picture

Assigned: Unassigned » shanethehat
Status: Needs review » Needs work
shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
22.72 KB
715 bytes
star-szr’s picture

Status: Needs review » Needs work
+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -527,7 +527,9 @@ function aggregator_category_load($cid) {
- * Prepares variables for an individual feed item for display in the block.
+ * Prepares variables for individual feed item templates.
+ *
+ * Individual feed items are displayed in a block.
  *
  * Default template: aggregator-block-item.html.twig.

Thanks @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.

shanethehat’s picture

Status: Needs work » Needs review
FileSize
858 bytes
22.7 KB

It feels to me like once the block is mentioned in the summary, the second line is redundant.

steveoliver’s picture

I agree with #35 -- interdiff looks good.

steveoliver’s picture

Issue summary: View changes

added followup

star-szr’s picture

Assigned: Unassigned » star-szr
Status: Needs review » Needs work
Issue tags: -Novice
star-szr’s picture

Issue summary: View changes

Remove sandbox link

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs work » Needs review
FileSize
12.74 KB

And here are just the theme function conversions.

star-szr’s picture

Issue summary: View changes

Remove .tpl.php conversions from summary

star-szr’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

Title: Convert aggregator module to Twig » aggregator.module - Convert theme functions to Twig
c4rl’s picture

Title: aggregator.module - Convert theme functions to Twig » aggregator.module - Convert PHPTemplate templates to Twig

We'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.

c4rl’s picture

Issue summary: View changes

Updated issue summary.

c4rl’s picture

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

Status: Needs work » Needs review
Issue tags: +needs profiling
FileSize
10.44 KB

Here are just the .tpl.php conversions.

star-szr’s picture

Issue summary: View changes

Updated issue summary.

shanethehat’s picture

Status: Needs review » Needs work

All 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 }}>

star-szr’s picture

Good 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 :)

c4rl’s picture

#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

c4rl’s picture

After 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.

star-szr’s picture

Status: Needs work » Needs review

I think it's fine as is especially for a straight conversion.

c4rl’s picture

Well, I made those revisions (and some doc cleanup) before @Cottser posted #47. Here it is nevertheless.

star-szr’s picture

Status: Needs review » Needs work

@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!

shanethehat’s picture

Status: Needs work » Needs review
FileSize
12.13 KB
1.06 KB
thedavidmeister’s picture

Status: Needs review » Needs work

+ * - 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.

shanethehat’s picture

First, a reroll.

shanethehat’s picture

Status: Needs work » Needs review
FileSize
12.41 KB
1.55 KB

Documentation 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.

shanethehat’s picture

Revised documentation after IRC feedback

mr.baileys’s picture

Status: Needs review » Needs work
+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -531,11 +538,18 @@ function theme_aggregator_page_opml($variables) {
+  die('<pre>'.print_r($variables['source'],true));

Left-over debug statement.

shanethehat’s picture

Oops! Thanks @mr.baileys

shanethehat’s picture

Status: Needs work » Needs review
jwilson3’s picture

Status: Needs review » Needs work
diff --git a/core/modules/aggregator/templates/aggregator-wrapper.tpl.php b/core/modules/aggregator/templates/aggregator-wrapper.tpl.php
deleted file mode 100644
index 0e97ba4..0000000

index 0e97ba4..0000000
--- a/core/modules/aggregator/templates/aggregator-wrapper.tpl.php
+++ /dev/nullundefined

Do 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.

jwilson3’s picture

Status: Needs work » Needs review

I couldn't find anything else wrong with this from a visual review. Still needs manual testing.

mr.baileys’s picture

Assigned: Unassigned » mr.baileys

Assigning to myself for manual testing a patch review.

mr.baileys’s picture

#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.

+++ b/core/modules/aggregator/templates/aggregator-feed-source.html.twigundefined
@@ -0,0 +1,35 @@
+  <div class="feed-updated">
+    <em>{{ 'Updated'|t }}: {{ last_checked }}</em>
+  </div>
+</div>
diff --git a/core/modules/aggregator/templates/aggregator-feed-source.tpl.php b/core/modules/aggregator/templates/aggregator-feed-source.tpl.php

The <code>{{last_checked}}

value should not be emphasized.

+++ b/core/modules/aggregator/templates/aggregator-item.html.twigundefined
@@ -0,0 +1,44 @@
+ * Default theme implementation to present a feed in an aggregator page.

Should actually be "feed item", since it renders an individual story from the feed.

Updated patch.

mr.baileys’s picture

Assigned: mr.baileys » Unassigned
Issue tags: -Needs manual testing
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -needs profiling

Thanks @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.

[ 8.x ] $ bbranches 51981a3a6232a 1896060-twig-aggregator
=== 8.x..8.x compared (51981a3a6232a..51981a807e11e):
 
ct : 38,281|38,281|0|0.0%
wt : 278,813|278,935|122|0.0%
cpu : 249,691|250,819|1,128|0.5%
mu : 11,528,872|11,528,872|0|0.0%
pmu : 11,703,344|11,703,344|0|0.0%
 
=== 8.x..1896060-twig-aggregator compared (51981a3a6232a..51981ab8ac6b7):
 
ct : 38,281|39,486|1,205|3.1%
wt : 278,813|285,778|6,965|2.5%
cpu : 249,691|256,387|6,696|2.7%
mu : 11,528,872|11,985,304|456,432|4.0%
pmu : 11,703,344|12,154,536|451,192|3.9%
 
---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

Scenario 2

Path: /aggregator/categories
1 category showing 4 articles tagged, showing 3 most recent.

[ 8.x ] $ bbranches 51981c003b825 1896060-twig-aggregator
=== 8.x..8.x compared (51981c003b825..51981d3824a90):
 
ct : 27,646|27,646|0|0.0%
wt : 220,204|220,698|494|0.2%
cpu : 191,689|192,597|908|0.5%
mu : 11,305,776|11,305,776|0|0.0%
pmu : 11,458,160|11,458,160|0|0.0%
 
=== 8.x..1896060-twig-aggregator compared (51981c003b825..51981d665ccb7):
 
ct : 27,646|27,723|77|0.3%
wt : 220,204|218,132|-2,072|-0.9%
cpu : 191,689|191,837|148|0.1%
mu : 11,305,776|11,365,376|59,600|0.5%
pmu : 11,458,160|11,517,216|59,056|0.5%
 
 
---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

thedavidmeister’s picture

wow.. Twig is a little faster here. that's nice :)

mr.baileys’s picture

Re-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.

=== 8.x..8.x compared (519976ba40ee4..5199772e4c075):

ct  : 55,875|55,875|0|0.0%
wt  : 312,034|312,990|956|0.3%
cpu : 304,019|308,020|4,001|1.3%
mu  : 15,058,656|15,058,656|0|0.0%
pmu : 15,215,328|15,215,328|0|0.0%
=== 8.x..1896060 compared (519976ba40ee4..51997782923af):

ct  : 55,875|56,902|1,027|1.8%
wt  : 312,034|314,763|2,729|0.9%
cpu : 304,019|308,019|4,000|1.3%
mu  : 15,058,656|15,130,696|72,040|0.5%
pmu : 15,215,328|15,278,648|63,320|0.4%

Scenario 2

Path: /aggregator/categories
1 category showing 4 articles tagged, showing 3 most recent.

=== 8.x..8.x compared (5199785bef0ea..51997891724d5):

ct  : 37,287|37,287|0|0.0%
wt  : 233,934|233,662|-272|-0.1%
cpu : 228,014|228,014|0|0.0%
mu  : 14,406,880|14,406,880|0|0.0%
pmu : 14,538,832|14,538,832|0|0.0%
=== 8.x..1896060 compared (5199785bef0ea..519978ce18449):

ct  : 37,287|37,359|72|0.2%
wt  : 233,934|233,240|-694|-0.3%
cpu : 228,014|228,014|0|0.0%
mu  : 14,406,880|14,454,392|47,512|0.3%
pmu : 14,538,832|14,585,504|46,672|0.3%
alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Can we explain the 1% slow down in scenario 1?

joelpittet’s picture

Assigned: Unassigned » joelpittet

Going to run again and try to explain.

joelpittet’s picture

Ok 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.

=== 8.x..8.x compared (519af44245f83..519af4a154bbb):

ct  : 63,134|63,134|0|0.0%
wt  : 587,813|588,322|509|0.1%
cpu : 543,456|541,759|-1,697|-0.3%
mu  : 39,675,544|39,675,544|0|0.0%
pmu : 39,841,240|39,841,240|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519af44245f83&...

=== 8.x..1896060-twig-aggregator compared (519af44245f83..519af4b90e33b):

ct  : 63,134|64,332|1,198|1.9%
wt  : 587,813|590,090|2,277|0.4%
cpu : 543,456|545,305|1,849|0.3%
mu  : 39,675,544|39,756,760|81,216|0.2%
pmu : 39,841,240|39,918,384|77,144|0.2%

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.

=== 8.x..8.x compared (519afa8ec2e5c..519afb15356f3):

ct  : 51,339|51,339|0|0.0%
wt  : 530,742|532,515|1,773|0.3%
cpu : 477,875|479,569|1,694|0.4%
mu  : 38,926,960|38,926,960|0|0.0%
pmu : 39,073,560|39,073,560|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519afa8ec2e5c&...

=== 8.x..1896060-twig-aggregator compared (519afa8ec2e5c..519afb4e24ae7):

ct  : 51,339|51,411|72|0.1%
wt  : 530,742|529,798|-944|-0.2%
cpu : 477,875|479,365|1,490|0.3%
mu  : 38,926,960|38,979,432|52,472|0.1%
pmu : 39,073,560|39,125,880|52,320|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519afa8ec2e5c&...

jenlampton’s picture

Assigned: joelpittet » Unassigned
Status: Needs review » Reviewed & tested by the community

less than 1% slower in one case, and faster in the next! Faster faster let's make drupal faster :)

alexpott’s picture

We still have an instance of aggregator wrapper in core...

    // Assemble output.
    $build = array(
      '#type' => 'container',
      '#attributes' => array('class' => array('aggregator-wrapper')),
    );
thedavidmeister’s picture

Status: Reviewed & tested by the community » Needs work

kk

mr.baileys’s picture

We still have an instance of aggregator wrapper in core...

What was removed in #293318: Convert Aggregator feeds into entities in was the use of the aggregrator-wrapper theme entry in aggregator's theme_hook. This patch cleans up by actually removing the aggregator-wrapper.tpl.php file, which is no longer being used.

There are three references to aggregator-wrapper left in core:

sudo grep -r "aggregator.wrapper" * -b2
core/modules/aggregator/aggregator.pages.inc-4769-    $build = array(
core/modules/aggregator/aggregator.pages.inc-4789-      '#type' => 'container',
core/modules/aggregator/aggregator.pages.inc:4819:      '#attributes' => array('class' => array('aggregator-wrapper')),
core/modules/aggregator/aggregator.pages.inc-4889-    );
core/modules/aggregator/aggregator.pages.inc-4896-    $build['feed_source'] = is_array($feed_source) ? $feed_source : array('#markup' => $feed_source);
--
core/modules/aggregator/aggregator.pages.inc-9946-  $build = array(
core/modules/aggregator/aggregator.pages.inc-9964-    '#type' => 'container',
core/modules/aggregator/aggregator.pages.inc:9992:    '#attributes' => array('class' => array('aggregator-wrapper')),
core/modules/aggregator/aggregator.pages.inc-10060-    '#sorted' => TRUE,
core/modules/aggregator/aggregator.pages.inc-10083-  );
--
core/modules/aggregator/aggregator.pages.inc-11281-  $build = array(
core/modules/aggregator/aggregator.pages.inc-11299-    '#type' => 'container',
core/modules/aggregator/aggregator.pages.inc:11327:    '#attributes' => array('class' => array('aggregator-wrapper')),
core/modules/aggregator/aggregator.pages.inc-11395-    '#sorted' => TRUE,
core/modules/aggregator/aggregator.pages.inc-11418-  );

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.)

mr.baileys’s picture

Status: Needs work » Needs review
alexpott’s picture

Title: aggregator.module - Convert PHPTemplate templates to Twig » [READY] aggregator.module - Convert PHPTemplate templates to Twig
Status: Needs review » Closed (duplicate)

Yep 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

alexpott’s picture

Issue summary: View changes

Updated issue summary.

ianthomas_uk’s picture

Issue summary: View changes

Add commit message

alexpott’s picture

Title: [READY] aggregator.module - Convert PHPTemplate templates to Twig » aggregator.module - Convert PHPTemplate templates to Twig
Status: Closed (duplicate) » Fixed

Committed 4cc09aa and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary (Remove READY from title).