Issue #2151109 by joelpittet, c4rl, IshaDakota, pplantinga, gnuget, longwave, jeanfei, sbudker1: Convert theme_system_modules_details() to Twig

Task

Convert theme_system_modules_details() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing
  • Profiling not needed - admin-only change (per #25)

Steps to test

Visit the http://d8.dev/admin/modules/ page and review markup and check for visual regressions.

Files: 
CommentFileSizeAuthor
#77 convert-2151109-77.patch13.53 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 115,480 pass(es). View
#77 interdiff.txt1.3 KBjoelpittet
#71 convert-2151109-70.patch13.57 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,791 pass(es), 376 fail(s), and 162 exception(s). View
#71 interdiff.txt2.03 KBjoelpittet
#66 interdiff.txt6.83 KBjoelpittet
#66 convert-2151109-66.patch13.47 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,049 pass(es), 4 fail(s), and 0 exception(s). View
#65 interdiff.txt996 bytesCottser
#65 convert-2151109-65.patch13.36 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,073 pass(es). View
#64 interdiff.txt1.69 KBCottser
#64 convert-2151109-64.patch13.35 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,059 pass(es). View
#63 interdiff.txt2.03 KBCottser
#63 convert-2151109-63.patch13.24 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,057 pass(es). View
#62 interdiff.txt3.21 KBCottser
#62 convert-2151109-61.patch13.15 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,064 pass(es). View
#61 before-after-markup.png168.08 KBjoelpittet
#60 markup.png127.32 KBjoelpittet
#60 before-visual.png140.35 KBjoelpittet
#60 after-visual.png142.39 KBjoelpittet
#59 interdiff.txt631 bytesCottser
#59 convert-2151109-59.patch11.28 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,675 pass(es). View
#55 interdiff.txt2.95 KBCottser
#55 convert-2151109-54.patch11.15 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,563 pass(es). View
#52 interdiff.txt1.52 KBCottser
#52 convert-2151109-52.patch11.09 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,562 pass(es), 8 fail(s), and 0 exception(s). View
#50 convert-2151109-50.patch11.68 KBjcnventura
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,250 pass(es). View
#50 interdiff-48-50.txt1.63 KBjcnventura
#48 convert-2151109-48.patch10.98 KBjcnventura
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,503 pass(es). View
#48 interdiff.txt775 bytesjcnventura
#46 interdiff-2151109-39-45.txt2.13 KBakalata
#46 convert-2151109-45.patch10.98 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,485 pass(es). View
#39 interdiff-2151109-36-39.txt2.26 KBakalata
#39 convert-2151109-39.patch10.96 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,726 pass(es). View
#36 interdiff-2151109-34-36.txt816 bytesakalata
#36 convert-2151109-36.patch11.12 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,983 pass(es). View
#34 interdiff-2151109-27-34.txt1.59 KBakalata
#34 convert-2151109-34.patch11.14 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,002 pass(es). View
#32 d8-patched.png198.44 KBderheap
#32 d8-head.png189.5 KBderheap
#27 interdiff-2151109-24-27.txt1.54 KBakalata
#27 convert-2151109-27.patch11.1 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,906 pass(es). View
#25 Screen Shot 2015-09-24 at 04.19.31.png84.78 KBlauriii
#25 Screen Shot 2015-09-24 at 04.19.14.png70.43 KBlauriii
#24 interdiff.txt6.52 KBjoelpittet
#24 convert-2151109-24.patch11.92 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,931 pass(es). View
#23 interdiff.txt3.51 KBlauriii
#23 convert-2151109-23.patch11.65 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,881 pass(es). View
#22 interdiff-19-22.txt7.28 KBakalata
#22 convert_system_modules_detail-2151109-22.patch10.84 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,909 pass(es). View
#19 interdiff-2151109-17-19.txt5.51 KBakalata
#19 convert_system_modules_detail-2151109-19.patch7.47 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,882 pass(es). View
#17 interdiff-2151109-13-17.txt6.58 KBakalata
#17 convert_system_modules_detail-2151109-17.patch8.35 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,734 pass(es). View
#13 convert_system_modules_detail-2151109-13.patch7.26 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,708 pass(es). View

Comments

Cottser’s picture

Issue summary: View changes

Adding a commit message to the issue summary so the folks who already worked on #1987410: [meta] system.module - Convert theme_ functions to Twig and #1898454: system.module - Convert PHPTemplate templates to Twig (comment #41 and above aka @c4rl) get credit.

Cottser’s picture

gnuget’s picture

Assigned: Unassigned » gnuget

I'm going to work on this, this night

mfernea’s picture

Shouldn't the task be to remove theme_system_modules_details and construct the render array differently in ModulesListForm->buildForm() as per #1938930: Convert theme_system_modules_details() to #type table?

Cottser’s picture

Issue tags: +Twig conversion
joelpittet’s picture

@mfernea I'd prefer we didn't have to do this conversion but the type=>table conversion is stuck right now, so need some eyeballs over there. Maybe you could find where I went wrong with the patch in #29?

mfernea’s picture

@joelpittet: I wish I had the required knowledge :), but I don't. I'm still in the learning phase in this matter.

