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

Task

Convert theme_system_modules_uninstall() to a Twig template.

Remaining tasks

Steps to reproduce/review

  1. Install Drupal 8.
  2. Visit admin/modules/uninstall.
  3. Confirm that the list of modules available to uninstall appears correctly in HEAD and with the patch applied.
  4. Select a module to delete and submit the form using the Uninstall button.
  5. Ensure that the "Confirm uninstall" page is correctly displayed in HEAD and with the patch applied, and that the module is actually disabled once the uninstall is confirmed.
CommentFileSizeAuthor
#60 2151113-data-attributes.png13.01 KBstar-szr
#60 2151113-grammar.png39.04 KBstar-szr
#59 convert-2151113-59.patch8.96 KBjoelpittet
#59 interdiff.txt775 bytesjoelpittet
#58 interdiff-2151113-47-58.txt1.87 KBRainbowArray
#58 2151113-58-convert-theme-system-modules-uninstall.patch8.93 KBRainbowArray
#56 2151113-56-empty-message.png29.5 KBstar-szr
#47 interdiff.txt4.56 KBjoelpittet
#47 convert-2151113-47.patch8.9 KBjoelpittet
#45 Screen Shot 2015-10-06 at 12.41.19 AM.png365.93 KBRainbowArray
#44 convert-2151113-44.patch9.1 KBjoelpittet
#44 interdiff.txt2.9 KBjoelpittet
#43 interdiff-2151113-41-43.txt3.09 KBRainbowArray
#43 2151113-43-convert-theme-system-modules-uninstall.patch9.19 KBRainbowArray
#41 interdiff-2151113-39-41.txt1.35 KBakalata
#41 2151113-41.patch8.26 KBakalata
#39 interdiff-2151113-34-39.txt6.01 KBakalata
#39 2151113-39-modules-uninstall-twig.patch8.67 KBakalata
#36 interdiff-2151113-34-36.txt180.35 KBakalata
#36 2151113-36-modules-uninstall-twig.patch207.69 KBakalata
#34 interdiff-2151113-33-34.txt1.93 KBakalata
#34 2151113-34-modules-uninstall-twig.patch7.69 KBakalata
#33 interdiff-2151131-31-33.txt5.05 KBakalata
#33 2151113-33-modules-uninstall-twig.patch7.78 KBakalata
#31 2151113-31-modules-uninstall-twig.patch7.77 KBmr.baileys
#22 interdiff-14-22.txt2.92 KBmartin107
#22 system-convert-to-twig-2151113-22.patch7.72 KBmartin107
#15 system-convert-to-twig-2151113-14.patch7.57 KBmfernea
#7 description some pixel too deep.PNG48.72 KBFelkerL
#5 system-convert-to-twig-2151113-5.patch5.72 KBmfernea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Issue summary: View changes

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

star-szr’s picture

mfernea’s picture

I'm looking into this.

mfernea’s picture

Assigned: Unassigned » mfernea
mfernea’s picture

Assigned: mfernea » Unassigned
Status: Active » Needs review
FileSize
5.72 KB

Please provide feedback as this our first Twig related patch. :)

FelkerL’s picture

Issue tags: +SprintWeekend 2014, +D8MA

1

FelkerL’s picture

sorry, forgot the screenshot ;)

mfernea’s picture

@FelkerL: Is the screenshot related to this issue? :)

FelkerL’s picture

