Convert theme_system_modules_details() to #type table

Is it applicable to include theme_system_modules_details? Seems like this could be refactored to be a single table as well.

UPDATE: I've added theme_system_modules_details to the list.

Removes theme_status_report

As per #4

I've added theme_system_modules_details to the list.

#6: 1938930-6-system-table.patch queued for re-testing.

@duellj suppose better to convert some of callbacks to use render arrays!

The related issue #2009674: Replace theme() with drupal_render() in system module

For theme_system_report() should be filed separate issue
Also related #2010982: Replace theme() with drupal_render() in system module for system_status()

Filed #2013258: Simplify render for theme_status_report()

Adding theme_system_modules_details

#6: 1938930-6-system-table.patch queued for re-testing.

Im on it

Working on this

Rerolled the patch

Some notes:

1. +++ b/core/modules/system/system.admin.inc
   @@ -1281,11 +1332,24 @@ function system_modules_uninstall($form, $form_state = NULL) {
   +      if (!empty($required_modules)) {
   +        $disabled_message = format_plural(count($required_modules),
   +          'To uninstall @module, the following module must be uninstalled first: @required_modules',
   +          'To uninstall @module, the following modules must be uninstalled first: @required_modules',
   +          array('@module' => $module_name, '@required_modules' => implode(', ', $required_modules)));
   +        $disabled_message = '<div class="admin-requirements">' . $disabled_message . '</div>';
   +      }
   +      else {
   +        $disabled_message = '';
   +      }
   +      $form['uninstall'][$module->name]['uninstall']['#markup'] = t($disabled_message);
   

   Unfortunately this is not injected to the right place. I remember there was an issue about that, can't find it right now. This problem still belongs in the patch, so the order should be taken care of at some point.

2. +++ b/core/modules/system/system.admin.inc
   @@ -2151,34 +2109,31 @@ function system_date_format_localize_form_submit($form, &$form_state) {
    /**
   - * Returns HTML for a locale date format form.
   + * Form constructor for the reset date format form.
     *
   - * @param $variables
   - *   An associative array containing:
   - *   - form: A render element representing the form.
   + * @param $langcode
   + *   Language code, e.g. 'en'.
     *
   - * @ingroup themeable
   + * @see locale_menu()
   + * @see system_date_format_localize_reset_form_submit()
   + * @ingroup forms
     */
   -function theme_system_date_format_localize_form($variables) {
   -  $form = $variables['form'];
   -  $header = array(
   -    'machine_name' => t('Machine Name'),
   -    'pattern' => t('Format'),
   -  );
   +function system_date_format_localize_reset_form($form, &$form_state, $langcode) {
   +  $form['langcode'] = array('#type' => 'value', '#value' => $langcode);
   +  return confirm_form($form,
   +    t('Are you sure you want to reset the date formats for %language to the global defaults?', array('%language' => language_load($langcode)->name)),
   +    'admin/config/regional/date-time/locale',
   +    t('Resetting will remove all localized date formats for this language. This action cannot be undone.'),
   +    t('Reset'), t('Cancel'));
   +}
    
   -  foreach (element_children($form['date_formats']) as $key) {
   -    $row = array();
   -    $row[] = $form['date_formats'][$key]['#title'];
   -    unset($form['date_formats'][$key]['#title']);
   -    $row[] = array('data' => drupal_render($form['date_formats'][$key]));
   -    $rows[] = $row;
   -  }
   -
   ...
   -  $output .= theme('table', array('header' => $header, 'rows' => $rows));
   -  $output .= drupal_render_children($form);
   -
   -  return $output;
   +/**
   + * Form submission handler for locale_date_format_reset_form().
   

   Removed this, thats a nice feature but I don't think that has to do something with the conversion

Ignore first patch.

Re-roll of #19

Fix the @todo for id.

another go.

and another, these errors will get beaten down no matter how tricky type=>table is. *Read masochist*

I believe this is blocked by #1876718: Allow modules to inject columns into tables more easily

field_system_info_alter is trying to inject another column on the modules table and that is at least part of why this conversion is failing.

This patch may pass BUT may cause others... See screenshot. Maybe someone has a work around for the missing header column?

29: 1938930-29-type-table-system.patch queued for re-testing.

theme_system_date_format_localize_form() was removed in #2127941: Remove two (out of 3) bogus and duplicate date languages UIs.

@sun maybe you can point me in the right direction for column injection?

I just spent time looking at https://drupal.org/node/2151113 ( recently closed )
and will I understand the issues involved I try and merge to efforts into one patch.

Simple steps first ... the patch on this issue no longer applies... so I am about to reroll

1) I have resolved the insertion of extra column in the 'admin/moduiles' page

The problem was with the ModulesListForm.php buildRow function and variable $row['links']
It was being using to compute row['description] and was left in the render array and so appeared twice in the render array
To avoid confusion this $row['links'] was moved close to where it was used and renamed to be just $links

2) have merged in code from the closed duplicate issue - now /modules/uninstall has been converted to #type table

I am expecting this to blow up because the WebTests are expecting a particular type of html which has changed... but when bot come back I will investigate in the morning

@martin107 can you do a re-roll then post your changes and interdiff in a separate patch? That way it's easier for others to see what you did at a glance.

I feel like my patch was so close except that column injection bit and passes the tests.

Anyways thank you for giving this a try! I appreciate any help I can get here.

Here's the re-roll of #29

Can you try to apply your interdiff to #38? And maybe not do the variable renaming if you can help it? $linkEntry doesn't really help. Try to do the least amount of changes, it will be more evident where things are failing.

I have the answer as to why so many exceptions

The Tests that fail are all of the form

$edit['uninstall[' . $module . ']'] = TRUE;
$this->drupalPostForm('admin/modules/uninstall', $edit, t('Uninstall'));

that is they require html element to have a specific name, for example the color module

<input name="uninstall[color]" type="checkbox" id="edit-modules-color-uninstall-checkbox" value="1" class="form-checkbox">

and the failing patch changes the "name" property... so instead of updating many webtests, in this next patch I am
overriding the checkbox attributes :-

      $form['modules'][$module->name]['uninstall']['checkbox'] = array(
        '#type' => 'checkbox',
        '#parent' => '$module->name',
        '#title' => $this->t('Uninstall @module module', array('@module' => $module_name)),
        '#title_display' => 'invisible',
        '#attributes' => array('name' => "uninstall[$module->name]")
      );

This is messy and should not be done in core.. but I am just proving my theory

I welcome advice on a cleaner way forward... lets see what tetsbot says

Sorry about the interdiff issue .. I will revert to the minimum change and try a better name as part of the next patch.
Unfortunately that local data structure /variable I called $linkEntry should not be included in the returned $row ( from the function buildRow )

This grep identifies all the places where the tests are tightly coupled to the form structure.

 grep  -R '$this->drupalPostForm(.admin/modules/uninstall' *

core/modules/comment/lib/Drupal/comment/Tests/CommentFieldsTest.php: $this->drupalPostForm('admin/modules/uninstall', $edit, t('Uninstall'));
core/modules/locale/lib/Drupal/locale/Tests/LocaleConfigTranslationTest.php: $this->drupalPostForm('admin/modules/uninstall', array('uninstall[image]' => "image"), t('Uninstall'));
core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.php: $this->drupalPostForm('admin/modules/uninstall', $edit, t('Uninstall'));
core/modules/node/lib/Drupal/node/Tests/NodeTypePersistenceTest.php: $this->drupalPostForm('admin/modules/uninstall', $edit, t('Uninstall'));
core/modules/system/lib/Drupal/system/Tests/Module/DependencyTest.php: $this->drupalPostForm('admin/modules/uninstall', $edit, t('Uninstall'));
core/modules/system/lib/Drupal/system/Tests/Module/DependencyTest.php: $this->drupalPostForm('admin/modules/uninstall', $edit, t('Uninstall'));
core/modules/system/lib/Drupal/system/Tests/Module/InstallUninstallTest.php: $this->drupalPostForm('admin/modules/uninstall', $edit, t('Uninstall'));
core/modules/system/lib/Drupal/system/Tests/System/MainContentFallbackTest.php: $this->drupalPostForm('admin/modules/uninstall', $edit, t('Uninstall'));
core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarAdminMenuTest.php: $this->drupalPostForm('admin/modules/uninstall', $edit, t('Uninstall'));

I am going to abandon the attribute bodge ... and just update the tests... preferably with a function call that abstract the changes to $edit into one place.

Returning to comment #38 which provided a reroll of #29

Unfortunately this patch no longer applies, so working from there and just applying minimal bug fixes.
The only file I had to touch was ModulesUninstallForm.php ( so no interdiff )

This works without any of the hackery of the WebTests.
I have a simple fix for the admin/modules layout issues but will apply if this come back green

So this patch is the minimal changes that solve the issue on the '/module/install' page, where the table appears shifted to the right as identified in #29

see ModulesListForm.php

the row returned from buildRow() had a stray key which should have remained as a local variable

$row['links'] becomes $rwoLinks

For once I can supply an interdiff.txt :)

Yay interdiff and green, thanks @martin107

    // Add the description, along with any modules it requires.
    $description = '';
    if ($module->info['version'] || $row['#requires'] || $row['#required_by']) {
      $description .= ' <div class="requirements">';
      if ($module->info['version']) {
        $description .= '<div class="admin-requirements">' . t('Version: !module-version', array('!module-version' => $module->info['version'])) . '</div>';
      }
      if ($row['#requires']) {
        $description .= '<div class="admin-requirements">' . t('Requires: !module-list', array('!module-list' => implode(', ', $row['#requires']))) . '</div>';
      }
      if ($row['#required_by']) {
        $description .= '<div class="admin-requirements">' . t('Required by: !module-list', array('!module-list' => implode(', ', $row['#required_by']))) . '</div>';
      }
      $description .= '</div>';
    }

    $links = '';
    foreach (array('help', 'permissions', 'configure') as $key) {
      $links .= (isset($rowLinks[$key]) ? drupal_render($rowLinks[$key]) : '');
    }

    if ($links) {
      $description .= '  <div class="links">';
      $description .= $links;
      $description .= '</div>';
    }

In terms of review, this could be better, concatenating string $links and $description is old school. I will fix if community thinks it is worth it.

sorry, cross post issue.

Awesome work @martin107! This is looking in some cases better and more features than before the patch. See my screenshots for a review of the display and markup that changed. Any questions please ask. This is very very close to RTBC:)

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallForm.php
@@ -87,35 +87,60 @@ public function buildForm(array $form, array &$form_state) {
+      $form['uninstall'][$module->name]['description']['uninstall']['#markup'] = t($disabled_message);

Can you use the injected t method? $this->t()?

For the concatenation, if you can remove the drupal_render() calls too I'd say go for it!

See what @sun did for the label stuff here: https://drupal.org/node/1938926#comment-8434777 for reference if that helps. May not be a good idea to duplicate but something to keep in mind. The label was removed from the hidden one and using a new #type=>label to associate it with the checkboxes.

Compiling a todo list generate from #48 and #49 just so none of the issues slip throughout the cracks

page '/modules/install'

A) Reintroduce id in the link contained in the descriptions pull down

You likely already know but if you want a class on the cell use ['#wrapper_attributes'] => array('class' => array('description'));

Checking in, @martin107 are you still on these changes in #51 or should we unassign?

Ah became snowed under with other things ... will find time to write a summary of what I know tomorrow

Good to see you're still around! People drop in and out so fast sometimes I just wanted to ping and see if someone else can tackle in your absence. Though you're doing a great job so rather see you plow this through:)

Patch no longer applies ... reroll ( so I can supply interdiff in the next comment )

This is a trivial patch ... it replaces $t() with $this->t() see comment #49. and deals with issue A.

The list now looks :-

page '/modules/install'

A) Reintroduce id in the link contained in the descriptions pull down see example "edit-modules-core-color-links-help"

B) Extra class checkbox class inserted to wrapping the auto-inserted table column containing the checkbox

C) Reintroduce the classes descriptions and expand

page 'modules/uninstall'

D) Bold tags missing from column 'name'

E) label tags missing from column 'name'

F) column description has missing class 'description'

G) column 'name' text has become mangled "Uninstall text Editor Module" has become "Update Text Editor"

H) checkbox value property was '1' now string ( e.g. "editor" )

Fixed C and F

list now reads

page '/modules/install'

A) Reintroduce id in the link contained in the descriptions pull down see example "edit-modules-core-color-links-help"

B) Extra class checkbox class inserted to wrapping the auto-inserted table column containing the checkbox

C) Reintroduce the classes descriptions and expand

page 'modules/uninstall'

D) Strong tags missing from column 'name'

E) label tags missing from column 'name'

F) column description has missing class 'description'

G) column 'name' text has become mangled "Uninstall text Editor Module" has become "Update Text Editor"

H) checkbox value property was '1' now string ( e.g. "editor" )

Here is a questionable fix for issue B .. or at least an explanation for where the bug is inserted.

in ModuleListForm.php here is the interdiff

$row['enable'] += array(
- '#wrapper_attributes' => array(
- 'class' => array('checkbox'),
- ),
'#id' => $id,
'#parents' => array('modules', $module->info['package'], $module->name, 'enable'),
);

[ Note the overriding of the checkboxes place in the render array using #parents.]

So the #wrapper_attributes exposes a bug deeper within drupal8...

The #wrapper_attributes is erroneously causing the class 'checkbox' to be added to its direct parent ( a div ) and its parents parent
( ie the 'td' element ) only the td element is required....

So the patch removes both the wanted and the stray class, making a one-to-one comparison between "patch" and "before patch" impossible.

The effect of making td.checkbox is to apply text-align:center; to an element without text..
This change removes a non-functional class attribute which I suggest is acceptable.

Reroll simple changes due to "detail" changes from the following issue.

https://drupal.org/node/1892182 "#type details: Rename #collapsed to #open"

fixed issues E,D,G
replaced another instance of $t() with $this->t() for better dependency injection.

removed some non-functional lines of code such as "<<< HEAD" which came in from a flawed reroll ( #60 )
and #tableselect => TRUE in the wrong place.

Regarding the label, check out the recently committed patch for simpletest #type table. @sun made it more accessible with #type=>label instead of those hidden labels. #1938926: Convert simpletest theme tables to table #type

Joelpittet's suggestion results in much simpler PHP code and much more standard HTML.

A consequence of this is that it abandons many of the tasks in #58 for example :-
(E) Replicating the existing non standard double implementation of "label" elements

In this light I have also removed the "strong" tags in column 2 as they make no sense.

One striking difference between 'before patch' and 'post patch' [ /admin/modules/uninstall ] is the loss of the "Uninstall" title from the checkbox column.
Using the new solution. and inspecting form.inc ( form_process_table() ) I see no way of restoring this!

Visually post patch looks better to me... but I will defer to consensus from the community on this point.

Manually installing and uninstalling 'book' now works so I expect the testbot to come back green.

After testbot comes back green I call dibs;)

@joelpittet Ok thank for all the prompt advice.

Running admin/modules/uninstall through the html validator at validator.w3.org results in the following errors

<th specifier="name" \>
and
<th specifier="description"\>

attribute specifier not allowed at this point!!!!

Can I suggest trivial removal of the lines from ModulesUninstallForm.php

      '#header' => array(
        'title' => array(
          'data' => $this->t('Name'),
-          'specifier' => 'name',
        ),
        'description' => array(
           'data' => $this->t('Description'),
-           'specifier' => 'description',
           ),
 

@martin107 of course you can remove those, they weren't in there before, the table wasn't sortable before. We can try to keep the improvements/features to a minimum unless they make the patch easier too.

#69 + #70 enough said

I'm a bit tired but tried to give you a few things still to tackle. Mostly coding standards, which you can compare against here: https://drupal.org/coding-standards

1. +++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.php
   @@ -185,35 +188,39 @@ public function buildForm(array $form, array &$form_state) {
   -    $row['links']['help'] = array();
   +    $rowLinks['help'] = array();
   

   We don't camelcase these variable names. I think I know why you changed the name but do you need to? Could you explain. If you do need to just change it to $row_links.

2. +++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.php
   @@ -185,35 +188,39 @@ public function buildForm(array $form, array &$form_state) {
   +              'id' => 'edit-modules-core-'.$module->name.'-links-help',
   +              'class' =>  array(
   ...
   +                'module-link-help'),
   +                'title' => $this->t('Help')
   +            )
   +          ),
   +          );
   

   once too many indents, missing ending commas on array last item. spaces around the concatenation period, only one space after the =>, and the ending parenthesis can go on the next line down.

   Just coding standards stuff.

3. +++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.php
   @@ -247,7 +254,6 @@ protected function buildRow(array $modules, $module, $distribution) {
        // Present a checkbox for installing and indicating the status of a module.
        $row['enable'] = array(
          '#type' => 'checkbox',
   -      '#title' => $this->t('Install'),
   

   Just add '#wrapper_attributes' => array('class' => array('checkbox')),

   And do we need to remove the #title?

4. +++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallForm.php
   @@ -87,31 +87,57 @@ public function buildForm(array $form, array &$form_state) {
   +        ),
   +      '#empty' => $this->t('There are no items yet. <a href="@add-url">Disable a module</a>', array(
   +        '@add-url' => url('admin/modules'),
   +      )),
   

   Just more coding standards with the parenthesis indentation and the array in the t() should be inline without the comma.

5. +++ b/core/modules/system/system.admin.inc
   @@ -414,61 +414,6 @@ function theme_system_modules_incompatible($variables) {
   -    '#empty' => t('No modules are available to uninstall.'),
   

   This #empty string got dropped by accident.

1.

Could you explain ? .. my first explanation was in #35

explanation:
This odd change is the fix for #29 the strange indentation issue.
It should be reinterpreted as - Why is an extra table column definitions being inserted into the render array.

Looking at buildRow()
$row[‘links’] its only valid use is to define $links, which is used to assemble $row[‘description’].
Leaving it as a key in the output variable $row is what is bleeding this spurious data into the
final render array.
end of explanation

Anyway $rowLinks has become $row_links

2. Fixed

3.

Am I interpreting you correctly... Are you saying there is a better way to add the checkbox class

      $table = array(
        '#type' => 'table',
        '#header' => array(
-        array('data' => '<span class="visually-hidden">' . $this->t('Installed') . '</span>', 'class' => array('checkbox')),
          array('data' => $this->t('Name'), 'class' => array('name')),
          array('data' => $this->t('Description'), 'class' => array('description', RESPONSIVE_PRIORITY_LOW)),
        ),
      ) + $form['modules'][$package];

and replace with

    // Present a checkbox for installing and indicating the status of a module.
    $row['enable'] = array(
      '#type' => 'checkbox',
+   '#title' => '<span class="visually-hidden">' . $this->t('Installed') . '</span>',
+   '#wrapper_attributes' => array('class' => array('checkbox')),
      '#default_value' => (bool) $module->status,
      '#disabled' => (bool) $module->status,
    );

That makes no sense to me as it would destroy the indentation... the first "th" element needs a checkbox class before it will behave
and adding #title here will add a fake install title to every row! not the column itself.

Maybe the bigger issue discovered here is that #wrapper_attributes here will not add the appropriate class to the "th" element.

Or maybe did you not see the other change I made to include a #wrapper_attribute at your suggestion?

5.

'#empty' => t('No modules are available to uninstall.'),

has not been dropped. Its function is performed by the #empty below

   $form['uninstall'] = array(
      '#type' => 'table',
      '#header' => array(
        'title' => array('data' => $this->t('Name')),
        'description' => array('data' => $this->t('Description')),
      ),
      '#empty' => $this->t('There are no items yet. <a href="@add-url">Disable a module</a>', array('@add-url' => url('admin/modules'))),
      '#tableselect' => TRUE,
      '#js_select' => FALSE,
    );

has it just been mangled into a form you object to ?

Just rereading #72 point 3

And do we need to remove the #title?

I now see that now as a nudge to sort out the column title

So from #65
One striking difference between 'before patch' and 'post patch' [ /admin/modules/uninstall ] is the loss of the "Uninstall" title from the checkbox column.
Using the new solution. and inspecting form.inc ( form_process_table() ) I see no way of restoring this!

I still have no idea how best to proceed.

1) don't worry about it, I understand enough and thanks for fixing naming.
3) Hold off on the #title add back in, i'm actually not sure what it's used for yet. But the wrapper_attributes for checkbox class on the td makes the column center it's contents. The checkbox class is for the TD, the TH has it already and it's empty so doesn't matter. @see screenshot

5) The wording changed that's why it looks dropped, and there is no disabling of modules in D8. But thanks for pointing where it went to. I just saw it removed, and searched the diff for part of the string and came up #empty (pun intended);)

wrapper's delight!

Looks like this needs a re-roll already. Thanks for the wrapper @martin107:)
http://www.youtube.com/watch?v=tUqvPJ3cbUQ

HOTEL-MOTEL...Holiday Inn. If your patch starts acting up then you take her friend.

Removing 'Needs reroll' tag.

1. When there is nothing to install this message is not there any more.

2. +++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.php
   @@ -185,35 +188,39 @@ public function buildForm(array $form, array &$form_state) {
   +              'class' => array(
   +                'module-link',
   +                'module-link-help'),
   

   This can be one line or the last one needs a command a linebreak after -help'

3. +++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.php
   @@ -185,35 +188,39 @@ public function buildForm(array $form, array &$form_state) {
   +            )
   

   This should have a comma at the end.

4. +++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallForm.php
   @@ -87,31 +87,55 @@ public function buildForm(array $form, array &$form_state) {
   +      '#js_select' => FALSE,
   

   I noticed that awesome check all feature you added to the uninstall in #44 is gone:(
   Maybe this removed that feature?

1. Perhaps I am confused, as I only see this as a repeat of the question, asked, answered and acknowledged in #72.5, #73.5 and #75.5 respectively.

2. Fixed.

3. Lines 185 to 188 don't look like that - nor can I find anything like it in the files. It is not clear what you mean.

4. Toggling '#js_select' certainly has the effect you describe. Looking at the interdiff it was not added in #44.

It comes from a period before I started looking at this issue. It was taken, by me out as part of the process of simplifying the table to identify what was going
on with the crazy indentation problems. It no longer causes indentation problems I suspect because of #76.

A - I question the usefulness of such as destructive checkbox? ... I read its purpose as delete everything that does not depend on other modules in the system and cannot see when it would be a benefit to select it.

B - Is that a feature request and out of scope of this issue?

failing test :-
A) Does not appear to be related to the page altered
B) It only a cosmetic change.

Broken HEAD or random tesbot fail... retesting

81: 1938930-80-type-table-system.patch queued for re-testing.

Regarding #81.1: My screenshot is after uninstalling all modules. That message does not exist with your patch. Have a go at it and see for yourself. That screenshot will show an empty screen with a title and no table or message.

#81.2 thank you:)

#81.3:

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.php
@@ -185,35 +188,37 @@ public function buildForm(array $form, array &$form_state) {
+          '#options' => array(
+            'attributes' => array(
+              'id' => 'edit-modules-core-' . $module->name . '-links-help',
+              'class' => array('module-link', 'module-link-help'),
+              'title' => $this->t('Help'),
+            ) <------ Need comma here.
+          ),

Maybe I need more context lines to see where.

#81.4: it's a nice feature, it is out of scope of this issue but you are removing the words "uninstall" in that header anyways. I can't see a downside to this feature at all, but maybe you can and it's only one less line of code to have it work. Though if you don't feel comfortable putting that in I can open up a follow-up for it and we can add the column header text back in?

81: 1938930-80-type-table-system.patch queued for re-testing.

#81.3 fixed.

So uninstalling all modules.. When I uninstall with all modules bad things happen..
Firstly php bums out after exceeding 30s and it destroys the site... So I cannot
repeat what you report ...

However hacking the code and inserting the following unset code in an attempt to repeat what you report was unsettling

    // Get a list of disabled, installed modules.
    $modules = system_rebuild_module_data();
    $uninstallable = array_filter($modules, function ($module) use ($modules) {
      return empty($modules[$module->name]->info['required']) && drupal_get_installed_schema_version($module->name) > SCHEMA_UNINSTALLED;
    });
    
+    unset($uninstallable);
    
    $form['uninstall'] = array();

A) I have removed the reference to the now defunct 'disable' in the empty text.
B) The fix was to move down the logic associated the comment.

// Only build the rest of the form if there are any modules available to
// uninstall;

Anyway please repeat whatever you were doing and confirm the #empty function now functions

Reasons for re-roll:
PSR-4 file locations #2083547: PSR-4: Putting it all together
module->getName #2210197: Remove public access to Extension::$type, ::$name, ::$filename
format_plural replaced by translation system #2149195: Replace format_plural with \Drupal::translation()->formatPlural()

Updating patch to remove forgotten "profile" reference; Will still need work with getting tests to pass.

Just providing an interdiff, as I follow along. Sometimes just seeing it is better than a description.

Reroll of #92.

+++ b/core/modules/system/src/Form/ModulesUninstallForm.php
@@ -106,34 +106,45 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-      $name = $module->info['name'] ?: $module->getName();
...
+      $form['uninstall'][$module->getName()]['title']['#title'] = $module->info['name'] ?: $module->getName();

This a quick drive by but the $name variable shouldn't be removed as it's used further down. There is likely a bunch of things that need checking but the least we need to change the better for re-rolls and reviewers.

@joelpittet - I believe $name variable has been removed in #92.

I think $name variable can be removed from this line as it's been set in line #128 before getting used in line #130.

$name = isset($modules[$dependent]->info['name']) ? $modules[$dependent]->info['name'] : $dependent;

+++ b/core/modules/system/src/Form/ModulesUninstallForm.php
@@ -106,34 +106,45 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+          $required_modules[] = $name;

This is where I saw it getting used again further down in the same loop but nested one more loop deep.

assigning to self

I really botched my reroll in #90, so it's no wonder the tests blew up so badly. :)

@joelpittet, removing the earlier $name is fine, in the loop you've identified, $name is defined two lines before it's added to the $required_modules array.

Thank you it's been a while and you're likely right. We can see how the testbot takes this and if it passes I'll do a manual test.

One thing I saw that was a bit puzzling in dreditor:

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -237,31 +240,33 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    $row['#requires'] = isset($row['#requires']) ? $row['#requires'] : FALSE;
+    $row['#required_by'] = isset($row['#required_by']) ? $row['#required_by'] : FALSE;

Maybe I'm missing something or will this always be false? $row doesn't seem to be defined before those two lines in buildRow() so I'd have to assume that.

Was reviewing the full issue to see if I missed anything else in my reroll. Noticed that the removal of theme_system_modules_details() in #6 got left behind at some point, so I've re-removed that, and fixed a few more instances of the $module->getName() update in ModulesListForm.

I've also noticed HTML tags showing up on admin/modules, so we know buildRow() needs another pass at any rate.

Set $row['#requires'] and $row['#required_by'] to empty arrays based on @joelpittet's feedback.

Pass $description through t() to render rather than print HTML within the description -- I'm not sure this is the right thing to do, but it is what's currently being done in core; for instance $row['#requires'][$dependency] = $this->t('@module (<span class="admin-missing">incompatible with</span> version @version)', array(

Not sure what I did to generate the last patch, manually checked this one to be sure. Compared to 108, same notes as 111.

Posting reroll.

This patch removes the semantic relationship between the checkbox & label. Now this could be resolved (as we discussed earlier today #1938920: Convert node_search_admin theme tables to table #type) but ultimately the checkboxes at a minimum need a relationship to a label (old school html or ARIA).

It looks fine visually, but looking at /admin/modules it is easy to see the changes to the semantics.

Sample diff is:

359,360c361,362
<                       <td class="module"><label id="module-help" for="edit-modules-core-help-enable" class="module-name table-filter-text-source">Help</label></td>
<                       <td class="description expand priority-low"><details id="edit-modules-core-help-enable-description" class="form-wrapper"><summary role="button" aria-controls="edit-modules-core-help-enable-description"><span class="text"> Manages the display of online help.</span></summary><div class="details-wrapper"><div class="details-description"><div class="requirements"><div class="admin-requirements">Machine name: help</div><div class="admin-requirements">Version: 8.0.0-dev</div></div>  <div class="links"><a href="http://drupal8.dev/admin/help/help" class="module-link module-link-help" title="Help" id="edit-modules-core-help-links-help">Help</a></div></div></div>
---
>                       <td class="module">Help</td>
>                       <td class="description expand priority-low"><details id="edit-modules-core-help-enable-description" aria-describedby="edit-modules-core-table-help-description--description" class="form-wrapper collapse-processed"><summary role="button" aria-controls="edit-modules-core-help-enable-description"><a href="#edit-modules-core-help-enable-description" class="details-title"><span class="details-summary-prefix visually-hidden">Show</span> <span class="text"> Manages the display of online help.</span></a><span class="summary"></span></summary><div class="details-wrapper"><div class="details-description"> <div class="requirements"><div class="admin-requirements">Version: 8.0.0-dev</div></div>  <div class="links"><a href="http://drupal8.dev/admin/help/help" id="edit-modules-core-help-links-help" class="module-link module-link-help" title="Help">Help</a></div></div></div>

Here's an update adding the <label for=""> syntax.

Better for sure. There are some inconsistencies in the second TD too. I'm not worried about the classes, maybe they matter, but it looks fine to me.

• I'm not sure where the empty <span class="summary"></span> got introduced.
• This looks like a basically redundant div <div class="requirements"><div class="admin-requirements">
• I like the aria-describedby="edit-modules-web-services-table-basic-auth-description--description" but what is it pointing to? I couldn't find an id that it referenced.

<                       <td class="module"><label id="module-basic-auth" for="edit-modules-web-services-basic-auth-enable" class="module-name table-filter-text-source">HTTP Basic Authentication</label></td>
<                       <td class="description expand priority-low"><details id="edit-modules-web-services-basic-auth-enable-description" class="form-wrapper"><summary role="button" aria-controls="edit-modules-web-services-basic-auth-enable-description"><span class="text"> Provides the HTTP Basic authentication provider</span></summary><div class="details-wrapper"><div class="details-description"><div class="requirements"><div class="admin-requirements">Machine name: basic_auth</div><div class="admin-requirements">Version: 8.0.0-dev</div></div></div></div>
---
>                       <td class="module"><label id="module-basic_auth" for="edit-modules-web-services-basic-auth-enable" class="module-name table-filter-text-source">HTTP Basic Authentication</label></td>
>                       <td class="description expand priority-low"><details id="edit-modules-web-services-basic-auth-enable-description" aria-describedby="edit-modules-web-services-table-basic-auth-description--description" class="form-wrapper collapse-processed"><summary role="button" aria-controls="edit-modules-web-services-basic-auth-enable-description"><a href="#edit-modules-web-services-basic-auth-enable-description" class="details-title"><span class="details-summary-prefix visually-hidden">Show</span> <span class="text"> Provides the HTTP Basic authentication provider</span></a><span class="summary"></span></summary><div class="details-wrapper"><div class="details-description"> <div class="requirements"><div class="admin-requirements">Version: 8.0.0-dev</div></div></div></div>

Minor changes included:

• changed some array()s to short-form []
• adjusted some more t()s to $this->t
• added back in the "Machine name" item that got lost
• fixed <span class="summary">, which was coming from how $description being added to the $row.

I think the <div class="requirements"><div class="admin-requirements"> isn't redundant, because "requirements" can wrap multiple "admin-requirements".

The aria-describedby is being added by doBuildForm() in FormBuilder. Agree it doesn't make sense, not yet sure how to fix.

1. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -98,7 +98,22 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    if(!empty($uninstallable)) {
   

   Coding standards needs space between if and parenthesis. if( to if (

2. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -98,7 +98,22 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#header' => array(
   +        'title' => array('data' => $this->t('Name')),
   +        'description' => array('data' => $this->t('Description')),
   +      ),
   

   Could probably short tag this too.

3. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -410,6 +416,63 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +    $id = Html::getClass('edit-modules-' . $module->info['package'] . '-' . $module->getName() . '-enable');
   

   getClass is a strange call for an $id.

4. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -410,6 +416,63 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +    $description .= ' <div class="requirements">';
   

   Remove space from starting of string.

5. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -106,34 +121,40 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        }
   ...
   +        }
   

   Indent needs to be unindented.

Changes based on @joelpittet's feedback.

Converting getClass to cleanCssIdentifier resulted in mixed-case ids, where testing assumes lowercase. Wrapped the cleanCssIdentifier in Unicode::strtolower, not sure if there's a better solution?

@akalata that use case has been added in Html:getClass. This should get you the same string:

$id = Html::getClass('edit-modules-' . $module->info['package'] . '-' . $module->getName() . '-enable');

More info on Html:getClass: https://api.drupal.org/api/drupal/core!lib!Drupal!Component!Utility!Html...

Thanks @idebr!

Rerolling and switching to Html::getClass.

Not sure what happened here, and I'm not in the right headspace to reroll this right now.

Done for a while if anyone wants to try fixing the remaining bug.

Here is a fix. $module vs. $module->getName(), the case is different.

This validation_reasons piece and its test could use some improvement.

Yay green! Nice work @lokapujya.

1. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -118,41 +134,53 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          'The following reasons prevents @module from being uninstalled: @reasons',
   

   should this be "prevent"?

2. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -118,41 +134,53 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        $disabled_message .= '<div class="validation_reasons">' . $disabled_message . '</div>';
   

   Could someone look at the markup being output for the "validate reasons" messages? It was a tough reroll, and I just made it work but didn't test it.

3. +++ b/core/modules/system/src/Tests/Module/UninstallTest.php
   @@ -54,7 +54,7 @@ function testUninstallPage() {
   +    $this->assertText('The following reason prevents node from being uninstalled: There is content for the entity type: Content', 'Content prevents uninstalling node module.');
   

   should we really have all the text in the assert()?

The check all box at the top of the table is part of this now, right? "Select all rows in this table". It seems to work fine, although I should mark a follow-up accessibility issue with it. There's no table header any more, would be nice if there was an invisible one for screen readers.

1. Agreed that it should be "The following reason prevents @module" & "The following reasons prevent @module". I've found examples as it is now, but think that it would be better.

2. There's a duplication with the validation. Is the class validation_reasons supposed to come with some style?

3. I'm not sure if there is a standard for assertText() length.

I did a manual test to check for any regressions. I found two regressions on the 'Uninstall' list for modules:

1. The wording has changed in the description of a module that cannot be uninstalled from 'The following reason prevents Block from being uninstalled:' to 'To uninstall block, the following module must be uninstalled first'. This description was changed recently in #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference
2. Styling from .admin-requirements has been lost for some modules

Here is an annotated screenshot:

#148.1 Done.
#148.2 Fixed the duplication. Added a style. Changed underscore to hyphen. Looks like the class was added in this issue.

#149.1 Not seeing that.

Reroll.

Ignore the #161 patch.

This patch still applies. I assume the best place to test it is still /admin/modules

In my quick review this patch looks good. Glad to see that the table headers are there again.

I agree patch looks good

Updated IS to show which theme functions we removed. Not sure what else is needed in IS.

1. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -424,6 +430,62 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +      '#parents' => ['modules', $module->info['package'], $module->getName(), 'enable'],
   

   This should be separated on multiple lines because its more than 80 characters long

2. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -424,6 +430,62 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +      $description .= '  <div class="links">';
   

   Is the whitespace on the beginning necessary?

3. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -110,7 +110,23 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form['uninstall'] = array(
   

   We could use the new array syntax here

4. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -118,41 +134,53 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      $form['uninstall'][$module->getName()]['description'] = array(
   

   We could use the new array syntax here