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] 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

CommentFileSizeAuthor
#131 aggregator-twig-1987390-131.patch587 bytesTim Bozeman
#131 interdiff.txt585 bytesTim Bozeman
#129 aggregator-twig-1987390-129.patch587 bytesTim Bozeman
#129 interdiff.txt585 bytesTim Bozeman
#125 aggregator-twig-1987390-125.patch587 bytesTim Bozeman
#112 aggregator-twig-1987390-112.patch4.79 KBRainbowArray
#107 twig-aggregator-1987390-107.patch5.07 KBRainbowArray
#103 twig-aggregator-theme-1987390-103.patch1.55 MBRainbowArray
#103 interdiff.txt1.46 KBRainbowArray
#98 1987390-twig-aggregator-theme-98.patch5.07 KBRainbowArray
#86 1987390-86-twig-aggregator-theme.patch5.08 KBgnuget
#82 twig-1987390-81.patch4.07 KBazinoman
#75 interdiff.txt10.15 KBjoelpittet
#75 1987390-75-twig-aggregator-theme.patch5.21 KBjoelpittet
#70 1987390-16.patch12.37 KBdrupalninja99
#70 interdiff.txt1008 bytesdrupalninja99
#66 1987390-15.patch12.35 KBdrupalninja99
#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
#52 1987390-13.patch12.55 KBdrupalninja99
#52 interdiff.txt490 bytesdrupalninja99
#50 1987390-12.patch12.53 KBdrupalninja99
#43 1987390-11.patch9.34 KBdrupalninja99
#39 1987390-10.patch9.1 KBdrupalninja99
#38 1987390-9.patch9.12 KBdrupalninja99
#31 1987390-8.patch12.34 KBdrupalninja99
#29 langcode-fix.patch.txt958 bytesdrupalninja99
#22 1987390-7.patch12.07 KBdrupalninja99
#21 1987390-7.patch12.07 KBdrupalninja99
#21 adding_comments.txt2 KBdrupalninja99
#7 1987390-6.patch11.79 KBvinmassaro
#4 1987390-4.patch12.74 KBstar-szr
#2 1987390-2.patch10.44 KBstar-szr
#2 interdiff.txt1.01 KBstar-szr
#1 1987390-1.patch10.44 KBstar-szr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Status: Active » Needs review
FileSize
10.44 KB

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

star-szr’s picture

Issue summary: View changes

Add conversion chart and update remaining

star-szr’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

FileSize
1.01 KB
10.44 KB

Here are the preprocess doc tweaks.

star-szr’s picture

Issue summary: View changes

Add aggregator theme_ function conversions to related

c4rl’s picture

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

We've reversed the purpose here. Per #1757550-44: [Meta] 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.

c4rl’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

Status: Needs work » Needs review
FileSize
12.74 KB

Here are just the theme_ function conversions.

star-szr’s picture

Issue summary: View changes

Updated issue summary.

epersonae2’s picture

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

vinmassaro’s picture

Assigned: Unassigned » vinmassaro

Going to reroll and attempt to profile.

vinmassaro’s picture

FileSize
11.79 KB
carsonevans’s picture

I am profiling this

vinmassaro’s picture

Assigned: vinmassaro » Unassigned
carsonevans’s picture

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

carsonevans’s picture

Issue tags: -needs profiling
cafuego’s picture

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

ezeedub’s picture

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

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

ezeedub’s picture

Issue summary: View changes

Updated issue summary.

ezeedub’s picture

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

ezeedub’s picture

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

ezeedub’s picture

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

ezeedub’s picture

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

ezeedub’s picture

Issue tags: +needs profiling

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

tlattimore’s picture

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.

drupalninja99’s picture

FileSize
2 KB
12.07 KB

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

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
12.07 KB

Here we go again with needs review status...

aspilicious’s picture

Thats a serious performance regression :s

drupalninja99’s picture

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?

drupalninja99’s picture

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

drupalninja99’s picture

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.

linclark’s picture

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

linclark’s picture

drupalninja99’s picture

FileSize
958 bytes

Attaching langcode diff

drupalninja99’s picture

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

drupalninja99’s picture

FileSize
12.34 KB

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.

TrevorBradley’s picture

Assigned: Unassigned » TrevorBradley

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

TrevorBradley’s picture

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

drupalninja99’s picture

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

drupalninja99’s picture

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

drupalninja99’s picture

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

TrevorBradley’s picture

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.

drupalninja99’s picture

FileSize
9.12 KB

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.

drupalninja99’s picture

FileSize
9.1 KB

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.

tim.plunkett’s picture

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

Status: Needs review » Needs work

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

drupalninja99’s picture

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

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
9.34 KB

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.

gnuget’s picture

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.

drupalninja99’s picture

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

joelpittet’s picture

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

