Problem/Motivation

See #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only

theme_system_modules_details() and template_preprocess_system_themes_page() use !placeholder.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 2568609-2.patch3.25 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

alexpott’s picture

Title: Replace remaining !placeholder for Non-URL HTML outputs only in theme_system_modules_details() » Replace remaining !placeholder for Non-URL HTML outputs only in theme functions in system.admin.inc
Issue summary: View changes
Status: Active » Needs review
Parent issue: #2568607: Replace remaining !placeholder for Non-URL HTML outputs only in Theme name stuff in SystemController » #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only
FileSize
3.25 KB

I might have gone a little bit further than necessary but if we're going to touch the lines like '!module-list' => implode(', ', $module['#required_by']) I think we should swap to using the new comma list item list since this is semantically correct and rendering produces a safe string.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/modules/system/system.admin.inc
    @@ -216,15 +216,31 @@ function theme_system_modules_details($variables) {
    +    $renderer = \Drupal::service('renderer');
    +    $machine_name_render = [
    

    Opened up #2568797: Add \Drupal::renderer() but document how and when it should be used after seeing that and agreeing that for now its the besser choice.

  2. +++ b/core/modules/system/system.admin.inc
    @@ -216,15 +216,31 @@ function theme_system_modules_details($variables) {
    +      '#prefix' => '<span dir="ltr" class="table-filter-text-source">',
    +      '#plain_text' => $key,
    +      '#suffix' => '</span'
    +    ];
    +    $description .= '<div class="admin-requirements">' . t('Machine name: @machine-name', array('@machine-name' =>  $renderer->render($machine_name_render))) . '</div>';
    

    On the longrun you would better put that entire theme function into a template right? In there it would also be MUCH faster!

  • catch committed d30d576 on 8.0.x
    Issue #2568609 by alexpott: Replace remaining !placeholder for Non-URL...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

xjm’s picture

On the longrun you would better put that entire theme function into a template right? In there it would also be MUCH faster!

That's kinda what I was hoping we'd do in this issue... followup?

Also, I would really have expected manual testing with screenshots for this change. @alexpott says he tested it manually in Stark and Bartik; no screenshots available though.

dawehner’s picture

Status: Fixed » Closed (fixed)

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