Task

Convert the following theme functions to use the new table #type:

Module Theme function name Where in Code What is it really?
system theme_system_modules_details function form table



These 2 were removed or converted to twig in other patches.

Module Theme function name Patch
system theme_status_report #2127941: Remove two (out of 3) bogus and duplicate date languages UIs
system theme_system_date_format_localize_form #2151101: Convert theme_status_report() to Twig

Related issues

Files: 
CommentFileSizeAuthor
#195 Extend___drupal_8_0_x_and_1__d8_html__sh_.png29.89 KBjoelpittet
#194 interdiff-191-194.txt1.07 KBjmolivas
#194 convert-1938930-194.patch15.12 KBjmolivas
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,244 pass(es). View
#191 1938930-191.patch15.23 KBrpayanm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,307 pass(es). View
#187 interdiff-1938930-180-187.txt544 bytesjmolivas
#187 convert-1938930-187.patch15.15 KBjmolivas
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch convert-1938930-187.patch. Unable to apply patch. See the log in the details link for more information. View
#184 1938930-after-node.txt2.26 KBakalata
#184 1938930-before-node.txt2.49 KBakalata
#184 node-after.png37.64 KBakalata
#184 node-before.png38.17 KBakalata
#180 interdiff-1938930-179-180.txt2.74 KBjmolivas
#180 convert-1938930-180.patch15.21 KBjmolivas
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,036 pass(es). View
#179 interdiff-1938930-177-179.txt6.65 KBjmolivas
#179 convert-1938930-179.patch12.73 KBjmolivas
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,054 pass(es). View
#177 convert-1938930-177.patch12.4 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,143 pass(es). View
#172 interdiff.txt3.57 KBjoelpittet
#172 convert_system_theme-1938930-172.patch22.67 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,912 pass(es), 3 fail(s), and 0 exception(s). View
#171 convert_system_theme-1938930-171.patch23.07 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,913 pass(es), 3 fail(s), and 176 exception(s). View
#162 1938930-162.patch23.68 KBrpayanm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,682 pass(es). View

Comments

duellj’s picture

Status: Active » Needs review
FileSize
10.27 KB
PASSED: [[SimpleTest]]: [MySQL] 54,575 pass(es). View

Initial pass at converting system theme tables. This doesn't include theme_status_report, since that table requires column attributes, which aren't supported by table form #types.

duellj’s picture

Since theme_status_report isn't a form, it doesn't need to be converted to a table #type.

duellj’s picture

#1: 1938930-1-system-tables.patch queued for re-testing.

c4rl’s picture

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.

c4rl’s picture

Issue summary: View changes

Removes theme_status_report

jibran’s picture

Status: Needs review » Needs work
Issue tags: +Novice

As per #4

I've added theme_system_modules_details to the list.
duellj’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
9.71 KB
20.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1938930-6-system-table.patch. Unable to apply patch. See the log in the details link for more information. View

Converted theme_system_modules_details(). Moved most of the logic to _system_modules_build_row(). Also rerolled initial patch to apply to HEAD again.

Status: Needs review » Needs work

The last submitted patch, 1938930-6-system-table.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, 1938930-6-system-table.patch, failed testing.

andypost’s picture

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

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

andypost’s picture

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

andypost’s picture

andypost’s picture

Issue summary: View changes

Adding theme_system_modules_details

jibran’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, 1938930-6-system-table.patch, failed testing.

ramlev’s picture

Assigned: duellj » ramlev

Im on it

ramlev’s picture

Assigned: ramlev » Unassigned
Hydra’s picture

Assigned: Unassigned » Hydra

Working on this

Hydra’s picture

Status: Needs work » Needs review
FileSize
28.18 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

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

Hydra’s picture

FileSize
28.18 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

Ignore first patch.

Status: Needs review » Needs work

The last submitted patch, 1938930-17-system-table.patch, failed testing.

joelpittet’s picture

Assigned: Hydra » Unassigned
Status: Needs work » Needs review
FileSize
28.3 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

