Files: 
CommentFileSizeAuthor
#22 drupal-core_replace_theme_aggregator-2008970-22.patch1.37 KBStephaneQ
PASSED: [[SimpleTest]]: [MySQL] 57,961 pass(es).
[ View ]
#22 interdiff-20-22.txt680 bytesStephaneQ
#20 drupal-core_replace_theme_aggregator-2008970-20.patch1.42 KBStephaneQ
PASSED: [[SimpleTest]]: [MySQL] 57,623 pass(es).
[ View ]
#12 twig-7563139-12.patch6.73 KBadamcowboy
PASSED: [[SimpleTest]]: [MySQL] 58,021 pass(es).
[ View ]
#6 drupal-core_replace_theme_aggregator-2008970-6.patch6.88 KBmarkie
PASSED: [[SimpleTest]]: [MySQL] 56,117 pass(es).
[ View ]
#2 drupal-core_replace_theme_aggregator-2008970-2.patch6.75 KBmarkie
PASSED: [[SimpleTest]]: [MySQL] 55,833 pass(es).
[ View ]

Comments

thedavidmeister’s picture

Issue tags:+Novice
markie’s picture

Assigned:Unassigned» markie
Status:Active» Needs review
StatusFileSize
new6.75 KB
PASSED: [[SimpleTest]]: [MySQL] 55,833 pass(es).
[ View ]

3 files modified.

thedavidmeister’s picture

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

This looks good to me.

Cottser’s picture

Status:Reviewed & tested by the community» Needs work

Thanks for working on this @markie!

I'd like to see better or more descriptive variable names for the render arrays…

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -586,10 +597,22 @@ function template_preprocess_aggregator_summary_item(&$variables) {
+  $themed = array(
+    '#theme' => 'feed_icon',
+    '#url' => $feed->url->value,
+    '#title' => t('!title feed', array('!title' => $feed->label())),
+  );
+  $variables['source_icon'] = drupal_render($themed);

I'd rather see this var called $feed_icon or $source_icon instead of $themed.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorCategoryBlock.phpundefined
@@ -68,17 +68,30 @@ protected function blockBuild() {
+      $theme = array(
+        '#theme' => 'more_link',
+        '#url' => 'aggregator/categories/' . $category->cid,
+        '#title' => t("View this category's recent news."),
+      );
+      $read_more = drupal_render($theme);
...
+        $ag_theme = array(
+          '#theme' => 'aggregator_block_item',
+          '#item' => $item,
+        );
+        $items[] = drupal_render($ag_theme);
...
+        $rendered = array(
+          '#theme' => 'item_list',
+          '#items' => $items,
+        );
         return array(
-          '#children' => theme('item_list', array('items' => $items)) . $read_more,
+          '#children' => drupal_render($rendered) . $read_more,

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.phpundefined
@@ -69,16 +69,28 @@ protected function blockBuild() {
+      $themed = array(
+        '#theme' => 'more_link',
+        '#url' => 'aggregator/sources/' . $feed->fid,
+        '#title' => t("View this feed's recent news."),
+      );
+      $read_more = drupal_render($themed);
...
+        $themed = array(
+          '#theme' => 'aggregator_block_item',
+          '#item' => $item,
+        );
+        $items[] = drupal_render($themed);
...
+        $themed = array(
+          '#theme' => 'item_list',
+          '#items' => $items,
+        );
         return array(
-          '#children' => theme('item_list', array('items' => $items)) . $read_more,
+          '#children' => drupal_render($themed) . $read_more,

thedavidmeister’s picture

We actually just updated the main issue summary:

The name of the variable for the renderable array should be the same as '#theme', (ie. $foo = array('#theme' => 'foo', ... );) if/when this would conflict with an existing variable in the current scope, $build is the alternative.

markie’s picture

Status:Needs work» Needs review
StatusFileSize
new6.88 KB
PASSED: [[SimpleTest]]: [MySQL] 56,117 pass(es).
[ View ]

Updated as requested.

eromero1’s picture

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

Tested @markie's patch and everything worked properly. When the aggregator was fed, it responded as requested. There were no apparent issues.

sbudker1’s picture

Tested @markie's patch and was successful when using the feed aggregator! Everything seemed to work normally and there were no visible problems.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -586,10 +597,22 @@ function template_preprocess_aggregator_summary_item(&$variables) {
+    $variables['source_image'] = l($image, $feed->link->value, array('html' => TRUE, 'attributes' => array('class' =>
+                                                                                                      'feed-image')));

This should just be on one line

thedavidmeister’s picture

    $image = drupal_render($image);
+    $variables['source_image'] = l($image, $feed->link->value, array('html' => TRUE, 'attributes' => array('class' =>
+                                                                                                      'feed-image')));

l() can take a renderable array for content now. no need to call drupal_render() on $image here.

adamcowboy’s picture

Assigned:Unassigned» adamcowboy

dibs!

adamcowboy’s picture

StatusFileSize
new6.73 KB
PASSED: [[SimpleTest]]: [MySQL] 58,021 pass(es).
[ View ]

I fixed it (I think).

adamcowboy’s picture

Status:Needs work» Needs review
azinoman’s picture

Assigned:adamcowboy» azinoman

dibs on review

azinoman’s picture

Status:Needs review» Reviewed & tested by the community

Aggregator is working like we expect. I added a news feed and everything was working. Nice job Adam!

jenlampton’s picture

Assigned:azinoman» Unassigned

And nice job on the review azinoman!

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 58c54c3 and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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

thedavidmeister’s picture

Status:Closed (fixed)» Needs work

theme() still exists in aggregator. See RSS.php for example.

StephaneQ’s picture

Status:Needs work» Needs review
StatusFileSize
new1.42 KB
PASSED: [[SimpleTest]]: [MySQL] 57,623 pass(es).
[ View ]

I found 2 instances left in 2 files

pplantinga’s picture

Status:Needs review» Needs work

What if instead of #markup + drupal_render() we just use #theme?

$build['pager'] = array('#theme' => 'pager');

StephaneQ’s picture

Status:Needs work» Needs review
StatusFileSize
new680 bytes
new1.37 KB
PASSED: [[SimpleTest]]: [MySQL] 57,961 pass(es).
[ View ]
pplantinga’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me!

I think it may be more appropriate to do this in #2046881: [meta] Avoid "early rendered" strings where beneficial to do so, build structured data to pass to drupal_render() once instead but at some point return drupal_render($build) will most likely need to become just return $build

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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