Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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
| 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 |
Comments
Comment #1
mrded CreditAttribution: mrded commentedPlease, check my patch.
Comment #2
mrded CreditAttribution: mrded commentedJust deleted check_plain() & check_url() because l() is use it already.
Comment #3
mrded CreditAttribution: mrded commented2: drupal-change-theme_aggregator_block_item-1763964.patch queued for re-testing.
Comment #4
ParisLiakos CreditAttribution: ParisLiakos commentedshouldnt 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?
Comment #5
ParisLiakos CreditAttribution: ParisLiakos commenteddouble post
Comment #6
ianthomas_ukAs 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).
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commentedComment #8
ParisLiakos CreditAttribution: ParisLiakos commentedComment #9
ParisLiakos CreditAttribution: ParisLiakos commentedpatch
Comment #11
ParisLiakos CreditAttribution: ParisLiakos commentedmeh
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commentedreroll
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commentedreroll
Comment #14
marvil07 CreditAttribution: marvil07 commentedPatch should now also remove core/themes/classy/templates/aggregator/aggregator-block-item.html.twig
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedgood point! also rerolled
Comment #16
marvil07 CreditAttribution: marvil07 commentedThanks for the reroll. Now it seems ok.
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedComment #18
alexpottLet's get a CR to announce the removal. Makes sense to me - cna be committed under the reducing fragility provision.
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedSure, thanks! added https://www.drupal.org/node/2428705
Comment #20
alexpottCommitted 61e09de and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.