Issue #1987390 by Cottser, vinmassaro, carsonevans, ezeedub: Aggregator.module - Convert theme_ functions to Twig.

Task

Convert theme_ functions to Twig templates.

Remaining

  • Patch review
  • Profiling

Theme function name Conversion status
theme_aggregator_block_item converted
theme_aggregator_categorize_items Converted to #type table (and committed) in #1938894: Convert aggregator theme tables to table #type
theme_aggregator_page_opml Will be converted in #1963544: Convert aggregator/rss to views
theme_aggregator_page_rss Will be converted in #1963544: Convert aggregator/rss to views
theme_aggregator_summary_item converted

#1896060: aggregator.module - Convert PHPTemplate templates to Twig
#1757550: [META-64] Convert core theme functions to Twig templates
#1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch

1) enable aggregator.module
2) visit the aggregator settings page and add a new feed here admin/config/services/aggregator/add/feed
3) import aggregator items here via the drop module /admin/config/services/aggregator
4) test the aggregator item output here aggregator/sources/1

Files: 
CommentFileSizeAuthor
#131 aggregator-twig-1987390-131.patch587 bytesTim Bozeman
PASSED: [[SimpleTest]]: [MySQL] 58,725 pass(es).
[ View ]
#131 interdiff.txt585 bytesTim Bozeman
#129 aggregator-twig-1987390-129.patch587 bytesTim Bozeman
PASSED: [[SimpleTest]]: [MySQL] 58,426 pass(es).
[ View ]
#129 interdiff.txt585 bytesTim Bozeman
#125 aggregator-twig-1987390-125.patch587 bytesTim Bozeman
PASSED: [[SimpleTest]]: [MySQL] 58,292 pass(es).
[ View ]
#112 aggregator-twig-1987390-112.patch4.79 KBmdrummond
PASSED: [[SimpleTest]]: [MySQL] 59,075 pass(es).
[ View ]
#107 twig-aggregator-1987390-107.patch5.07 KBmdrummond
PASSED: [[SimpleTest]]: [MySQL] 58,629 pass(es).
[ View ]
#103 twig-aggregator-theme-1987390-103.patch1.55 MBmdrummond
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-aggregator-theme-1987390-103.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#103 interdiff.txt1.46 KBmdrummond
#98 1987390-twig-aggregator-theme-98.patch5.07 KBmdrummond
PASSED: [[SimpleTest]]: [MySQL] 58,487 pass(es).
[ View ]
#86 1987390-86-twig-aggregator-theme.patch5.08 KBgnuget
PASSED: [[SimpleTest]]: [MySQL] 57,952 pass(es).
[ View ]
#82 twig-1987390-81.patch4.07 KBazinoman
FAILED: [[SimpleTest]]: [MySQL] 58,123 pass(es), 0 fail(s), and 6 exception(s).
[ View ]
#75 interdiff.txt10.15 KBjoelpittet
#75 1987390-75-twig-aggregator-theme.patch5.21 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987390-75-twig-aggregator-theme.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#70 1987390-16.patch12.37 KBdrupalninja99
PASSED: [[SimpleTest]]: [MySQL] 56,444 pass(es).
[ View ]
#70 interdiff.txt1008 bytesdrupalninja99
#66 1987390-15.patch12.35 KBdrupalninja99
PASSED: [[SimpleTest]]: [MySQL] 55,884 pass(es).
[ View ]
#66 interdiff.txt1.1 KBdrupalninja99
#65 8x-head-rss.txt24.32 KBdrupalninja99
#62 interdiff-7-60.txt3.8 KBjoelpittet
#61 rss-without-print-or-exit.txt38.87 KBdrupalninja99
#61 rss-without-exit.txt26.23 KBdrupalninja99
#61 rss-with-exit.txt17.84 KBdrupalninja99
#60 1987390-14.patch12.54 KBdrupalninja99
PASSED: [[SimpleTest]]: [MySQL] 55,896 pass(es).
[ View ]
#52 1987390-13.patch12.55 KBdrupalninja99
PASSED: [[SimpleTest]]: [MySQL] 55,968 pass(es).
[ View ]
#52 interdiff.txt490 bytesdrupalninja99
#50 1987390-12.patch12.53 KBdrupalninja99
PASSED: [[SimpleTest]]: [MySQL] 55,947 pass(es).
[ View ]
#43 1987390-11.patch9.34 KBdrupalninja99
FAILED: [[SimpleTest]]: [MySQL] 56,870 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#39 1987390-10.patch9.1 KBdrupalninja99
FAILED: [[SimpleTest]]: [MySQL] 57,913 pass(es), 0 fail(s), and 8 exception(s).
[ View ]
#38 1987390-9.patch9.12 KBdrupalninja99
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#31 1987390-8.patch12.34 KBdrupalninja99
PASSED: [[SimpleTest]]: [MySQL] 56,128 pass(es).
[ View ]
#29 langcode-fix.patch.txt958 bytesdrupalninja99
#22 1987390-7.patch12.07 KBdrupalninja99
PASSED: [[SimpleTest]]: [MySQL] 55,666 pass(es).
[ View ]
#21 1987390-7.patch12.07 KBdrupalninja99
PASSED: [[SimpleTest]]: [MySQL] 55,723 pass(es).
[ View ]
#21 adding_comments.txt2 KBdrupalninja99
#7 1987390-6.patch11.79 KBvinmassaro
PASSED: [[SimpleTest]]: [MySQL] 55,895 pass(es).
[ View ]
#4 1987390-4.patch12.74 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 57,016 pass(es).
[ View ]
#2 1987390-2.patch10.44 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,554 pass(es).
[ View ]
#2 interdiff.txt1.01 KBCottser
#1 1987390-1.patch10.44 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,628 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new10.44 KB
PASSED: [[SimpleTest]]: [MySQL] 55,628 pass(es).
[ View ]