Re-roll of #19

Status: Needs review » Needs work

The last submitted patch, 1938930-21-type-table-system-reroll.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
28.1 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

Fix the @todo for id.

Status: Needs review » Needs work

The last submitted patch, 1938930-22-type-table-system.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.79 KB
27.77 KB
FAILED: [[SimpleTest]]: [MySQL] 59,383 pass(es), 19 fail(s), and 0 exception(s). View

another go.

Status: Needs review » Needs work

The last submitted patch, 1938930-24-type-table-system.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.47 KB
26 KB
FAILED: [[SimpleTest]]: [MySQL] 59,437 pass(es), 5 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 1938930-27-type-table-system.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
55.5 KB
9.02 KB
14.93 KB
PASSED: [[SimpleTest]]: [MySQL] 57,977 pass(es). View

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?

inject fail

Status: Needs review » Needs work

The last submitted patch, 29: 1938930-29-type-table-system.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
Cottser’s picture

Issue summary: View changes

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

joelpittet’s picture

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

martin107’s picture

Assigned: Unassigned » martin107

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

martin107’s picture

FileSize
20.72 KB
FAILED: [[SimpleTest]]: [MySQL] 63,650 pass(es), 607 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 35: 1938930-35-type-table-system.patch, failed testing.

joelpittet’s picture

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
14.92 KB
FAILED: [[SimpleTest]]: [MySQL] 64,194 pass(es), 1 fail(s), and 0 exception(s). View

Here's the re-roll of #29

joelpittet’s picture

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.

martin107’s picture

FileSize
20.84 KB
PASSED: [[SimpleTest]]: [MySQL] 64,218 pass(es). View

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 )

The last submitted patch, 38: 1938930-type-table-system-29-reroll.patch, failed testing.

martin107’s picture

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.

martin107’s picture

FileSize
15.61 KB
PASSED: [[SimpleTest]]: [MySQL] 64,319 pass(es). View

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

martin107’s picture

FileSize
17.55 KB
PASSED: [[SimpleTest]]: [MySQL] 64,288 pass(es). View
2.71 KB

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

joelpittet’s picture

Yay interdiff and green, thanks @martin107

martin107’s picture

Issue summary: View changes
    // 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.

joelpittet’s picture

sorry, cross post issue.

joelpittet’s picture

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

joelpittet’s picture

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

joelpittet’s picture

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.

martin107’s picture

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

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

I will start posting fixes soon

joelpittet’s picture

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

joelpittet’s picture

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

martin107’s picture

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

joelpittet’s picture

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

martin107’s picture

FileSize
17.77 KB
PASSED: [[SimpleTest]]: [MySQL] 64,723 pass(es). View

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

martin107’s picture

FileSize
18.19 KB
PASSED: [[SimpleTest]]: [MySQL] 64,729 pass(es). View
3.56 KB

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

martin107’s picture

FileSize
1.71 KB
18.41 KB
PASSED: [[SimpleTest]]: [MySQL] 64,740 pass(es). View

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

martin107’s picture

FileSize
18.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,725 pass(es). View
692 bytes

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.

martin107’s picture

FileSize
15.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,677 pass(es). View

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

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

martin107’s picture

FileSize
15.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,306 pass(es), 528 fail(s), and 0 exception(s). View
3 KB

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.

Status: Needs review » Needs work

The last submitted patch, 61: 1938930-61-type-table-system.patch, failed testing.

The last submitted patch, 61: 1938930-61-type-table-system.patch, failed testing.

joelpittet’s picture

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

martin107’s picture

FileSize
15.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,694 pass(es). View
2.36 KB

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.

martin107’s picture

Status: Needs work » Needs review
joelpittet’s picture

After testbot comes back green I call dibs;)

martin107’s picture

Assigned: martin107 » Unassigned

@joelpittet Ok thank for all the prompt advice.

martin107’s picture

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',
           ),
 
joelpittet’s picture

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

martin107’s picture