joelpittet’s picture

@mfernea no worries at all, there is sooo much things still that could use your eyeballs in the #1757550: [Meta] Convert core theme functions to Twig templates! And also I still find type=>table difficult to comprehend and I've done quite a few. They can be tricky, especially when they are form based and the keys need to change for the submit handler to accept the values.

mfernea’s picture

@joelpittet: Ok, I'll search there for things that I can tackle. :)

Cottser’s picture

Status: Active » Postponed

Postponed on the #type table conversion in #1938930: Convert theme_system_modules_details() to #type table then.

akalata’s picture

Status: Postponed » Active
akalata’s picture

Status: Active » Needs review
FileSize
7.26 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,708 pass(es). View
akalata’s picture

Assigned: gnuget » Unassigned
Cottser’s picture

I have a review I will post later or tomorrow. Overall looks great :) thanks @akalata!

Cottser’s picture

Status: Needs review » Needs work

Thanks again @akalata!

I think the Twig template has two blank lines at the bottom which git is complaining about:

Checking patch core/modules/system/templates/system-modules-details.html.twig...
<stdin>:202: new blank line at EOF.

Just in general I'm wondering if more of the markup in preprocess could be moved to the Twig template but there might be reasons why not, we can talk about it at DrupalCon :)

  1. +++ b/core/modules/system/system.admin.inc
    @@ -170,39 +170,49 @@ function template_preprocess_status_report(&$variables) {
    + * Prepares variables for the modules details template.
    

    Minor: I think we usually pluralize these, like…

    Prepares variables for module detail templates.

    Internal pluralization may not be quite right but hopefully you get the idea.

  2. +++ b/core/modules/system/system.admin.inc
    @@ -170,39 +170,49 @@ function template_preprocess_status_report(&$variables) {
    + *     - #requires: (optional) A list of modules that the project requires
    + *     - #required_by: (optional) A list of modules that require the project
    

    Minor: Add dots.

  3. +++ b/core/modules/system/system.admin.inc
    @@ -170,39 +170,49 @@ function template_preprocess_status_report(&$variables) {
    +    $item['checkbox'] = drupal_render($module['enable']);
    

    Not sure if it's possible but would be nice to avoid calling drupal_render() if we can.

  4. +++ b/core/modules/system/system.admin.inc
    @@ -211,21 +221,29 @@ function theme_system_modules_details($variables) {
    +    // Begin building out the description content for the module details
    

    Minor: Add a dot to the end.

  5. +++ b/core/modules/system/system.admin.inc
    @@ -211,21 +221,29 @@ function theme_system_modules_details($variables) {
    -      '#suffix' => '</span'
    +      '#suffix' => '</span>'
    

    Nice find! Minor: We can also add a trailing comma here.

  6. +++ b/core/modules/system/system.admin.inc
    @@ -211,21 +221,29 @@ function theme_system_modules_details($variables) {
    +    $description .=  t('Machine name: @machine-name', array('@machine-name' =>  $renderer->render($machine_name_render)));
    

    Minor: Extra space after double arrow/hash rocket/fat comma (=> $renderer).

  7. +++ b/core/modules/system/templates/system-modules-details.html.twig
    @@ -0,0 +1,54 @@
    + * Default theme implementation for the modules listing page. Displays a list of
    + * all packages in a project.
    

    I think the second sentence should be on its own line, with a blank line in between per https://www.drupal.org/node/1354#general.

  8. +++ b/core/modules/system/templates/system-modules-details.html.twig
    @@ -0,0 +1,54 @@
    + * - header: Table header cells.
    + *   - content: A localized string for the title of the column.
    

    The modules one is well formatted, the end of the header needs to end in a colon to set up the next list per https://www.drupal.org/node/1354#lists.

  9. +++ b/core/modules/system/templates/system-modules-details.html.twig
    @@ -0,0 +1,54 @@
    + *   - attributes: Attributes on the row
    ...
    + *   - details: Other details about the module
    

    Add dots.

  10. +++ b/core/modules/system/templates/system-modules-details.html.twig
    @@ -0,0 +1,54 @@
    +      {%
    +        set zebra = cycle(['even', 'odd'], loop.index)
    +      %}
    +      <tr{{ module.attributes.addClass(zebra) }}>
    

    Nice :)

    Potentially the set could be all on one line here.

akalata’s picture

Status: Needs work » Needs review
FileSize
8.35 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,734 pass(es). View
6.58 KB

Muchas gracias for the review @Cottser!

Updated patch addresses feedback in #16 and renames to system_module_details for less pluralization confusion.

Specific to #3, I'm not sure which is better: calling \Drupal::service('renderer')->render($stuff) multiple times, or declaring $renderer = \Drupal::service('renderer'); and calling $renderer->render($stuff) multiple times. The second pattern of usage was already present in the patch, but the first usage seems clearer to me?

Re: Removing markup from the preprocess -- most of that markup is provided via a '#type' => 'details',, I'm not sure if there's an easy way to call that formatter from within a twig template? The only other markup is the <label> element, and it's important to get that right for accessibility.

Cottser’s picture

Technically changing to system_module_details would be a BC break so not sure if that would be allowed in this stage of the release.

For point 3 I was thinking of removing the drupal_render() altogether and just render it in the Twig template but I did not look at it super closely.

Thanks!

akalata’s picture

FileSize
7.47 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,882 pass(es). View
5.51 KB

Ok, so no BC break. I'd forgotten about the "just render it in the template" piece as well - seems to work ok on my end.

joelpittet’s picture

  1. +++ b/core/modules/system/system.admin.inc
    @@ -170,39 +170,49 @@ function template_preprocess_status_report(&$variables) {
    +    'name' => $form['#header'][1]['data'],
    +    'description' => $form['#header'][1]['data'],
    

    Is description supposed to be 2 instead of index 1?

    Maybe this could be done somehow in a loop to avoid this kind of thing?

  2. +++ b/core/modules/system/system.admin.inc
    @@ -211,21 +221,29 @@ function theme_system_modules_details($variables) {
           '#prefix' => '<span dir="ltr" class="table-filter-text-source">',
    ...
    -      '#suffix' => '</span'
    +      '#suffix' => '</span>',
    

    This is a nice catch and also this would be nice in the template.

  3. +++ b/core/modules/system/system.admin.inc
    @@ -211,21 +221,29 @@ function theme_system_modules_details($variables) {
    -    $description .= '<div class="admin-requirements">' . t('Machine name: @machine-name', array('@machine-name' =>  $renderer->render($machine_name_render))) . '</div>';
    +    $description .=  t('Machine name: @machine-name', array('@machine-name' => $renderer->render($machine_name_render)));
    

    Same here.

  4. +++ b/core/modules/system/system.admin.inc
    @@ -211,21 +221,29 @@ function theme_system_modules_details($variables) {
         if ($version) {
           $description .= '<div class="admin-requirements">' . t('Version: @module-version', array('@module-version' => $renderer->render($module['version']))) . '</div>';
         }
    

    This would be awesome in the template.

lauriii’s picture

  1. +++ b/core/modules/system/system.admin.inc
    @@ -170,39 +170,49 @@ function template_preprocess_status_report(&$variables) {
    + *     - name: The name of the module.
    ...
    + *     - enable: A checkbox for enabling the module.
    

    Lets put enable after name because those two are always the only ones existing

  2. +++ b/core/modules/system/system.admin.inc
    @@ -170,39 +170,49 @@ function template_preprocess_status_report(&$variables) {
    + *     - links: (optional) Administration links provided by the module.
    

    "A list of administration links provided by the module"

  3. +++ b/core/modules/system/templates/system-modules-details.html.twig
    @@ -0,0 +1,54 @@
    \ No newline at end of file
    

    Missing newline

akalata’s picture

FileSize
10.84 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,909 pass(es). View
7.28 KB

#20.1 - yes, good catch. We could put it in a loop, but that makes it less semantic when the variables are passed to the template.

#20.2, 20.3, 20.4: I've moved these into the template, though I'm not sure that this is the correct solution because now it's building the <details> element manually, instead of using '#type' => 'details'.

#21 feedback also addressed - thanks @lauriii!

lauriii’s picture

FileSize
11.65 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,881 pass(es). View
3.51 KB
  1. Machine name was not being printed to the details
  2. Added test for machine name missing from the module details because it was missing on #22 but there was no failing tests.
  3. There was used new and old array syntaxes; changed all for the new syntax
  4. Made few code comment changes
joelpittet’s picture

FileSize
11.92 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,931 pass(es). View
6.52 KB

Got rid of all the early rendering, the new line on required by/requires was added by the new inline item_list type and has another issue to get in to fix that but this totally side steps that problem by building the inline comma list in twig.

I'm sure we can still clean this up some more, thank you very much @akalata for taking this on it's looking awesome.

Hope I didn't break it more but seems to work locally.

lauriii’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing, -Needs profiling
FileSize
70.43 KB
84.78 KB

Admin facing pages don't need profiling.

I did the manual testing:

Before:

After:

The visual changes are approved because its broken in the head. It is being fixed also in here: #2557367: Fix inline list CSS

akalata’s picture

Should we close #1938930: Convert theme_system_modules_details() to #type table in favor of this patch, and approach theme_system_modules_uninstall() in the same way?

akalata’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.1 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,906 pass(es). View
1.54 KB

I've removed the code in theme_system_modules_uninstall() that #24 added, and fixed the docs for template_preprocess_system_modules_details() (#requires and #required_by are provided starting with #).

akalata’s picture

Issue summary: View changes
lauriii’s picture

+++ b/core/modules/system/system.admin.inc
@@ -170,110 +170,63 @@ function template_preprocess_status_report(&$variables) {
+ *     - links: (optional) Administration links provided by the module.
+ *     - #requires: (optional) A list of modules that the project requires.
+ *     - #required_by: (optional) A list of modules that require the project.

At least these three are actually required. There is also #attributes variable

stefan.r’s picture

Priority: Normal » Major

Getting rid of theme functions should be major, right?

stefan.r’s picture

derheap’s picture

FileSize
189.5 KB
198.44 KB

I have made some screenshots. With the patch applied Chrome produces an extra space before the module description.
That should be removed, like in the code below.

<summary aria-controls="{{ module.id }}" role="button"><span class="text module-description">{{ module.description }}</span></summary>

HEAD:

Before

With the patch:

After

akalata’s picture

Issue tags: +Barcelona2015

@lauriii re: #29, How do we define "required" in this sense? I couldn't find anything on it in the docs. I considered those 3 items optional because not every module will have those values to pass to the template. The missing #attributes is a good catch.

@derheap: Thanks for the screenshots! I enjoyed helping you at the Barcelona sprint.

akalata’s picture

FileSize
11.14 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,002 pass(es). View
1.59 KB

This patch adds #attributes to the docblock for template_preprocess_system_modules_details() and fixes the extra space in Chrome identified in #32.

lauriii’s picture

Status: Needs review » Needs work

They are required in that sense that it will cause warning if the variable doesn't exist :)

akalata’s picture

Status: Needs work » Needs review
FileSize
11.12 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,983 pass(es). View
816 bytes

Ahh, I see. joel's changes had introduced that. :)

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as concerns after #25 has been addressed.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/system/system.admin.inc
    @@ -170,110 +170,64 @@ function template_preprocess_status_report(&$variables) {
    + *     - links: (optional) Administration links provided by the module.
    

    Links are still required.

  2. +++ b/core/modules/system/system.admin.inc
    @@ -170,110 +170,64 @@ function template_preprocess_status_report(&$variables) {
    +    if (empty($module['version']['#markup'])) {
    

    TBH we shouldn't be making this kind of assumptions..

akalata’s picture

Status: Needs work » Needs review
FileSize
10.96 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,726 pass(es). View
2.26 KB

@lauriii: Should we just not worry then about flagging things as "optional" since we're getting them from the $form anyway? Even if there's no content, they're at least empty and exist.

derheap’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed the patch and the interdiff.
It addresses #29, #38 and the extra space from #32.

@aklata & @ laurii: The template checks for requires, links etc. So no need to worry about.

So I am putting this back to RTBC

@akalata:
I enjoyed your help and support at the sprint. It very helpfull to hear, that reviews and screenshots are as important as patches.

joelpittet’s picture

RTBC++ thanks @derheap for doing final checks, the thorough review from @lauriii and the patches @akalata

catch’s picture

This conflicts with #2372183: Display same info for themes as for modules could use cross review.

LewisNyman’s picture

I would commit this one first. It shouldn't be too hard to convert the class changes in #2372183: Display same info for themes as for modules in the new twig template.

jcnventura’s picture

I'm also postponing #2574721: Provide access to action links in the modules page on small screens on this one. Not sure if the blocker tag is still used for anything useful.

Cottser’s picture

This is looking very good, just found two minor documentation points that can either be fixed on commit or in a normal follow-up. Awesome work @akalata, @lauriii, @joelpittet and all!

  1. +++ b/core/modules/system/src/Tests/Form/ModulesListFormWebTest.php
    @@ -49,6 +49,10 @@ public function testModuleListForm() {
    +    // Ensure that Testing modules machine name is printed. Testing module is
    

    Minor: Ensure that the Testing module's machine name is printed. Add "the", apostrophe for module. Needs to be re-wrapped to 80 chars also because those changes would put it over.

  2. +++ b/core/modules/system/templates/system-modules-details.html.twig
    @@ -0,0 +1,84 @@
    + *   - namet: A localized string for the title of the Name column.
    

    Minor: s/namet/name/

  3. +++ b/core/modules/system/templates/system-modules-details.html.twig
    @@ -0,0 +1,84 @@
    + *   - id: A unqiue id for interacting with the details element.
    + *   - enable_id: A unique id for interacting with the checkbox element.
    

    Minor: In docs we usually use uppercase ID I think.

akalata’s picture

FileSize
10.98 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,485 pass(es). View
2.13 KB

Re 45.3, I couldn't find anything specific in the docs, but a quick search of the codebase shows "unique id", "unique ID", and "unique identifier" are all used. I think "unique identifier" is clearest in this case, because these are not numeric or UUIDs.

Attaching a patch with doc changes to make it easier for a committer. :)

Cottser’s picture

Thanks @akalata :)

+++ b/core/modules/system/templates/system-modules-details.html.twig
@@ -8,14 +8,14 @@
+ *   - id: A unqiue identifier for interacting with the details element.

Oops there is one more here, s/unqiue/unique/.

PS. Of course in my previous comment I meant three, not two :)

jcnventura’s picture

FileSize
775 bytes
10.98 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,503 pass(es). View

Saving @akalata some time. Because Barkalona. And swimming.

And besides, this is blocking me on two other issues.

PS: priorities not in the order listed.

Cottser’s picture

+++ b/core/modules/system/system.admin.inc
@@ -170,110 +170,60 @@ function template_preprocess_status_report(&$variables) {
-    $description .= '<div class="admin-requirements">' . t('Machine name: @machine-name', array('@machine-name' =>  $renderer->render($machine_name_render))) . '</div>';
...
-      $description .= '<div class="admin-requirements">' . t('Version: @module-version', array('@module-version' => $renderer->render($module['version']))) . '</div>';
...
-      $description .= '<div class="admin-requirements">' . t('Requires: @module-list', array('@module-list' => $renderer->render($requires))) . '</div>';
...
-      $description .= '<div class="admin-requirements">' . t('Required by: @module-list', array('@module-list' => $renderer->render($required_by))) . '</div>';

+++ b/core/modules/system/templates/system-modules-details.html.twig
@@ -0,0 +1,84 @@
+                <div class="admin-requirements">{{ 'Machine name:'|t }} <span class="table-filter-text-source" dir="ltr">{{ module.machine_name }}</span></div>
...
+                  <div class="admin-requirements">{{ 'Version:'|t }} {{ module.version }}</div>
...
+                  <div class="admin-requirements">{{ 'Requires:'|t }} {{ module.requires|safe_join(', ') }}</div>
...
+                  <div class="admin-requirements">{{ 'Required by:'|t }} {{ module.required_by|safe_join(', ') }}</div>

I don't want to hold this up but have to ask: are we okay with these translatable strings losing some context? We could probably use trans tags (and maybe set tags if needed?) to keep the translatable strings the same.

jcnventura’s picture

FileSize
1.63 KB
11.68 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,250 pass(es). View

As a translator myself, yes we shouldn't touch those strings at this point. Problem is, I can't seem to make it work except for the first one (machine name).

In the case of the version, when keeping the translatable string, the version number simply refuses to show up.

In the case of the required_by, HTML gets escaped, and I can see the tags.. Same is applicable to requires, but not so easy to spot.

Is there anyway to have HTML survive a filter like t({'@var': var|safe_join(', ')}) without getting escaped?

Cottser’s picture

Status: Reviewed & tested by the community » Needs review
Cottser’s picture

FileSize
11.09 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,562 pass(es), 8 fail(s), and 0 exception(s). View
1.52 KB

Thanks @jcnventura! This should fix the version and the requires/required by needs some more thought. Some or all of this may need to use trans tags instead.

Status: Needs review » Needs work

The last submitted patch, 52: convert-2151109-52.patch, failed testing.

jcnventura’s picture

Nice one, that render filter.. I guess someone should update https://www.drupal.org/node/2357633, with that and other missing filters?

Cottser’s picture

Status: Needs work » Needs review
FileSize
11.15 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,563 pass(es). View
2.95 KB

I'm not super thrilled about this but it works and maintains the translatable strings. The alternative seems to be a lot of looping and ifs in the template which seems even uglier. Thoughts?

Cottser’s picture

@jcnventura that would be great, the changed/added filters and functions are in the \Drupal\Core\Template\TwigExtension class, getFunctions and getFilters methods.

Cottser’s picture

After some more digging and IRC discussion with @xjm and @alexpott I created #2579091: Make safe_join Twig filter return a Markup object. This can still go in and be refactored later IMO.

The last submitted patch, 52: convert-2151109-52.patch, failed testing.

Cottser’s picture

FileSize
11.28 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,675 pass(es). View
631 bytes

Like so.

joelpittet’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
142.39 KB
140.35 KB
127.32 KB

I'd prefer to not do the renderer stuff, but that can be likely added to #2579091: Make safe_join Twig filter return a Markup object

There is no visual regression to speak of:
Before:

After:

But there is markup regression that likely needs to be addressed.

joelpittet’s picture

Issue summary: View changes
FileSize
168.08 KB

Sorry had my diff backwards in #60

But here is a couple more markup regressions.

Cottser’s picture

Status: Needs work » Needs review
FileSize
13.15 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,064 pass(es). View
3.21 KB

Thanks @joelpittet there is also an empty links div, hopefully this isn't too big of a change.

There's still some more but this is all I have time for now.

Cottser’s picture

FileSize
13.24 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,057 pass(es). View
2.03 KB

Okay room for one more.

Cottser’s picture

FileSize
13.35 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,059 pass(es). View
1.69 KB

Lalala. Some of this is kinda ugly but not sure how else to do it.

Cottser’s picture

FileSize
13.36 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,073 pass(es). View
996 bytes

Sorry for all the patches, missed one thing. This should be pretty darn close.

joelpittet’s picture

Issue summary: View changes
FileSize
13.47 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,049 pass(es), 4 fail(s), and 0 exception(s). View
6.83 KB

Remove the unnessasary build up of the header text by moving it to the template and avoiding the hardcoded indexes since the classes are already there I removed the #header key all together and duplicate class build-up that isn't used. Avoids dealing with ['data'] and ['class'] keys that are for #theme table.

Get the |render out of the template as it would be nice if it weren't necessary to treat those variables as render arrays in the first place... If we can avoid treating variables as render arrays, the better (exception for maybe checking if they are empty).

Cottser’s picture

Overall looks like a good direction, thanks @joelpittet! A couple points:

  1. +++ b/core/modules/system/templates/system-modules-details.html.twig
    @@ -58,21 +48,21 @@
    -                  <div class="admin-requirements">{{ 'Machine name: @machine-name'|t({'@machine-name': module.machine_name|render}) }}</div>
    +                  <div class="admin-requirements">{{ 'Machine name: <span dir="ltr" class="table-filter-text-source">@machine-name</span>'|t({'@machine-name': module.machine_name }) }}</div>
    

    I don't think we can add this span stuff to the translatable string.

  2. +++ b/core/modules/system/templates/system-modules-details.html.twig
    @@ -58,21 +48,21 @@
    -                    {% for link in module.links %}
    -                      {{ link }}
    +                    {% for link_type in ['help', 'permissions', 'configure'] %}
    +                      {{ module.links[link_type] }}
    

    Not totally sold on this part being display logic but I could probably be convinced.

Status: Needs review » Needs work

The last submitted patch, 66: convert-2151109-66.patch, failed testing.

joelpittet’s picture

Re #67.1 I'd rather just remove the span all together, why do we need that class wrapping it and the ltr looks like a mistake. Needs git bisect to find out what is up there...

#67.2 Without that in the template any other link type will be overwritten and you'd have to get pretty creative in preprocess to get other links(if needed). This just exposes this hardcoded "which links to display" decision that to the themer.

Cottser’s picture

#67.1 is for the module filter so it needs to stay.

Fair enough on point 2 I suppose, but doesn't that also mean that if modules want to add more links they're stuck? I guess the only reason why we iterate specifically through those 3 is to show them in that order?

joelpittet’s picture

Status: Needs work » Needs review
FileSize
2.03 KB
13.57 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,791 pass(es), 376 fail(s), and 162 exception(s). View

Fixed my bad copy/paste and semi-reverted #67.1 because apparently JS tied to that span tag and I'd rather just remove it all together... JS should be a data-attribute to prevent unnessary dom traversal but that is out of scope here.

joelpittet’s picture

Status: Needs review » Needs work

Re #70

Fair enough on point 2 I suppose, but doesn't that also mean that if modules want to add more links they're stuck? I guess the only reason why we iterate specifically through those 3 is to show them in that order?

If modules want to add more links they coudn't before either. So I'm not changing this, I'm just moving it it's responsibility away from preprocess. Yeah not sure about it being an order thing but that seems to be a good guess for why... there are likely better ways to do that but out of scope here I think.

The last submitted patch, 66: convert-2151109-66.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Whoops, needs review.

Status: Needs review » Needs work

The last submitted patch, 71: convert-2151109-70.patch, failed testing.

The last submitted patch, 71: convert-2151109-70.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
13.53 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 115,480 pass(es). View

I guess I should wake up, seriously. I'm not drinking, promise! ... maybe I should start.

mdrummond’s picture

Status: Needs review » Reviewed & tested by the community

From what I can tell, looks like all remaining feedback has been addressed. Back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yay, another one bites the dust-ah!

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 02bd75b on 8.0.x
    Issue #2151109 by akalata, Cottser, joelpittet, jcnventura, lauriii,...

Status: Fixed » Closed (fixed)

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