Initial patch, I noticed a couple of the preprocess docs should be tweaked to match #1913208: [policy] Standardize template preprocess function documentation.

Issue summary:View changes

Add conversion chart and update remaining

Issue summary:View changes

Updated issue summary.

StatusFileSize
new1.01 KB
new10.44 KB
PASSED: [[SimpleTest]]: [MySQL] 55,554 pass(es).
[ View ]

Here are the preprocess doc tweaks.

Issue summary:View changes

Add aggregator theme_ function conversions to related

Title:aggregator.module - Convert PHPTemplate templates to Twigaggregator.module - Convert theme_ functions to Twig
Status:Needs review» Needs work

We've reversed the purpose here. Per #1757550-44: [META-64] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions rather than templates. See #1896060: aggregator.module - Convert PHPTemplate templates to Twig for template conversion instead.

Issue summary:View changes

Updated issue summary.

Status:Needs work» Needs review
StatusFileSize
new12.74 KB
PASSED: [[SimpleTest]]: [MySQL] 57,016 pass(es).
[ View ]

Here are just the theme_ function conversions.

Issue summary:View changes

Updated issue summary.

#4: 1987390-4.patch queued for re-testing.

Assigned:Unassigned» vinmassaro

Going to reroll and attempt to profile.

StatusFileSize
new11.79 KB
PASSED: [[SimpleTest]]: [MySQL] 55,895 pass(es).
[ View ]

I am profiling this

Assigned:vinmassaro» Unassigned

bbranches 519fe4a4d98a1 profile_aggregator
Warning: Must specify directory location for XHProf runs. Trying /tmp as default. You can either pass the directory location as an argument to the constructor for XHProfRuns_Default() or set xhprof.output_dir ini param.
=== 8.x..8.x compared (519fe4a4d98a1..519fe4f3d483f):

ct : 22,777|22,777|0|0.0%
wt : 333,158|332,921|-237|-0.1%
cpu : 324,698|324,076|-622|-0.2%
mu : 35,277,512|35,277,512|0|0.0%
pmu : 35,389,704|35,389,704|0|0.0%

Profiler output

Warning: Must specify directory location for XHProf runs. Trying /tmp as default. You can either pass the directory location as an argument to the constructor for XHProfRuns_Default() or set xhprof.output_dir ini param.
=== 8.x..profile_aggregator compared (519fe4a4d98a1..519fe521c4e93):

ct : 22,777|22,777|0|0.0%
wt : 333,158|331,702|-1,456|-0.4%
cpu : 324,698|322,947|-1,751|-0.5%
mu : 35,277,512|35,277,888|376|0.0%
pmu : 35,389,704|35,390,504|800|0.0%

Profiler output

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

Issue tags:-Needs profiling

... am testing manually at the DC Twig sprint.

... am not testing manually, as my D8 install appears to now be broken.

Status:Needs review» Needs work
Issue tags:-Twig

The last submitted patch, 1987390-6.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Twig

#7: 1987390-6.patch queued for re-testing.

Issue summary:View changes

Updated issue summary.

added a feed block with 10 items on the home page.
hits aggregator-block-item.html.twig

=== 8.x..8.x compared (51a184397f8eb..51a184d5a049b):
ct  : 44,672|44,672|0|0.0%
wt  : 387,649|386,327|-1,322|-0.3%
cpu : 388,025|388,024|-1|-0.0%
mu  : 8,448,364|8,448,364|0|0.0%
pmu : 8,529,196|8,529,196|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a184397f8eb&...

=== 8.x..aggregator-1987390-7 compared (51a184397f8eb..51a1855d1e29c):
ct  : 44,672|45,151|479|1.1%
wt  : 387,649|391,959|4,310|1.1%
cpu : 388,025|388,025|0|0.0%
mu  : 8,448,364|8,464,348|15,984|0.2%
pmu : 8,529,196|8,545,192|15,996|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a184397f8eb&...

pointed home page to aggregator/rss/1
hits aggregator-page-rss.html.twig

=== 8.x..8.x compared (51a188154f4e0..51a188a760170):
ct  : 31,129|31,129|0|0.0%
wt  : 272,032|268,427|-3,605|-1.3%
cpu : 268,016|268,017|1|0.0%
mu  : 6,258,380|6,258,380|0|0.0%
pmu : 6,311,460|6,311,460|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a188154f4e0&...

=== 8.x..aggregator-1987390-7 compared (51a188154f4e0..51a18906335b0):
ct  : 31,129|38,788|7,659|24.6%
wt  : 272,032|416,375|144,343|53.1%
cpu : 268,016|344,022|76,006|28.4%
mu  : 6,258,380|6,867,432|609,052|9.7%
pmu : 6,311,460|6,962,336|650,876|10.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a188154f4e0&...

point home page to aggregator/sources
hits aggregator-summary-item.html.twig

=== 8.x..8.x compared (51a18dd97f04c..51a18e7fe7c93):
ct  : 99,488|99,488|0|0.0%
wt  : 753,823|753,231|-592|-0.1%
cpu : 748,047|744,046|-4,001|-0.5%
mu  : 7,912,704|7,912,704|0|0.0%
pmu : 8,030,528|8,030,528|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a18dd97f04c&...

=== 8.x..aggregator-1987390-7 compared (51a18dd97f04c..51a18f1daaf9e):
ct  : 99,488|101,729|2,241|2.3%
wt  : 753,823|771,713|17,890|2.4%
cpu : 748,047|764,048|16,001|2.1%
mu  : 7,912,704|7,946,428|33,724|0.4%
pmu : 8,030,528|8,064,204|33,676|0.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a18dd97f04c&...