FileSize
15.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,689 pass(es). View
1 KB

#69 + #70 enough said

joelpittet’s picture

Status: Needs review » Needs work

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.

martin107’s picture

Status: Needs work » Needs review
FileSize
15.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,086 pass(es). View
4.24 KB

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 ?

martin107’s picture

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.

joelpittet’s picture

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

martin107’s picture

FileSize
15.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,101 pass(es). View
713 bytes

wrapper's delight!

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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

martin107’s picture

Status: Needs work » Needs review
FileSize
15.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,296 pass(es). View

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

Sutharsan’s picture

Issue tags: -Needs reroll

Removing 'Needs reroll' tag.

joelpittet’s picture

Status: Needs review » Needs work
FileSize
10.58 KB
  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?

martin107’s picture

Status: Needs work » Needs review
FileSize
15.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,253 pass(es). View
788 bytes

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?

Status: Needs review » Needs work

The last submitted patch, 81: 1938930-80-type-table-system.patch, failed testing.

martin107’s picture

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

martin107’s picture

The last submitted patch, 81: 1938930-80-type-table-system.patch, failed testing.

joelpittet’s picture

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?

joelpittet’s picture

martin107’s picture

Status: Needs work » Needs review
FileSize
22.84 KB
15.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,245 pass(es). View
2.47 KB

#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

akalata’s picture

Assigned: Unassigned » akalata
Issue tags: +TCDrupal 2014, +Twig, +needs re-roll
akalata’s picture

FileSize
14.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,671 pass(es), 673 fail(s), and 11,616 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 90: 1938930-table-type-system-reroll-90.patch, failed testing.

akalata’s picture

FileSize
14.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,620 pass(es), 652 fail(s), and 222 exception(s). View

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

akalata’s picture

Assigned: akalata » Unassigned
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 92: 1938930-system-tables-92.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 92: 1938930-system-tables-92.patch, failed testing.

martin107’s picture

FileSize
928 bytes

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

tim.plunkett’s picture

Issue tags: -needs re-roll +Needs reroll
amitgoyal’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
14.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,808 pass(es), 652 fail(s), and 222 exception(s). View
2.76 KB

Reroll of #92.

Status: Needs review » Needs work

The last submitted patch, 98: 1938930-system-tables-98.patch, failed testing.

joelpittet’s picture

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

amitgoyal’s picture

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

joelpittet’s picture

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

ChandeepKhosa’s picture

Assigned: Unassigned » ChandeepKhosa

assigning to self

ChandeepKhosa’s picture

Assigned: ChandeepKhosa » Unassigned
akalata’s picture

FileSize
15.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,958 pass(es), 934 fail(s), and 52,835 exception(s). View
2.77 KB

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.

akalata’s picture

Status: Needs work » Needs review
joelpittet’s picture

Issue tags: +Needs manual testing

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.

akalata’s picture

FileSize
18.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,466 pass(es), 4 fail(s), and 0 exception(s). View
5.09 KB

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.

The last submitted patch, 105: 1938930-system-tables-105.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 108: 1938930-convert-system-tables-108.patch, failed testing.

akalata’s picture