@mfernea: sorry, I failed :( it is the wrong screenshot and the wrong issue!

Status: Needs review » Needs work

The last submitted patch, 5: system-convert-to-twig-2151113-5.patch, failed testing.

star-szr’s picture

YesCT’s picture

Issue tags: -SprintWeekend 2014 +SprintWeekend2014

fixing tag. sorry for noise.

mfernea’s picture

I think I was on the wrong path as most probably the correct solution should be to use table #type as per #1938930: Convert theme_system_modules_details() to #type table. Please adivse.

mfernea’s picture

I'm working on patch to remove theme_system_modules_uninstall and use the #type table when building the form.

mfernea’s picture

Status: Needs work » Needs review
FileSize
7.57 KB

I wrote a patch for this so now we are using #type table.

The main modifications are:

  1. remove system_modules_uninstall theme and theme_system_modules_uninstall function
  2. move the logic from theme_system_modules_uninstall function to ModulesUninstallForm->buildForm
  3. reduce the data added to the form array in ModulesUninstallForm->buildForm
  4. as a consequence of the above I had to modify validateForm and submitForm

Few questions:

  1. shouldn't we go for tableselect? The answer is no, after seeing #1876714: Remove #type 'tableselect'
  2. how we can add the values in "Name" column as labels for the checkboxes (as it was before)?
  3. should we care about align="center" for the tds with checkboxes and class="description" of the tds with descriptions?

Status: Needs review » Needs work

The last submitted patch, 15: system-convert-to-twig-2151113-14.patch, failed testing.

star-szr’s picture

Issue tags: +Twig conversion
martin107’s picture

Looking at log from test bot, and I see :-

Undefined index: selected_modules	Notice	ModulesUninstallForm.php	175	Drupal\system\Form\ModulesUninstallForm->submitForm()
  public function submitForm(array &$form, array &$form_state) {
    // Get the selected modules.
    $selected_modules = $form_state['values']['selected_modules'];  <<<--- line 175

    // Save all the values in an expirable key value store.
    $account = $this->currentUser()->id();
    $this->keyValueExpirable->setWithExpire($account, $selected_modules, 60);

    // Redirect to the confirm form.
    $form_state['redirect_route']['route_name'] = 'system.modules_uninstall_confirm';
  }

and yet there is no definition of 'selected_modules' in the buildForm method ( only 'modules' )
( NB This WOT NO 'selected_module' index issue also applies to validateForm )

I think the appropriate index is 'modules' but I need sleep... so I am just posting here. I will have another look at this tomorrow.

joelpittet’s picture

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallForm.php
@@ -125,24 +148,38 @@ public function buildForm(array $form, array &$form_state) {
+    $selected_modules = $form_state['values']['selected_modules'];

This is looking for selected_modules

joelpittet’s picture

Sorry there was more to that:

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallForm.php
@@ -125,24 +148,38 @@ public function buildForm(array $form, array &$form_state) {
+      $form_state['values']['selected_modules'] = $selected_modules;

Which only get's set on else.

mfernea’s picture

$form_state['values']['selected_modules'] is used to store the actual list of selected modules. We need that list on both validate and submit. So I thought to create it on validate and use it on submit. Validation should pass only if there are modules selected so the $form_state['values']['selected_modules'] is set.
I'm wondering: how can it get to submit phase without passing successfully through validate? Maybe I'm making a wrong assumption.

martin107’s picture

Assigned: Unassigned » martin107
Status: Needs work » Needs review
FileSize
7.72 KB
2.92 KB

Ok so there are 2 bugs:-

1) The table render structure was malformed in a few places... for example

-          $form['modules'][$module->name]['checkbox']['#disabled'] = TRUE;
+          $form['modules'][$module->name]['uninstall']['checkbox']['#disabled'] = TRUE;

2) As identified in #18, #19, #20... the minimal approach is to just set 'selected_modules' even when it is array()

Status: Needs review » Needs work

The last submitted patch, 22: system-convert-to-twig-2151113-22.patch, failed testing.

star-szr’s picture

This is quickly becoming a duplicate of #1938930: Convert theme_system_modules_details() to #type table, should we just merge it back in there?

martin107’s picture

Status: Needs work » Closed (duplicate)

Thanks Cottser - you saved me some work... I will start contributing to the other issue.

martin107’s picture

Assigned: martin107 » Unassigned
akalata’s picture

Status: Closed (duplicate) » Needs work
Related issues: +#2151109: Convert theme_system_modules_details() to Twig

Re-opening this issue since we can use #2151109: Convert theme_system_modules_details() to Twig as a guideline to create the system-modules-uninstall template.

akalata’s picture

Issue summary: View changes
Issue tags: -needs profiling
stefan.r’s picture

Priority: Normal » Major

Getting rid of theme functions should be major, right?

mr.baileys’s picture

Assigned: Unassigned » mr.baileys
mr.baileys’s picture

Just uploading WIP to discuss with @joelpittet

mr.baileys’s picture

Status: Needs work » Closed (duplicate)

Talked this through with @akalata, and since the current theme function includes form elements besides the table itself, the approach in #2151109: Convert theme_system_modules_details() to Twig might be better than converting the entire theme function to a twig template as attempted here. Re-opening the other issue, and marking this one as duplicate.

akalata’s picture

Issue summary: View changes
Status: Closed (duplicate) » Needs review
Issue tags: +Barcelona2015
FileSize
7.78 KB
5.05 KB

@mr.baileys we were really really close here! joelpittet helped me figure out what we were missing. (Your patch also missed adding the new template file, so you might want to double check your git workflow on that.)

akalata’s picture

Adding in some fun Twig magic to make Joël happy :)