set home page to aggregator/opml
hits aggregator-page-opml.html.twig

=== 8.x..8.x compared (51a191a350534..51a19213db58a):
ct  : 30,919|30,919|0|0.0%
wt  : 264,392|264,564|172|0.1%
cpu : 264,017|260,017|-4,000|-1.5%
mu  : 6,256,416|6,256,416|0|0.0%
pmu : 6,312,996|6,312,996|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a191a350534&...

=== 8.x..aggregator-1987390-7 compared (51a191a350534..51a19275db084):
ct  : 30,919|38,577|7,658|24.8%
wt  : 264,392|329,366|64,974|24.6%
cpu : 264,017|328,020|64,003|24.2%
mu  : 6,256,416|6,714,240|457,824|7.3%
pmu : 6,312,996|6,821,252|508,256|8.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a191a350534&...

Issue tags:+Needs profiling

aggregator-page-rss.html.twig and aggregator-page-opml.html.twig need a second look

Status:Needs review» Needs work

Here's some cleanup that needs to be done on the patch in #7:

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -511,18 +515,19 @@ function aggregator_category_load($cid) {
+ * @param array $variables
  *   An associative array containing:
  *   - item: The item to be displayed.
  *   - feed: Not used.

Neither `item` nor `feed` are used in this function. The comment should contain a description of`url` and `title`

+++ b/core/modules/aggregator/templates/aggregator-page-opml.html.twigundefined
@@ -0,0 +1,30 @@
+ * Available variables:
+ * - title: @todo.
+ * - date: @todo.
+ * - feeds: @todo.
+ *   - feed.title: @todo.
+ *   - feed.url: @todo.

These @todo's should be filled out.

+++ b/core/modules/aggregator/templates/aggregator-page-rss.html.twigundefined
@@ -0,0 +1,31 @@
+    {{ args }}

This doesn't have a comment in the doc block. I think it should.

StatusFileSize
new2 KB
new12.07 KB
PASSED: [[SimpleTest]]: [MySQL] 55,723 pass(es).
[ View ]

@Thomas, here is my attempt to apply those requested changes in #20 in patch #7. I have re-rolled the #7 patch and included my diff as well. I don't submit patches very often so forgive me if I did anything wrong...

Status:Needs work» Needs review
StatusFileSize
new12.07 KB
PASSED: [[SimpleTest]]: [MySQL] 55,666 pass(es).
[ View ]

Here we go again with needs review status...

Thats a serious performance regression :s

I am using the aggregator_test module to compare output against HEAD, 2 things:

1. I opened an issue here http://drupal.org/node/2004622 bc there is Drupal block content showing up on the test feed page
2. Should I create another issue to add more test pages to the aggregator_test module? We only test aggregator_test_rss091.xml but that module has 2 more test xml files and we have no OPML .xml tests that I am aware of. Would it be useful for the purposes of comparing output before and after to add more tests to that module?

RSS before and after tests
RSS URL used: http://rss.cnn.com/rss/cnn_topstories.rss

Test 1 - compare output of block and page:

Steps:
1. Added aggregator block to left sidebar
2. Tested /aggregator/sources/1 & /aggregator pages

Result: PASS, no differences

Test 2 - compare RSS output:

Steps:
1. Tested /aggregator/rss

Result: FAIL, unable to render RSS

With patch "1987390-7.patch" I received this error:
Notice: Use of undefined constant LANGUAGE_TYPE_CONTENT - assumed 'LANGUAGE_TYPE_CONTENT' in template_preprocess_aggregator_page_rss() (line 481 of core/modules/aggregator/aggregator.pages.inc).

2 Problems at at /aggregator/rss, function aggregator_page_rss():

1. The LANGUAGE_TYPE_CONTENT is not defined and we don't need to check it anyway. I am attaching a patch where we just leave the langcode as is if no $category['langcode'] is found. Let me know if this works
2. The RSS output renders in the content area of the page instead of on it's own (with no Drupal page markup wrapper). I just need to know how to make it print without the wrapper and I could submit a patch to fix this.

I'm not sure whether we need to use Twig in aggregator. See #1839468: [Followup] Replace aggregator rss parsing with Zend Feed

StatusFileSize
new958 bytes

Attaching langcode diff

@Lin I think we would need twig for the non-XML output theme functions. I doubt those would change bc Zend wouldn't replace that. As far as /aggregator/rss (not sure ab aggregator/opml) - those twig/theme functions might go away in favor of Zend (I suppose).

StatusFileSize
new12.34 KB
PASSED: [[SimpleTest]]: [MySQL] 56,128 pass(es).
[ View ]

OK I fixed some things and re-rolled back into 1 patch. Here are my fixes:

1. Work-around for the langcode issue is to make it NULL so we at least don't get an error, but there is probably a more elegant way to find the current site language. When langcode is null you get <language></language> in the RSS
2. I added print theme('..'); exit; to both the aggregator page rss and opml page functions. This outputs the XML without the Drupal wrapper. I am not sure if this is the proper way to do this.
3. I fixed the twig files which were unnecessarily escaping double quotes with \" in some cases, like the rss page twig file.

The /aggregator/rss is now rendering much better. The only difference I have seen so far is the lack of a language code.

Assigned:Unassigned» TrevorBradley

I'll give profiling the new patch a shot. I'll start working on profiles for all four twig templates.

Assigned:TrevorBradley» Unassigned
Status:Needs review» Needs work

Hopefully I get this right, but it looks like this needs work. I imported a RSS feed from CNN's Top Stories. With the patch applied, I get a white screen (0 byte file) for the aggregator/rss URL, with no errors in the apache error log. Returning to the 8.x branch I see a nice page displayed.

(Don't take my word for it though! I did do a git pull and git rebase to make sure there weren't any errors, but it could be tester error!)

I suppose you cleared caches? Dd you make sure to import some items?

I suppose you cleared caches? Dd you make sure to import some items?

I suppose you cleared caches? Dd you make sure to import some items?

Status:Needs work» Needs review

I did import items but did not clear caches. Unfortunately this was pointed out to me just as I was leaving Acquia today. Changing status back. Sorry everybody!

EDIT: Verified this is a caching issue. Difficult to fix in the car's back seat back to Vancouver, BC. I'll pass this on to someone else.

StatusFileSize
new9.12 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Added a fix for the language code. Now the RSS is identical with 1 caveat: the HEAD rss page does not have an exit(); after it prints the rss output, so it continues rentering html on the page. I have to patch that page to get a true comparison.

It is a bit debatable perhaps if we need twig files for RSS/opml output at all. There is an API function format_rss_channel() in HEAD that we could continue to use and not convert theme_aggregator_page_rss() to twig at all. If the goal is to kill every theme function no matter what than we can proceed, I just was thinking that I doubt RSS output is overridden all that often.

StatusFileSize
new9.1 KB
FAILED: [[SimpleTest]]: [MySQL] 57,913 pass(es), 0 fail(s), and 8 exception(s).
[ View ]

Actually the LANGUAGE_TYPE_CONTENT constant was still giving me errors, I think that has been eliminated in 8. I went with the simpler language_default()->langcode which works for me with no errors.

use Drupal\Core\Language\Language;
language(Language::TYPE_CONTENT)->langcode

Status:Needs review» Needs work

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

@Tim, gotcha thanks. I wasn't familiar with the new syntax we needed.

Status:Needs work» Needs review
StatusFileSize
new9.34 KB
FAILED: [[SimpleTest]]: [MySQL] 56,870 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

I have added Tim's language fix to my last patch, this is ready for review.

Status:Needs review» Needs work
Issue tags:-Needs profiling, -Twig

The last submitted patch, 1987390-11.patch, failed testing.

Status:Needs work» Needs review

#43: 1987390-11.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs profiling, +Twig

The last submitted patch, 1987390-11.patch, failed testing.

Hmm, ya I have seen that bug before, seems like a views problem but I could investigate.

Hi @drupalninja99 thank you very much for giving a go at this conversion!

So a couple general suggestions:

  • Please provide an interdiff.txt with your patch so that it's clear what has changed between your latest patch and the patch before. If you're not sure how have a look here: https://drupal.org/documentation/git/interdiff but basically it's a diff between the latest changes and the changes before and if it's on a branch it's a diff between your latest commit HEAD and the previous commit HEAD^, shortcut: git show > interdiff.txt
  • If your editor has it, turn on removing ending whitespace, the patches apply cleanly. If you have dreditor browser userscript installed these show up as red. http://drupal.org/project/dreditor
  • I may have missed something but this patch has no twig templates in it, did it go missing?
  • I am quite sure the exit() calls aren't the way to go about this. Most likely a renderable array would be the case.

Below are some specific items:

+++ b/core/modules/aggregator/aggregator.module
@@ -511,18 +515,19 @@ function aggregator_category_load($cid) {
  *
- * @param $variables
- *   An associative array containing:
- *   - item: The item to be displayed.
- *   - feed: Not used.
+ * Default template: aggregator-block-item.html.twig.
  *
- * @ingroup themeable
+ * @param array $variables
+ *   An associative array containing:
+ *   - url: URL to the feed item.
+ *   - title: Title of the feed item.

So the @param isn't to tell you what is coming out but what is coming in. So the original item and feed variables should stay.

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -416,55 +417,81 @@ function aggregator_page_rss() {
   }
   $feeds = $result->fetchAll();
-  return theme('aggregator_page_rss', array('feeds' => $feeds, 'category' => $category));
+  ¶
+  print theme('aggregator_page_rss', array('feeds' => $feeds, 'category' => $category));
+  exit;
}

Not sure why this is printing and exiting, this looks like a mistake.

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -416,55 +417,81 @@ function aggregator_page_rss() {
+  // We strip all HTML tags, but need to prevent double encoding from properly
+  // escaped source data (such as &amp becoming &amp;amp;).
+  $variables['description'] = check_plain(decode_entities(strip_tags($description)));
+  $variables['items'] = $items;
+  ¶
+  /**
+   * Use RSS language code if found, otherwise default to existing page language code ¶
+   */
+  if (isset($category['langcode']))
+    $variables['langcode'] = check_plain($category['langcode']);
+  // Try to default to the site language code
+  elseif (!isset($variables['langcode']))    ¶
+    $variables['langcode'] = language(Language::TYPE_CONTENT)->langcode;

There is some ending whitespace in the patch, if your editor has it turn on it's remove ending whitespace preference.

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -488,37 +515,32 @@ function aggregator_page_opml($cid = NULL) {
   $feeds = $result->fetchAll();
-  return theme('aggregator_page_opml', array('feeds' => $feeds));
+  print theme('aggregator_page_opmlr', array('feeds' => $feeds));
+  exit;
}

This should probably be returned as a renderable array. eg.array('#theme' => 'aggregator_page_opml', '#feeds' => $feeds);

Cheers, keep up the good work:)

#31 Has the twig templates in them, so you had them they may have just not got added to the commit.

@drupalninja99 here's my workflow if it helps: http://pittet.ca/drupal/patch the same as https://drupal.org/node/707484

Status:Needs work» Needs review
StatusFileSize
new12.53 KB
PASSED: [[SimpleTest]]: [MySQL] 55,947 pass(es).
[ View ]

Hi Joel, thank you for your help. Yes I did my last diff wrong, I am attaching the latest with all of the twig files and your recommended changes including cleaning up some extraneous whitespace.

The thing I haven't figured out is how to get the print to work without the exit. I agree normally you return the renderable array, not print/exit. The issue is that we are not display content within the Drupal page layout. We are outputting only XML to the screen and nothing else. In Drupal 6/7 if you start printing in a page handle it will not display the output in the Drupal layout.

If you just return the renderable array you get XML output in the page layout. That is definitely what we don't want. If you instead print the output you get halfway there, the page starts with just the XML output but then it ends with Drupal page markup which i invalid. So I don't know the proper Drupal8 way to do that. I am trying to find another module that has to do something similar so that I can see how they do it.

I am attaching my latest patch.

This is the closest comp I can see as to how to resolve the exit problem,

If the XMLRPC module in xmlrpc.server.inc it does an echo and then a drupal_exit(); so I will add the same to my patch.

function xmlrpc_server_output($xml) {
  $xml = '<?xml version="1.0"?>' . "\n" . $xml;
  drupal_add_http_header('Content-Length', strlen($xml));
  drupal_add_http_header('Content-Type', 'text/xml');
  echo $xml;
  drupal_exit();
}

StatusFileSize
new490 bytes
new12.55 KB
PASSED: [[SimpleTest]]: [MySQL] 55,968 pass(es).
[ View ]

I have added drupal_exit and re-tested from a clean install. This is looking pretty good, I am attaching patch 13 + the interdiff.

Looking at the drupal_exit() code that I borrowed from xmlrpcs, this was lifted straight from an xmlrpc Drupal7 include so I think it's proper to use it in our use case.

I also ran the Drupal\aggregator\Tests\AggregatorRenderingTest simple test locally and received no errors this time.

Status:Needs review» Needs work
Issue tags:-Needs profiling, -Twig

The last submitted patch, 1987390-13.patch, failed testing.

Status:Needs work» Needs review

#52: 1987390-13.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1987390-13.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs profiling, +Twig

#52: 1987390-13.patch queued for re-testing.

Thanks for adding back the files and providing an interdiff!

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -416,55 +417,81 @@ function aggregator_page_rss() {
   $feeds = $result->fetchAll();
-  return theme('aggregator_page_rss', array('feeds' => $feeds, 'category' => $category));
+  ¶
+  echo theme('aggregator_page_rss', array('feeds' => $feeds, 'category' => $category));
+  drupal_exit();

There is whitespace errors in the files.

And I really don't think we need the exit and print, we should be able to leave it as just return statement. Or maybe I'm missing something?

Also, please use print instead of echo. I like echo too, but Drupal uses print exclusively.

StatusFileSize
new12.54 KB
PASSED: [[SimpleTest]]: [MySQL] 55,896 pass(es).
[ View ]

I turned on remove trailing whitespace in Netbeans and changed out the echo statements with print. I will explain in my next comment why we need drupal_exit();

StatusFileSize
new17.84 KB
new26.23 KB
new38.87 KB

Re: "Or maybe I'm missing something?" - yes.

You can see the 3 options and their corresponding output:

1. (current) print + exit means you get just the XML, see rss-with-exit.txt
2. print + no exit gets you extra Drupal markup after the XML prints, see rss-without-exit.txt. This means your RSS is invalid.
3. No print (just return) + no exit means you get the complete Drupal page with XML in the content area, which obviously is not valid, see rss-without-print-or-exit.txt.

I am pretty sure you have to have the drupal_exit() for the same reason XML RPC uses it and has used it since Drupal 7.

StatusFileSize
new3.8 KB

ok @drupalninja99 attached is an interdiff between comment #7 and your comment #60.

This issue is to be as close to a straight conversion as possible. All other fixes to broken bits should have their own issue. So please confirm that the the way without the twig template is producing the expected results and that the addition print + exit is necessary for the conversion?

I'll review the other bits of this interdiff in the next comment.

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -7,6 +7,7 @@
+use Drupal\Core\Language\Language;

Great, this is definitely needed now:)

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -416,11 +417,9 @@ function aggregator_page_rss() {
-  return array(
-    '#theme' => 'aggregator_page_rss',
-    '#feeds' => $feeds,
-    '#category' => $category,
-  );
+
+  print theme('aggregator_page_rss', array('feeds' => $feeds, 'category' => $category));
+  drupal_exit();
}

This looks like a regression, we are trying to avoid using theme() call's and instead returning renderable arrays.

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -478,8 +477,14 @@ function template_preprocess_aggregator_page_rss(&$variables) {
-  $langcode = (isset($category['langcode']) ? $category['langcode'] : language(LANGUAGE_TYPE_CONTENT)->langcode);
-  $variables['langcode'] = check_plain($langcode);
+  /**
+   * Use RSS language code if found, otherwise default to existing page language code
+   */
+  if (isset($category['langcode']))
+    $variables['langcode'] = check_plain($category['langcode']);
+  // Try to default to the site language code
+  elseif (!isset($variables['langcode']))
+    $variables['langcode'] = language(Language::TYPE_CONTENT)->langcode;

The original ternary operator was fine and clear, just need to change the constant to use the Language::TYPE_CONTENT static. If you a really think it needs a comment, don't use the docblock style inside the function. Inline comment style please.

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -510,7 +515,8 @@ function aggregator_page_opml($cid = NULL) {
   $feeds = $result->fetchAll();
-  return theme('aggregator_page_opml', array('feeds' => $feeds));
+  print theme('aggregator_page_opml', array('feeds' => $feeds));
+  drupal_exit();

The aim here is to have this return a renderable array.

+++ b/core/modules/aggregator/templates/aggregator-page-opml.html.twig
@@ -4,11 +4,11 @@
- * - title: @todo.
- * - date: @todo.
- * - feeds: @todo.
- *   - feed.title: @todo.
- *   - feed.url: @todo.
+ * - title: Title of the feed.
+ * - date: Last date feed was modified.
+ * - feeds: An associative array of feed items containing:
+ *   - feed.title: Title of the feed item.
+ *   - feed.url: URL to the feed item.
  *
  * @see template_preprocess()

Thank you! no more @todos:)

+++ b/core/modules/aggregator/templates/aggregator-page-opml.html.twig
@@ -16,8 +16,8 @@
-<?xml version="1.0" encoding="utf-8"?>
-<opml version="1.1">
+<?xml version="1.0" encoding="utf-8"?>
+<opml version="1.1">
   <head>
     <title>{{ title }}</title>

Great that is a good catch.

+++ b/core/modules/aggregator/templates/aggregator-page-rss.html.twig
@@ -16,8 +17,8 @@
-<?xml version="1.0" encoding="utf-8"?>
-<rss version="2.0">
+<?xml version="1.0" encoding="utf-8"?>
+<rss version="2.0">

Same here, nice one!

Status:Needs review» Needs work

StatusFileSize
new24.32 KB

Re: "This looks like a regression, we are trying to avoid using theme() call's and instead returning renderable arrays."

I fully understand what you are saying. I wish Drupal had a way to specify if output should not render the Drupal layout. 8.x HEAD is currently broken because it does not use an exit. What you get is a lot of layout html at the end of the file. So if we want to use return and not print + exit then it will continue to be broken.

Please look at the RSS output I just grabbed from /aggregator/rss on 8.x HEAD. This is not right. I don't have any other way to fix this than to use the convention that already has a precedent in the XML RPC module which dates back at least to Drupal 7 as well as D8. So I have no other ideas for how to handle this use case.

StatusFileSize
new1.1 KB
new12.35 KB
PASSED: [[SimpleTest]]: [MySQL] 55,884 pass(es).
[ View ]

Re "The original ternary operator was fine and clear" I have reverted this back to the original conditional with the new language change.

I don't have any other fixes though for the print/drupal_exit() logic.

Status:Needs work» Needs review

Ok thanks again @drupalninja99 so it seems this is an issue that has existed for a while and nobody has complained.

Unfortunately I don't have a good solution either for this issue. What you have done makes sense to me though I haven't seen that approach yet in my travels through twig conversion land. Also, d8 was or is supposed to have help for this in the WSSCI initiative, to make sure it doesn't pretend everything is an HTML page(heard this in a talk a while back).

The only way we have dealt with these so far is that we create a follow-up issue related to the specific problem. Leave these issues to as straight conversion as we can. Also, the fix may get in to 8.x faster if it's clear as well what the problem is and what the print/exit's are trying to solve.

If you would like I could help you create a followup issue and we can split the patch between them?

Hi Joel, re: "If you would like I could help you create a followup issue and we can split the patch between them?" - can you go ahead and create an issue regarding the problem? The issue is that we have no way to specify that this is a stand-alone page with no Drupal layout. In my view, I wish we had a way to specify this in the menu hook. So instead of:

$items['aggregator/rss'] = array(
    'title' => 'RSS feed',
    'page callback' => 'aggregator_page_rss',
    'access arguments' => array('access news feeds'),
    'type' => MENU_CALLBACK,
    'file' => 'aggregator.pages.inc',
  );

We would have a new 'type' constant called "MENU_CUSTOM" or "MENU_NO_LAYOUT" or something like that.

Another option would be to have a parameter in the render array that's returned. So here you would add something additional parameter telling it not to render the full layout.

return array(
    '#theme' => 'aggregator_page_rss',
    '#feeds' => $feeds,
    '#category' => $category,
  );

StatusFileSize
new1008 bytes
new12.37 KB
PASSED: [[SimpleTest]]: [MySQL] 56,444 pass(es).
[ View ]

I have added another patch where I remove the print/exit() code per Joel's request in lieu of a new ticket to address the output problem. Note that this effectively breaks the RSS/OPML pages as now they will render XML within the content area of the page.

@drupalninja99 thanks: Here's a the new issue for you
#2010956: aggregator_page_rss and aggregator_page_opml are producing HTML after their root elements. You are more than welcome to update the issue summary with better details on the problem. And I would suggest posting the print/exit bits to kick off the dialog:) Maybe someone can help with an even better solution.

So where do we stand with this patch?

Patch looks good, we just need to do some profiling, and to help out write out the scenarios from #15 to #18 to profile would be great so others can do them or confirm them.

Would like to see some manual testing on this, I see some results from @drupalninja99 in #25 and #26 but many changes since then, I'd like someone else to please run through this :)

As @joelpittet said - writing up manual testing steps would be extremely helpful for manual testing as well as profiling. Tagging as needs STR as well.

Issue summary:View changes

add attribution

StatusFileSize
new5.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987390-75-twig-aggregator-theme.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new10.15 KB

Updated the summary to remove:

theme_aggregator_page_opml
theme_aggregator_page_rss

Will be converted in #1963544: Convert aggregator/rss to views

Removed the theme call around datetime
Removed the aggregator-page-rss.html.twig and aggregator-page-opml.html.twig templates and reverted their theme functions.
Removed the left over theme_aggregator_summary_item

Did a markup comparison on the block and it's rendering perfect. Not sure where to find the other templates at the moment.

Status:Needs review» Needs work

failed testing needs to be rerolled

Status:Needs work» Needs review

test bot passed it, let's give it another go.

no Jen I'm pretty sure this one is gonna need a reroll

Issue tags:+Needs reroll

Status:Needs review» Needs work

The last submitted patch, 1987390-75-twig-aggregator-theme.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.07 KB
FAILED: [[SimpleTest]]: [MySQL] 58,123 pass(es), 0 fail(s), and 6 exception(s).
[ View ]

I rerolled this.

Status:Needs review» Needs work

The last submitted patch, twig-1987390-81.patch, failed testing.

Thanks @azinoman! The rerolled patch is missing the changes to template_preprocess_aggregator_summary_item().

Before reroll:
4 files changed, 50 insertions, 28 deletions.

After reroll:
4 files changed, 42 insertions, 21 deletions.

(Stats coming from Dreditor)

Assigned:Unassigned» gnuget

Assigned:gnuget» Unassigned
Status:Needs work» Needs review
StatusFileSize
new5.08 KB
PASSED: [[SimpleTest]]: [MySQL] 57,952 pass(es).
[ View ]

Rerolled version attached

Issue summary:View changes

page_rss and page_rss being taken care of else where.

Issue summary:View changes

added how to test

Issue summary:View changes

added link

Status:Needs review» Reviewed & tested by the community

I followed the instructions to test this patch. Everything ran smoothly. This patch seems fine.

Status:Reviewed & tested by the community» Needs review
Issue tags:-Needs steps to reproduce, -Needs reroll

Thanks for testing @siccababes, this still needs manual testing and profiling before we can RTBC it though.

Updating tags because we have a fresh patch and it looks like we have testing steps in the issue summary.

RSS before and after tests
RSS URL used: http://rss.cnn.com/rss/cnn_topstories.rss

Test 1 - compare output of block and page:

Steps:
1. Added aggregator block to left sidebar
2. Tested /aggregator/sources/1 & /aggregator pages
3. Added permission for anonymous users to view news feeds

Result: PASS, no differences

Test 2 - compare RSS output:

Steps:
1. Tested /aggregator/rss

Result: PASS, no differences

I used daisy diff to compare output.

One note is that the RSS page is not currently rendering correctly in 8.x. It renders the XML first, then the entire HTML page (starting with DOCTYPE). This is a separate issue.

Issue tags:+Novice

manual testing is a novice task

I did some manual testing in #89, is there something else we need to do on this to satisfy manual testing?

no, that's fine. Apologies, I didn't see that and the tag was still on the issue.

One note is that the RSS page is not currently rendering correctly in 8.x. It renders the XML first, then the entire HTML page (starting with DOCTYPE). This is a separate issue.

It would be good if you could open an issue for this and link to it from here.

I think I filed an issue awhile back, I will take a look again. Also I am going to retest just to confirm that it's still broken in head, it's possible it's been fixed.

@thedavidmeister, okay great news RSS is working now, I think it got fixed in head maybe July 10th (or perhaps earlier?)
https://drupal.org/node/1963544

I updated my local copy and re-tested the RSS. Looks good.

Status:Needs review» Needs work
Issue tags:+Needs reroll

Tagging for reroll.

Assigned:Unassigned» mdrummond

Working on the reroll.

Status:Needs work» Needs review
StatusFileSize
new5.07 KB
PASSED: [[SimpleTest]]: [MySQL] 58,487 pass(es).
[ View ]

Here's the reroll. No conflicts needed to be resolved on rebase. Yo go, git!

Issue tags:-Needs reroll

Removing the "needs reroll" tag.

Assigned:mdrummond» Unassigned

Also, unassigning myself.

Some day I'll remember to do all these things at once.

Status:Needs review» Needs work

@mdrummond Thank you very much for the re-roll.

There is a couple clean-up items but this should be ready to go for profiling.

  1. +++ b/core/modules/aggregator/aggregator.pages.inc
    @@ -7,6 +7,7 @@
    +use Drupal\Core\Language\Language;

    I am pretty sure this isn't needed in the conversion as well.

  2. +++ b/core/modules/aggregator/templates/aggregator-block-item.html.twig
    @@ -0,0 +1,16 @@
    + * @see template_preprocess()
    +++ b/core/modules/aggregator/templates/aggregator-summary-item.html.twig
    @@ -0,0 +1,16 @@
    + * @see template_preprocess()

    These can be removed.

Issue tags:+Novice

tag

Status:Needs work» Needs review
StatusFileSize
new1.46 KB
new1.55 MB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-aggregator-theme-1987390-103.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Made the updates requested.

Status:Needs review» Needs work

The last submitted patch, twig-aggregator-theme-1987390-103.patch, failed testing.

@mdrummond You may have forgot to rebase? 1.55MB is a bit big for a patch.

0_o

StatusFileSize
new5.07 KB
PASSED: [[SimpleTest]]: [MySQL] 58,629 pass(es).
[ View ]

Here is the reroll of the patch in #86 plus the changes requested in #101. This patch is only 5K, so hopefully should be correct.

Status:Needs work» Needs review

@mdrummond thanks for the re-roll! That looks like it should work. Though you may have forgot to post the changes from the interdiff you had in #103 that fixed up #100

Going to test them locally though to see if they pass.

Oh @mdrummond you may know this already but you can apply interdiffs against your git repo too.

curl https://drupal.org/files/interdiff_9270.txt | git apply

Edit: This is applied against your latest working directory should apply cleanly.

The removal of the use Drupal\Core\Language\Language; didn't fail my tests locally so that's cool. Give it a go and see what testbot says.

StatusFileSize
new4.79 KB
PASSED: [[SimpleTest]]: [MySQL] 59,075 pass(es).
[ View ]

Okay, I've gone through this again, and it should be correct. Yes, the changes in the interdiff aren't showing up in the patch, but the end result is correct. The changes requested in #101 took effect when I applied the patch against HEAD. So... I think we're good to go. We'll see if testbot agrees.

Issue tags:-Novice

Sweet on to profiling!

Assigned:Unassigned» joelpittet

profiling

Not totally convinced by my profiling results because of the -1 ct, so probably need another round of testing:

Scenario:

  • Aggregator module turned on
  • Anonymous permissions set for aggregator
  • Front page set to aggregator/sources
  • Left sidebar "Aggregator feed" block placed

=== 8.x..8.x compared (523c58bd29e4d..523c5919acc8c):
ct  : 57,404|57,403|-1|-0.0%
wt  : 371,942|372,384|442|0.1%
cpu : 337,329|335,958|-1,371|-0.4%
mu  : 13,734,376|13,734,360|-16|-0.0%
pmu : 13,797,528|13,797,432|-96|-0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523c58bd29e4d&...

=== 8.x..1987390-twig-aggregator-theme compared (523c58bd29e4d..523c5966f039b):
ct  : 57,404|58,760|1,356|2.4%
wt  : 371,942|378,121|6,179|1.7%
cpu : 337,329|343,548|6,219|1.8%
mu  : 13,734,376|13,811,832|77,456|0.6%
pmu : 13,797,528|13,874,272|76,744|0.6%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523c58bd29e4d&...

Status:Needs review» Needs work
  1. +++ b/core/modules/aggregator/aggregator.module
    @@ -449,18 +451,19 @@ function aggregator_category_load($cid) {
    +  $variables['url'] = check_url($variables['item']->link);

    should be String::checkPlain(Url::stripDangerousProtocols();

  2. +++ b/core/modules/aggregator/aggregator.module
    @@ -449,18 +451,19 @@ function aggregator_category_load($cid) {
    +  $variables['title'] = check_plain($variables['item']->title);
    +++ b/core/modules/aggregator/aggregator.pages.inc
    @@ -271,21 +256,20 @@ function template_preprocess_aggregator_summary_items(&$variables) {
    +  $variables['url'] = l(check_plain($item->label()), check_url(url($item->link->value, array('absolute' => TRUE))), array(

    Should be String::checkPlain().

Status:Needs work» Needs review

@jibran, I'm putting this back to CNR because check_plain() and check_url() are not deprecated functions. @webchick raised some concerns about the DX of using namespaces at the .module level over at #2093143: [meta] Remove calls to @deprecated and "backwards compatibility" procedural functions from core, so we should pick up discussion there :)

@jibran thanks, I think for this conversion issue we will leave that out, I'm sure there is an issue to convert those somewhere and if that get's in before this one, we'll re-roll.

If we regressed that change I'd change it right away.

Assigned:joelpittet» Unassigned
Issue tags:-Needs profiling

Ok re-done after some fixes to my laptop's apache/disabled xdebug:

Same scenario as #115

=== 8.x..8.x compared (523f527beec76..523f52fbabfcb):
ct  : 80,907|80,907|0|0.0%
wt  : 517,669|513,750|-3,919|-0.8%
cpu : 470,969|463,194|-7,775|-1.7%
mu  : 32,437,744|32,437,744|0|0.0%
pmu : 32,577,704|32,577,704|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f527beec76&...

=== 8.x..1987390-twig-aggregator-theme compared (523f527beec76..523f531e517da):
ct  : 80,907|81,346|439|0.5%
wt  : 517,669|513,611|-4,058|-0.8%
cpu : 470,969|463,807|-7,162|-1.5%
mu  : 32,437,744|32,465,152|27,408|0.1%
pmu : 32,577,704|32,604,112|26,408|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f527beec76&...

Status:Needs review» Reviewed & tested by the community

Profiled in #119
Manually tested in #89

Status:Reviewed & tested by the community» Fixed

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

Status:Fixed» Active

I'm not really a fan of re-opening issues but this is a one word change:

The docblock in aggregator-summary-item.html.twig refers to a 'link' variable, but it should be 'url'.

Issue tags:+Novice

And I would be remiss to miss the tag.

Assigned:Unassigned» Tim Bozeman

Yoink!

Assigned:Tim Bozeman» Unassigned
Status:Active» Needs review
StatusFileSize
new587 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,292 pass(es).
[ View ]

I changed line 7 of the docblock in aggregator-summary-item.html.twig from 'link' to 'url'.

Best mentor ever!
Cottser++

Status:Needs review» Reviewed & tested by the community
Issue tags:-Novice+Quick fix

Back to RTBC for the docblock fix. thanks @Tim Bozeman and @Cottser

Status:Reviewed & tested by the community» Needs work

Sorry, but the variable should be 'url' (lowercase). Thanks for picking this up @Tim Bozeman :)

Assigned:Unassigned» Tim Bozeman

Whoops!

Status:Needs work» Needs review
StatusFileSize
new585 bytes
new587 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,426 pass(es).
[ View ]

Devils in the details! I made the 'url' variable lowercase.

Status:Needs review» Needs work

Nice catch but you're URL for the description was right, just not the variable name. You can keep URL, no need to change it to Url. Sorry to be so picky, and not being picky enough before.

Status:Needs work» Needs review
StatusFileSize
new585 bytes
new587 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,725 pass(es).
[ View ]

Thank you @Cottser & @Joelpittet!

Third times the charm. This patch changes the docblock variable to 'url' and the description to 'URL'.

Status:Needs review» Reviewed & tested by the community

Thanks @Tim Bozeman now it's back to RTBC:)

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks! :)

PS: "Yoink!" made me laugh. :) Thanks for that.

Assigned:Tim Bozeman» Unassigned

:D

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

Issue summary:View changes

added links