Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Patch- Patch review
- Manual testing
Steps to reproduce/review
- Install Drupal 8.
- Visit admin/modules/uninstall.
- Confirm that the list of modules available to uninstall appears correctly in HEAD and with the patch applied.
- Select a module to delete and submit the form using the Uninstall button.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#60 | 2151113-data-attributes.png | 13.01 KB | star-szr |
#60 | 2151113-grammar.png | 39.04 KB | star-szr |
#59 | convert-2151113-59.patch | 8.96 KB | joelpittet |
#59 | interdiff.txt | 775 bytes | joelpittet |
#58 | interdiff-2151113-47-58.txt | 1.87 KB | RainbowArray |
Comments
Comment #1
star-szrAdding 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.
Comment #2
star-szrComment #3
mfernea CreditAttribution: mfernea commentedI'm looking into this.
Comment #4
mfernea CreditAttribution: mfernea commentedComment #5
mfernea CreditAttribution: mfernea commentedPlease provide feedback as this our first Twig related patch. :)
Comment #6
FelkerL CreditAttribution: FelkerL commented1
Comment #7
FelkerL CreditAttribution: FelkerL commentedsorry, forgot the screenshot ;)
Comment #8
mfernea CreditAttribution: mfernea commented@FelkerL: Is the screenshot related to this issue? :)
Comment #9
FelkerL CreditAttribution: FelkerL commented@mfernea: sorry, I failed :( it is the wrong screenshot and the wrong issue!
Comment #11
star-szr5: system-convert-to-twig-2151113-5.patch queued for re-testing.
Comment #12
YesCT CreditAttribution: YesCT commentedfixing tag. sorry for noise.
Comment #13
mfernea CreditAttribution: mfernea commentedI 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.
Comment #14
mfernea CreditAttribution: mfernea commentedI'm working on patch to remove theme_system_modules_uninstall and use the #type table when building the form.
Comment #15
mfernea CreditAttribution: mfernea commentedI wrote a patch for this so now we are using #type table.
The main modifications are:
Few questions:
Comment #17
star-szrComment #18
martin107 CreditAttribution: martin107 commentedLooking at log from test bot, and I see :-
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.
Comment #19
joelpittetThis is looking for selected_modules
Comment #20
joelpittetSorry there was more to that:
Which only get's set on else.
Comment #21
mfernea CreditAttribution: mfernea commented$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.
Comment #22
martin107 CreditAttribution: martin107 commentedOk so there are 2 bugs:-
1) The table render structure was malformed in a few places... for example
2) As identified in #18, #19, #20... the minimal approach is to just set 'selected_modules' even when it is array()
Comment #24
star-szrThis is quickly becoming a duplicate of #1938930: Convert theme_system_modules_details() to #type table, should we just merge it back in there?
Comment #25
martin107 CreditAttribution: martin107 commentedThanks Cottser - you saved me some work... I will start contributing to the other issue.
Comment #26
martin107 CreditAttribution: martin107 commentedComment #27
akalata CreditAttribution: akalata commentedRe-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.
Comment #28
akalata CreditAttribution: akalata commentedComment #29
stefan.r CreditAttribution: stefan.r commentedGetting rid of theme functions should be major, right?
Comment #30
mr.baileysComment #31
mr.baileysJust uploading WIP to discuss with @joelpittet
Comment #32
mr.baileysTalked 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.
Comment #33
akalata CreditAttribution: akalata commented@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.)
Comment #34
akalata CreditAttribution: akalata commentedAdding in some fun Twig magic to make Joël happy :)
Comment #35
joelpittetI'm guessing this needs to be done or remove the @todo.
nit: short array syntax here.
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 %}
This should move to the template as well like the modules page conversion.
Same with this t() can be moved to the template.
the indent looks a bit odd on this, may just need the parenthesis on the previous line.
Comment #36
akalata CreditAttribution: akalata commented#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?
Comment #39
akalata CreditAttribution: akalata commentedugh I really should pay more attention. on the other hand, testbot_speed++
Comment #40
star-szrOverall this looks very good at a glance, thank you for all the hard work @akalata you rock!
Commented code should be removed.
Leftover debug :)
Just my preference but since this isn't really long maybe we should just put it all on one line?
Comment #41
akalata CreditAttribution: akalata commentedThanks Cottser, happy to help!
Comment #42
star-szrI took another look, here are some more thoughts:
I think this should be updated to something like "Prepares variables for module uninstall templates" per https://www.drupal.org/node/1354#themepreprocess.
Strictly speaking this should say "uninstalled" rather than "disabled" shouldn't it?
This threw me off but I see this was already accounted for and I confirmed it works as expected :)
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.
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.
Comment #43
RainbowArrayThis should address the feedback in #42.
Comment #44
joelpittetThanks @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.
Comment #45
RainbowArrayManually 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.
Comment #46
RainbowArrayAlso manually tested the confirm form. Looks the same both visually and in the markup.
Comment #47
joelpittetThis 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.
Comment #50
joelpittetComment #54
star-szrComment #56
star-szrI'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.
The preprocess docblock is missing the 'Default template: system-modules-uninstall.html.twig' line.
Is the renderer used anywhere now? Seems like it can be removed.
Minor: On the other issue (module install page) we used identifier instead of id here.
Minor: Extra space here between * and @, two spaces instead of one.
Comment #58
RainbowArrayThis should fix the coding standard issues detailed in #56.
Comment #59
joelpittetCovering the empty message classes mentioned in #56
Comment #60
star-szrIt'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 :)
Comment #61
webchickCommitted and pushed to 8.0.x. Thanks!
Comment #64
cilefen CreditAttribution: cilefen commentedIt looks like this one fixed #2392533: Grammatical error in ModuleUninstallValidatorException message.
Comment #65
cilefen CreditAttribution: cilefen commentedActually, there is an exception message in ModuleInstaller that is still wrong, so we will fix that in the other issue.