Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thedavidmeister’s picture

Issue tags: +Novice
markie’s picture

Assigned: Unassigned » markie
Status: Active » Needs review
FileSize
6.75 KB

3 files modified.

thedavidmeister’s picture

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

This looks good to me.

star-szr’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
FileSize
6.88 KB

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

FileSize
6.73 KB

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
FileSize
1.42 KB

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
FileSize
680 bytes
1.37 KB
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.