joelpittet’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/system.admin.inc
    @@ -277,83 +277,59 @@ function theme_system_modules_details($variables) {
    + * @todo
    

    I'm guessing this needs to be done or remove the @todo.

  2. +++ b/core/modules/system/system.admin.inc
    @@ -277,83 +277,59 @@ function theme_system_modules_details($variables) {
    +  $variables['header'] = array(
    ...
    +  );
    

    nit: short array syntax here.

  3. +++ b/core/modules/system/system.admin.inc
    @@ -277,83 +277,59 @@ function theme_system_modules_details($variables) {
       // No theming for the confirm form.
       if (isset($form['confirm'])) {
         return drupal_render($form);
       }
    

    This will not work unfortunately and will need to be returned dealt with in the template. Hmm.... May just need a large condition if form.confirm %}{{ form }}{% else %}

  4. +++ b/core/modules/system/system.admin.inc
    @@ -277,83 +277,59 @@ function theme_system_modules_details($variables) {
    +    if (!empty($module['#required_by'])) {
    +      $module['#validation_reasons'][] = \Drupal::translation()
    +        ->translate('Required by: @modules', array('@modules' => implode(', ', $module['#required_by'])));
         }
    

    This should move to the template as well like the modules page conversion.

  5. +++ b/core/modules/system/system.admin.inc
    @@ -277,83 +277,59 @@ function theme_system_modules_details($variables) {
    +      $module['disabled_header'] = \Drupal::translation()->formatPlural(count($module['#validation_reasons']),
    ...
    -        array('@module' => $form['modules'][$module]['#module_name']));
    +        array('@module' => $module['#module_name']));
    

    Same with this t() can be moved to the template.

  6. +++ b/core/modules/system/templates/system-modules-uninstall.html.twig
    @@ -0,0 +1,77 @@
    +{{ form|without(
    +    'filters',
    +    'modules',
    +    'uninstall'
    +) }}
    

    the indent looks a bit odd on this, may just need the parenthesis on the previous line.

akalata’s picture

Assigned: mr.baileys » Unassigned
Status: Needs work » Needs review
FileSize
207.69 KB
180.35 KB

#35.1: Pretty sure we wanted to document the pieces of the child elements of the form. This is done.

#35.2: Done.

#35.3: If it's not working then I don't know what it was doing (if anything) - removed it entirely.

#35.4 and #35.5: I'm betting there was an easier way to do this. :)

#35.6: Adjusted indent, does that help?

Status: Needs review » Needs work

The last submitted patch, 36: 2151113-36-modules-uninstall-twig.patch, failed testing.

The last submitted patch, 36: 2151113-36-modules-uninstall-twig.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
8.67 KB
6.01 KB

ugh I really should pay more attention. on the other hand, testbot_speed++

star-szr’s picture

Overall this looks very good at a glance, thank you for all the hard work @akalata you rock!

  1. +++ b/core/modules/system/system.admin.inc
    @@ -277,83 +277,63 @@ function theme_system_modules_details($variables) {
    +/*
    ...
    +    }*/
    

    Commented code should be removed.

  2. +++ b/core/modules/system/system.admin.inc
    @@ -277,83 +277,63 @@ function theme_system_modules_details($variables) {
    +    krumo($module);
    

    Leftover debug :)

  3. +++ b/core/modules/system/templates/system-modules-uninstall.html.twig
    @@ -0,0 +1,84 @@
    +{{ form|without(
    +  'filters',
    +  'modules',
    +  'uninstall'
    +) }}
    

    Just my preference but since this isn't really long maybe we should just put it all on one line?

akalata’s picture

Thanks Cottser, happy to help!

star-szr’s picture

Status: Needs review » Needs work

I took another look, here are some more thoughts:

  1. +++ b/core/modules/system/system.admin.inc
    @@ -277,83 +277,49 @@ function theme_system_modules_details($variables) {
    + * Prepares variables for the module uninstall template.
    

    I think this should be updated to something like "Prepares variables for module uninstall templates" per https://www.drupal.org/node/1354#themepreprocess.

  2. +++ b/core/modules/system/system.admin.inc
    @@ -277,83 +277,49 @@ function theme_system_modules_details($variables) {
    + *       cannot be disabled.
    

    Strictly speaking this should say "uninstalled" rather than "disabled" shouldn't it?

  3. +++ b/core/modules/system/system.admin.inc
    @@ -277,83 +277,49 @@ function theme_system_modules_details($variables) {
    -  // No theming for the confirm form.
    -  if (isset($form['confirm'])) {
    -    return drupal_render($form);
    -  }
    

    This threw me off but I see this was already accounted for and I confirmed it works as expected :)

  4. +++ b/core/modules/system/system.admin.inc
    @@ -277,83 +277,49 @@ function theme_system_modules_details($variables) {
    -      $form['modules'][$module]['#validation_reasons'][] = \Drupal::translation()->translate('Required by: @modules', array('@modules' => implode(', ',$form['modules'][$module]['#required_by'])));
    
    +++ b/core/modules/system/templates/system-modules-uninstall.html.twig
    @@ -0,0 +1,80 @@
    +                    <li>{{ 'Required by:'|t }} {{ module.required_by|safe_join(', ') }}</li>
    

    I'm not sure about this translatable string change just yet, see #2151109-49: Convert theme_system_modules_details() to Twig for my reasoning. I'm not sure if we can do the safe_join in a trans block though.

  5. +++ b/core/modules/system/templates/system-modules-uninstall.html.twig
    @@ -0,0 +1,80 @@
    +#}
    +
    +{{ form.filters }}
    

    Minor: I think there's an unneeded blank line at the start of the template.

I also found a string reference to theme_system_modules_uninstall() in /core/lib/Drupal/Core/Extension/ModuleUninstallValidatorInterface.php that could be updated to point to the preprocess function I think.

RainbowArray’s picture

This should address the feedback in #42.

joelpittet’s picture

Thanks @mdrummond. I've went a bit closer to the end goal by moving the t() calls to the template, just leaving the renders in preprocess. And some minor coding standard stuff.

RainbowArray’s picture

Issue summary: View changes
FileSize
365.93 KB

Manually tested this. There is a minor visual discrepancy where the text shifts a bit in the table, so I looked at the source code.

Before is on the right, after is on the left:

There are a few differences.

1) Missing the div with tableresponsive-toggle-columns

2) Table has class system-modules-uninstall instead of responsive-enabled with data-striping="1"

3) Reasons for being installed has div class="item-list" and a whole bunch more markup in that list than what is in HEAD.

RainbowArray’s picture

Also manually tested the confirm form. Looks the same both visually and in the markup.

joelpittet’s picture

This doesn't resolve it all but does resolve a bunch. The safe_join() here is not a regression because in core it's using that.

Status: Needs review » Needs work

The last submitted patch, 47: convert-2151113-47.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

The last submitted patch, 31: 2151113-31-modules-uninstall-twig.patch, failed testing.

The last submitted patch, 31: 2151113-31-modules-uninstall-twig.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 47: convert-2151113-47.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review

star-szr’s picture

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

I'm guessing for classes we want to err on the side of Classy, see screenshot for classes we can add to the empty message (tested with Seven).

This is looking really close in my opinion.

  1. +++ b/core/modules/system/system.admin.inc
    @@ -248,83 +248,49 @@ function template_preprocess_system_modules_details(&$variables) {
     /**
    - * Returns HTML for a table of currently disabled modules.
    + * Prepares variables for module uninstall templates.
      *
      * @param $variables
      *   An associative array containing:
    

    The preprocess docblock is missing the 'Default template: system-modules-uninstall.html.twig' line.

  2. +++ b/core/modules/system/system.admin.inc
    @@ -248,83 +248,49 @@ function template_preprocess_system_modules_details(&$variables) {
    +      $renderer = \Drupal::service('renderer');
    

    Is the renderer used anywhere now? Seems like it can be removed.

  3. +++ b/core/modules/system/templates/system-modules-uninstall.html.twig
    @@ -0,0 +1,74 @@
    + *   - checkbox_id: A unique id for interacting with the checkbox element.
    

    Minor: On the other issue (module install page) we used identifier instead of id here.

  4. +++ b/core/modules/system/templates/system-modules-uninstall.html.twig
    @@ -0,0 +1,74 @@
    + *  @see template_preprocess_system_modules_uninstall()
    

    Minor: Extra space here between * and @, two spaces instead of one.

The last submitted patch, 47: convert-2151113-47.patch, failed testing.

RainbowArray’s picture

This should fix the coding standard issues detailed in #56.

joelpittet’s picture

Covering the empty message classes mentioned in #56

star-szr’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
39.04 KB
13.01 KB

It's worth mentioning that this removes tableresponsive.js from /admin/modules/uninstall, but it appears as though it wasn't actually being used before because no priorities were set.

This is ready to go, markup looks good. I compared with a visual difftool and DaisyDiff. The only non-whitespace markup change is some additional data-drupal-selector attributes (one per row) but those are just the result of actually printing all the attributes instead of hardcoding it in the theme function, so I think if they're to be removed it should happen earlier than the template. I think they might just be auto-generated because it's coming from a form.

I also tested the JavaScript filter and the form manually and everything works as expected.

This patch manages to also fix a grammar issue in HEAD :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 525430b on 8.0.x
    Issue #2151113 by akalata, joelpittet, mdrummond, mfernea, martin107, mr...

Status: Fixed » Closed (fixed)

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

cilefen’s picture

cilefen’s picture

Actually, there is an exception message in ModuleInstaller that is still wrong, so we will fix that in the other issue.