FileSize
438.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1938930-convert-system-tables-111.patch. Unable to apply patch. See the log in the details link for more information. View
398.33 KB

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(

akalata’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 111: 1938930-convert-system-tables-111.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
18.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1938930-convert-system-tables-114.patch. Unable to apply patch. See the log in the details link for more information. View
1.25 KB

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

mgifford’s picture

Status: Needs review » Needs work

The last submitted patch, 114: 1938930-convert-system-tables-114.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
18.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,041 pass(es). View

Posting reroll.

mgifford’s picture

Issue tags: +accessibility

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>
mgifford’s picture

Status: Needs review » Needs work
akalata’s picture

Status: Needs work » Needs review
FileSize
19.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,055 pass(es). View
1.59 KB

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

mgifford’s picture

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>
akalata’s picture

FileSize
19.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,031 pass(es). View
3.53 KB

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.

joelpittet’s picture

  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.

akalata’s picture

FileSize
19.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,608 pass(es), 8 fail(s), and 41,596 exception(s). View
2.49 KB

Changes based on @joelpittet's feedback.

Status: Needs review » Needs work

The last submitted patch, 125: 1938930-convert-system-tables-125.patch, failed testing.

akalata’s picture

FileSize
19.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,025 pass(es). View
1.06 KB

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’s picture

Status: Needs work » Needs review
idebr’s picture

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

akalata’s picture

FileSize
19.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,270 pass(es), 674 fail(s), and 512 exception(s). View
620 bytes

Thanks @idebr!

Rerolling and switching to Html::getClass.

Status: Needs review » Needs work

The last submitted patch, 130: 1938930-convert-system-tables-130.patch, failed testing.

akalata’s picture

Issue tags: +Needs reroll

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

prajaankit’s picture

Issue tags: +SprintWeekend2015
FileSize
13.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,468 pass(es), 679 fail(s), and 168 exception(s). View
prajaankit’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 133: 1938930-convert-system-tables-131.patch, failed testing.

lokapujya’s picture

Assigned: Unassigned » lokapujya
Status: Needs work » Needs review
Issue tags: -TCDrupal 2014, -Needs reroll, -SprintWeekend2015
FileSize
19.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,070 pass(es), 1 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 136: 1938930-136.patch, failed testing.

lokapujya’s picture

Assigned: lokapujya » Unassigned
Status: Needs work » Needs review
FileSize
2.51 KB
21.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,086 pass(es), 1 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 138: 1938930-137.patch, failed testing.

lokapujya’s picture

FileSize
21.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,089 pass(es). View
895 bytes

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

This validation_reasons piece and its test could use some improvement.

lokapujya’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 140: 1938930-140.patch, failed testing.

joelpittet queued 140: 1938930-140.patch for re-testing.

The last submitted patch, 140: 1938930-140.patch, failed testing.

joelpittet queued 140: 1938930-140.patch for re-testing.

Cottser’s picture

Status: Needs work » Needs review

Yay green! Nice work @lokapujya.

lokapujya’s picture

  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()?

mgifford’s picture

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.

table header with check all

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?

duplication for validation reasons

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

idebr’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs manual testing
FileSize
203.19 KB

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:

lokapujya’s picture

Assigned: Unassigned » lokapujya
lokapujya’s picture

Assigned: lokapujya » Unassigned
Status: Needs work » Needs review
FileSize
23.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1938930-151.patch. Unable to apply patch. See the log in the details link for more information. View
3.35 KB

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

mgifford queued 151: 1938930-151.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 151: 1938930-151.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
23.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1938930-154.patch. Unable to apply patch. See the log in the details link for more information. View

Reroll.

mgifford queued 154: 1938930-154.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 154: 1938930-154.patch, failed testing.

mgifford’s picture

Issue tags: +Needs reroll
rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
23.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,599 pass(es). View
akalata’s picture

Issue tags: +Needs reroll
akalata’s picture

Status: Needs review » Needs work
rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
17.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,410 pass(es). View
rpayanm’s picture

FileSize
23.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,682 pass(es). View

Ignore the #161 patch.

mgifford’s picture

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.

marcingy’s picture

I agree patch looks good

lokapujya’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

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

lauriii queued 162: 1938930-162.patch for re-testing.

lauriii queued 162: 1938930-162.patch for re-testing.

lauriii’s picture

Status: Needs review » Needs work
  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

a_thakur’s picture

Assigned: Unassigned » a_thakur
akalata’s picture

Assigned: a_thakur » Unassigned
joelpittet’s picture

Status: Needs work » Needs review
FileSize
23.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,913 pass(es), 3 fail(s), and 176 exception(s). View

Re-rolled and removed the test change that remove the translation inside the assertText() method.

joelpittet’s picture

FileSize
22.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,912 pass(es), 3 fail(s), and 0 exception(s). View
3.57 KB

Fixes form #168 and a fix from the re-roll.

The last submitted patch, 171: convert_system_theme-1938930-171.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 172: convert_system_theme-1938930-172.patch, failed testing.

joelpittet’s picture

Because we have such a big hunk being removed I guess we have to review changes that are happening within it on those re-rolls.

I'm thinking splitting this up into two issues. One for instsall and one for uninstall page. Then it's easier to review.

joelpittet’s picture

Title: Convert system theme tables to table #type » Convert theme_system_modules_details() tables to table #type
Issue summary: View changes

Ok splitting the uninstall theme function off to it's own issue here #2507243: Convert theme_system_modules_uninstall() tables to table #type

joelpittet’s picture

Status: Needs work » Needs review
FileSize
12.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,143 pass(es). View

Here's the split.

jmolivas’s picture

Assigned: Unassigned » jmolivas

@joelpittet: I will try changing some code here and test again

jmolivas’s picture

FileSize
12.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,054 pass(es). View
6.65 KB

joelpittet:

- I fixed some syntax here only on new code.

- On the next patch I will inject the renderer service, to remove the deprecated drupal_render function introduced on previous patch at line 458.

jmolivas’s picture

FileSize
15.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,036 pass(es). View
2.74 KB

Injecting the renderer service, to ModulesListForm class to remove the deprecated drupal_render function introduced on previous patch at #177.

joelpittet’s picture

Issue tags: +Needs manual testing

Awesome thanks @jmolivas. Tagging with needs manual testing.

See if the markup is as close to identical as possible and that we didn't break anything.

jmolivas’s picture

Assigned: jmolivas » Unassigned

Not sure against what compare the markup.

joelpittet’s picture

I could be wrong but I think this is just the modules page and it would be before and after the patch. Turning twig.debug to true can help see if the twig template is being rendered and drush cr to make sure.

akalata’s picture

Status: Needs review » Needs work
FileSize
38.17 KB
37.64 KB
2.49 KB
2.26 KB
  1. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -312,14 +324,14 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
    +      '#wrapper_attributes' => ['class' => ['checkbox']],
    

    This is adding a 'checkbox' class that does not exist in HEAD.

  2. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -413,6 +425,66 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
    +      '#template' => '<label id="module-{{ machine_name }}" for="{{ label_id }}" class="module-name table-filter-text-source">{{ module_name }}</label>',
    

    The <label> that is being specified here is not rendering, causing not only visual regression but the inability to click the label to operate the checkbox..

  3. +++ b/core/modules/system/system.admin.inc
    @@ -197,87 +197,6 @@ function template_preprocess_status_report(&$variables) {
    -    $description .= '<div class="admin-requirements">' . t('Machine name: !machine-name', array('!machine-name' => '<span dir="ltr" class="table-filter-text-source">' . $key . '</span>')) . '</div>';
    

    This span is missing (though I'm not sure it's necessary?)

  4. +++ b/core/modules/system/system.admin.inc
    @@ -197,87 +197,6 @@ function template_preprocess_status_report(&$variables) {
    -    foreach (array('help', 'permissions', 'configure') as $link_type) {
    -      $links .= drupal_render($module['links'][$link_type]);
    -    }
    

    There are IDs and data-drupal-selectors present on these links in HEAD that are missing here.

  5. There is a drupal-data-selector on each <tr> that has changed (from edit-modules-core-node to edit-modules-core-table-node, though I couldn't figure out where in the patch this happened.
  6. This may be a new bug (unrelated to this patch since issue appears in HEAD), but I could not see any module's Permissions link with the other two (Help and Configure). I briefly searched for an existing issue and did not find one, but I wanted to check in with somebody more familiar with this page to confirm.
akalata’s picture

jmolivas’s picture

Assigned: Unassigned » jmolivas

Working on @akalata recommendations at #185

jmolivas’s picture

FileSize
15.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch convert-1938930-187.patch. Unable to apply patch. See the log in the details link for more information. View
544 bytes

This is adding a 'checkbox' class that does not exist in HEAD.

Fixing #184.1 Removing checkbox class

lauriii’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 187: convert-1938930-187.patch, failed testing.

lauriii’s picture

Issue tags: +Needs reroll
rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
15.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,307 pass(es). View

Please review.

jmolivas’s picture

Assigned: jmolivas » Unassigned
jmolivas’s picture

Related to #184.2

The that is being specified here is not rendering, causing not only visual regression but the inability to click the label to operate the checkbox.

Seems like inline_template is not rendering the label tag

    [
      '#type'   => 'inline_template',
      '#template' => '<label id="module-{{ machine_name }}" for="{{ label_id }}" class="module-name table-filter-text-source">{{ module_name }}</label>',
      '#context' => [
        'machine_name' =>  $module->getName(),
        'label_id' => $id,
        'module_name' => $module->info['name'],
      ],
      '#wrapper_attributes' => ['class' => ['module']],
    ];

I also tried to use markup instead, with no luck

    [
      '#type'   => 'markup',
      '#markup' => sprintf(
        '<label id="module-%s" for="%s" class="module-name table-filter-text-source">%s</label>',
        $module->getName(),
        $id,
        $module->info['name']
      ),
      '#wrapper_attributes' => ['class' => ['module']],
    ];

Same output label tag is removed

jmolivas’s picture

FileSize
15.12 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,244 pass(es). View
1.07 KB

Updating inline_template with label to render label element:

    // Add the module label and expand/collapse functionality.
    $row['name'] = [
      '#type' => 'label',
      '#id' => 'module-'.$module->getName(),
      '#title' => $module->info['name'],
      '#for' => $id,
      '#attributes' => [
        'class' => ['module-name', 'table-filter-text-source'],
      ],
      '#wrapper_attributes' => ['class' => ['module']]
    ];
joelpittet’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
29.89 KB

Extra id and data selector on table and missing checkbox class on cell.

Before:
<table class="responsive-enabled" data-striping="1">

After:
<table data-drupal-selector="edit-modules-core-table" id="edit-modules-core-table" class="responsive-enabled" data-striping="1">

Before

<tr data-drupal-selector="edit-modules-core-action" class="odd">
<td class="checkbox"><div class="form-item js-form-type-checkbox form-type-checkbox form-item-modules-core-action-enable form-no-label">

After

<tr data-drupal-selector="edit-modules-core-table-action" class="odd">
<td><div class="form-item js-form-type-checkbox form-type-checkbox form-item-modules-core-action-enable form-no-label">
         

lauriii’s picture

Title: Convert theme_system_modules_details() tables to table #type » Convert theme_system_modules_details() and theme_system_modules_uninstall() tables to table #type
Issue tags: +blocker
Related issues: +#2544156: Deprecate drupal_render_children(), +#2280965: [meta] Remove every SafeMarkup::set() call

Cottser has closed #2151113: Convert theme_system_modules_uninstall() to Twig as a duplicate for this. This is also blocker for #2544156: Deprecate drupal_render_children() which is children of critical meta #2280965: [meta] Remove every SafeMarkup::set() call

joelpittet’s picture

Not a duplicate, I split them up because they were getting bugs in one and not the other. Easier to review and test separate because they are two different pages

Check out the child issue

joelpittet’s picture

Needs a reroll since the safemarkup went in. Also title revert @lauriii?

Cottser’s picture

Title: Convert theme_system_modules_details() and theme_system_modules_uninstall() tables to table #type » Convert theme_system_modules_details() to #type table

Just a heads up: #2568935: Convert theme_system_modules_details() to a template

If that gets momentum we could postpone this.

akalata’s picture

Same as #200, since that one was marked as a dupe: #2151109: Convert theme_system_modules_details() to Twig

akalata’s picture

Status: Needs work » Closed (duplicate)

Closing as duplicate of #2151109: Convert theme_system_modules_details() to Twig since that issue has had some good work on it.