Updated: Comment #7

Problem/Motivation

theme_aggregator_block_item() only outputs a link

Proposed resolution

According to #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays replace it with #type => 'link'

Original report by mrded

theme_aggregator_block_item() use HTML for generation items:

/**
 * Returns HTML for an individual feed item for display in the block.
 *
 * @param $variables
 *   An associative array containing:
 *   - item: The item to be displayed.
 *   - feed: Not used.
 *
 * @ingroup themeable
 */
function theme_aggregator_block_item($variables) {
  // Display the external link to the item.
  return '<a href="' . check_url($variables['item']->link) . '">' . check_plain($variables['item']->title) . "</a>\n";
}

Is there any reason not to use l() function here?

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because parent issue is task
Prioritized changes Switching this untested function to a #type=>link reduces fragility
Disruption Although it removes a theme function i really doubt anyone is using it outside of core. I would be surprised, really
CommentFileSizeAuthor
#15 drupal-aggregator_block_item_type_link-1763964-15.patch3.52 KBParisLiakos
#13 drupal-aggregator_block_item_type_link-1763964-13.patch2.93 KBParisLiakos
#12 drupal-aggregator_block_item_type_link-1763964-12.patch3 KBParisLiakos
#11 interdiff.txt773 bytesParisLiakos
#11 drupal-aggregator_block_item_type_link-1763964-11.patch2.68 KBParisLiakos
#9 drupal-aggregator_block_item_type_link-1763964-9.patch2.68 KBParisLiakos
#2 drupal-change-theme_aggregator_block_item-1763964.patch899 bytesmrded
#1 drupal-change-theme_aggregator_block_item-1763964.patch970 bytesmrded

Comments

mrded’s picture

mrded commented
StatusFileSize
new drupal-change-theme_aggregator_block_item-1763964.patch970 bytes

Please, check my patch.

mrded’s picture

mrded commented
StatusFileSize
new drupal-change-theme_aggregator_block_item-1763964.patch899 bytes

Just deleted check_plain() & check_url() because l() is use it already.

mrded’s picture

mrded commented

2: drupal-change-theme_aggregator_block_item-1763964.patch queued for re-testing.

ParisLiakos’s picture

ParisLiakos commented

shouldnt we remove this theme function completely according to #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays?

ParisLiakos’s picture

ParisLiakos commented

double post

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
Version: 7.x-dev » 8.x-dev
Priority: Normal » Minor
Issue summary: View changes
Status: Needs review » Needs work

As per the backport policy (https://drupal.org/node/767608) this needs to be fixed in Drupal 8 first. It would be a slightly different fix, as the theme function has been replaced with aggregator-block-item.html.twig, but the fundamental issue remains. However I agree with #4 that a better fix would be to remove this template altogether.

Even if this is fixed in 8.x, I do not think a similar fix would be accepted for 7.x, because it is an api change for the sake of nicer code (it will break any code that is overriding this theme function).

ParisLiakos’s picture

ParisLiakos commented
Category: Bug report » Task
Issue summary: View changes
Parent issue: » #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays
ParisLiakos’s picture

ParisLiakos commented
Title: Use l() for theme_aggregator_block_item() » Use #type => link for theme_aggregator_block_item()
ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review
StatusFileSize
new drupal-aggregator_block_item_type_link-1763964-9.patch2.68 KB

patch

Status: Needs review » Needs work

The last submitted patch, 9: drupal-aggregator_block_item_type_link-1763964-9.patch, failed testing.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review
StatusFileSize
new drupal-aggregator_block_item_type_link-1763964-11.patch2.68 KB
new interdiff.txt773 bytes

meh

ParisLiakos’s picture

ParisLiakos commented
Priority: Minor » Normal
StatusFileSize
new drupal-aggregator_block_item_type_link-1763964-12.patch3 KB

reroll

ParisLiakos’s picture

ParisLiakos commented
StatusFileSize
new drupal-aggregator_block_item_type_link-1763964-13.patch2.93 KB

reroll

marvil07’s picture

marvil07 commented
Status: Needs review » Needs work

Patch should now also remove core/themes/classy/templates/aggregator/aggregator-block-item.html.twig

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review
StatusFileSize
new drupal-aggregator_block_item_type_link-1763964-15.patch3.52 KB

good point! also rerolled

marvil07’s picture

marvil07 commented
Status: Needs review » Reviewed & tested by the community

Thanks for the reroll. Now it seems ok.

ParisLiakos’s picture

ParisLiakos commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Let's get a CR to announce the removal. Makes sense to me - cna be committed under the reducing fragility provision.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

Sure, thanks! added https://www.drupal.org/node/2428705

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 61e09de and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 61e09de on 8.0.x 
    Issue #1763964 by ParisLiakos, mrded: Use #type => link for...

Status: Fixed » Closed (fixed)

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