Convert theme_system_modules_details() to Twig

I'm going to work on this, this night

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?

@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?

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

@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.

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

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.

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.

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.

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

#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!

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

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.

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

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?

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 #).

+++ 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

Getting rid of theme functions should be major, right?

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:

With the patch:

@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.

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

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

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

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

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..

@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.

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.

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

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

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.

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.

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. :)

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.

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?

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

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.

Sorry had my diff backwards in #60

But here is a couple more markup regressions.

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).

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.

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.

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.

Whoops, needs review.

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

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

Yay, another one bites the dust-ah!

Committed and pushed to 8.0.x. Thanks!