joelpittet’s picture

#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

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
12.53 KB

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.

drupalninja99’s picture

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();
}
drupalninja99’s picture

FileSize
490 bytes
12.55 KB

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

drupalninja99’s picture

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.

drupalninja99’s picture

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.

star-szr’s picture

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.

star-szr’s picture

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

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

joelpittet’s picture

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.

drupalninja99’s picture

FileSize
12.54 KB

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();

drupalninja99’s picture

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.

joelpittet’s picture

FileSize
3.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.

joelpittet’s picture

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

joelpittet’s picture

Status: Needs review » Needs work
drupalninja99’s picture

FileSize
24.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.

drupalninja99’s picture

FileSize
1.1 KB
12.35 KB

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.

drupalninja99’s picture

Status: Needs work » Needs review
joelpittet’s picture

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?

drupalninja99’s picture

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,
  );
drupalninja99’s picture

FileSize
1008 bytes
12.37 KB

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.

joelpittet’s picture

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

drupalninja99’s picture

So where do we stand with this patch?

joelpittet’s picture

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.

star-szr’s picture

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.

star-szr’s picture

Issue summary: View changes

add attribution

joelpittet’s picture

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.

azinoman’s picture

Status: Needs review » Needs work

failed testing needs to be rerolled

jenlampton’s picture

Status: Needs work » Needs review

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

azinoman’s picture

azinoman’s picture

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

azinoman’s picture

Issue tags: +Needs reroll

Status: Needs review » Needs work

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

azinoman’s picture

Status: Needs work » Needs review
FileSize
4.07 KB

I rerolled this.

Status: Needs review » Needs work

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

star-szr’s picture

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)

gnuget’s picture

Assigned: Unassigned » gnuget
gnuget’s picture

Assigned: gnuget » Unassigned
Status: Needs work » Needs review
FileSize
5.08 KB

Rerolled version attached

gnuget’s picture

Issue summary: View changes

page_rss and page_rss being taken care of else where.

siccababes’s picture

Issue summary: View changes

added how to test

siccababes’s picture

Issue summary: View changes

added link

siccababes’s picture

Status: Needs review » Reviewed & tested by the community

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

star-szr’s picture

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.

drupalninja99’s picture

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.

thedavidmeister’s picture

Issue tags: +Novice

manual testing is a novice task

drupalninja99’s picture

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

thedavidmeister’s picture

Issue tags: -Novice, -Needs manual testing

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

thedavidmeister’s picture

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.

drupalninja99’s picture

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.

drupalninja99’s picture

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

star-szr’s picture

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

Tagging for reroll.

RainbowArray’s picture

Assigned: Unassigned » RainbowArray

Working on the reroll.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
5.07 KB

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

RainbowArray’s picture

Issue tags: -Needs reroll

Removing the "needs reroll" tag.

RainbowArray’s picture

Assigned: RainbowArray » Unassigned

Also, unassigning myself.

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

joelpittet’s picture

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.

jenlampton’s picture

Issue tags: +Novice

tag

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
1.46 KB
1.55 MB

Made the updates requested.

Status: Needs review » Needs work

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

joelpittet’s picture

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

RainbowArray’s picture

0_o

RainbowArray’s picture

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.

RainbowArray’s picture

Status: Needs work » Needs review
joelpittet’s picture

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

joelpittet’s picture

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.

joelpittet’s picture

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.

RainbowArray’s picture

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.

joelpittet’s picture

Issue tags: -Novice

Sweet on to profiling!

joelpittet’s picture

Assigned: Unassigned » joelpittet

profiling

joelpittet’s picture

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

jibran’s picture

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

thedavidmeister’s picture

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

joelpittet’s picture

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

joelpittet’s picture

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

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Profiled in #119
Manually tested in #89

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

star-szr’s picture

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

star-szr’s picture

Issue tags: +Novice

And I would be remiss to miss the tag.

Tim Bozeman’s picture

Assigned: Unassigned » Tim Bozeman

Yoink!

Tim Bozeman’s picture

Assigned: Tim Bozeman » Unassigned
Status: Active » Needs review
FileSize
587 bytes

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

Best mentor ever!
Cottser++

joelpittet’s picture

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

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

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

Tim Bozeman’s picture

Assigned: Unassigned » Tim Bozeman

Whoops!

Tim Bozeman’s picture

Status: Needs work » Needs review
FileSize
585 bytes
587 bytes

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

joelpittet’s picture

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.

Tim Bozeman’s picture

Status: Needs work » Needs review
FileSize
585 bytes
587 bytes

Thank you @Cottser & @Joelpittet!

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

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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

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

Tim Bozeman’s picture

Assigned: Tim Bozeman » Unassigned

:D

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

Anonymous’s picture

Issue summary: View changes

added links