Follow-up to #1938930: Convert theme_system_modules_details() to #type table

Please add these people to the commit message as they worked on it in the issue this was split from:

martin107, akalata, joelpittet, lokapujya, rpayanm, duellj, Hydra, amitgoyal, prajaankit

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

CommentFileSizeAuthor
#29 2507243-21.patch12.27 KBjmolivas
#25 interdiff-2507243-21-22.txt3.25 KBjmolivas
#22 interdiff-2507243-21-22.txt3.25 KBjmolivas
#22 convert-2507243-22.patch14.15 KBjmolivas
#21 interdiff-2507243-21.txt2.41 KBlokapujya
#21 2507243-21.patch12.27 KBlokapujya
#15 interdiff-2507243-54.patch862 byteslokapujya
#15 2507243-54.patch12.19 KBlokapujya
#10 interdiff-2507243-9.txt1.76 KBlokapujya
#10 2507243-9.patch11.35 KBlokapujya
#5 interdiff-2507243-1-5.txt1.94 KBlokapujya
#5 2507243-5.patch10.54 KBlokapujya
#3 2507243-3.patch10.51 KBlokapujya
#3 interdiff-2507243-3.txt1.21 KBlokapujya
#1 convert-2507243-1.patch10.26 KBjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Status: Active » Needs review
FileSize
convert-2507243-1.patch10.26 KB

Here's the split off from #1938930: Convert theme_system_modules_details() to #type table

Status: Needs review » Needs work

The last submitted patch, 1: convert-2507243-1.patch, failed testing.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Status: Needs work » Needs review
FileSize
interdiff-2507243-3.txt1.21 KB
2507243-3.patch10.51 KB

So, formatPlural needs safeMarkup into it?

Status: Needs review » Needs work

The last submitted patch, 3: 2507243-3.patch, failed testing.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Status: Needs work » Needs review
FileSize
2507243-5.patch10.54 KB
interdiff-2507243-1-5.txt1.94 KB

there is 2 formatPlurals().

Status: Needs review » Needs work

The last submitted patch, 5: 2507243-5.patch, failed testing.

joelpittet queued 5: 2507243-5.patch for re-testing.

The last submitted patch, 5: 2507243-5.patch, failed testing.

jmolivas’s picture

jmolivas CreditAttribution: jmolivas at FFW commented
Assigned: Unassigned » jmolivas

I will give a try to fix the failing tests

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Status: Needs work » Needs review
FileSize
2507243-9.patch11.35 KB
interdiff-2507243-9.txt1.76 KB

Assignment changed while I was uploading this. @jmolivas: You can work on it. I'll upload anyway, because I want to see the test results. This should fix one fail.

Status: Needs review » Needs work

The last submitted patch, 10: 2507243-9.patch, failed testing.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented

ooo one fail left! Nice

jmolivas’s picture

jmolivas CreditAttribution: jmolivas at FFW commented

lokapujya: awesome

joelpittet: I'll take care of the last one

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented

Last Fail, clue: Something is happening in \Drupal::translation()->translate( line 59 of: core/modules/system/src/Tests/Module/UninstallTest.php that is messing up the string that is passed to the assert().

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Status: Needs work » Needs review
FileSize
2507243-54.patch12.19 KB
interdiff-2507243-54.patch862 bytes

The modules names are not getting displayed instead of the title. Don't know how to get module title.

Status: Needs review » Needs work

The last submitted patch, 15: interdiff-2507243-54.patch, failed testing.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Status: Needs work » Needs review
jmolivas’s picture

jmolivas CreditAttribution: jmolivas at FFW commented

Removing myself as assigned, since I haven't had time

@lokapujya just a recomendation interdiff-2507243-54.patch must be renamed to interdiff-2507243-54.txt, to avoid the test-bot to try testing it

Status: Needs review » Needs work

The last submitted patch, 15: interdiff-2507243-54.patch, failed testing.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Assigned: jmolivas » Unassigned

A couple things still need some help fixing here that I could spot by scanning the patch:

  1. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
@@ -110,7 +111,23 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+        'title' => ['data' => $this->t('Name')],
+        'description' => ['data' => $this->t('Description')],

    I don't think they need the data key for these. 

  2. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
@@ -118,41 +135,51 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+        $disabled_message = \Drupal::translation()->formatPlural(count($required_modules),
...
+        $disabled_message = \Drupal::translation()->formatPlural(count($validation_reasons[$module_key]),
...
+      $form['uninstall'][$module->getName()]['description']['uninstall']['#markup'] = $this->t($disabled_message);

    This looks like it's translated twice? I could be wrong but inside formatPlural it calls doTranslate in the translationManager and SafeMarkup format and so does the t() call.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Status: Needs work » Needs review
FileSize
2507243-21.patch12.27 KB
interdiff-2507243-21.txt2.41 KB

Yes, no need to translate twice just because we added a div wrapper.

jmolivas’s picture

jmolivas CreditAttribution: jmolivas at FFW commented
FileSize
convert-2507243-22.patch14.15 KB
interdiff-2507243-21-22.txt3.25 KB

I replace the Static Service Container wrapper \Drupal::translation() by injecting the string_translation service.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented

@jmolivas that is a bit of scope creep on this issue. Would you mind moving that to a follow-up issue please?

Mention the new issue number and that may get in quicker than this issue, hope not but likely ;)

There are a few other things in this patch like the typo/language fix that should also be in in it's own issue to avoid cluttering.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Status: Needs review » Needs work
jmolivas’s picture

jmolivas CreditAttribution: jmolivas at FFW commented
FileSize
interdiff-2507243-21-22.txt3.25 KB

@joelpittet I replace it on new code that was aded on this patch, I was trying to avoid adding legacy code when adding new lines.

My interdiff file was bad I uploaded again please confirm is still is needed to remove code ?

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented

@jmolivas Yeah it's creeping the scope. The task is to convert one theme_* to a #type=>table. To avoid any overlap with other issues, to make this easier to review and commit we try to avoid adding more than is needed. It's hard to get time for reviewers to look at things we want it as straight forward as possible.

Your changes are an improvement that would be great to have but if this issue didn't make it in for whatever reason it would be nice to still have those changes don't you think? That's why I'm suggesting splitting them out.

jmolivas’s picture

jmolivas CreditAttribution: jmolivas at FFW commented

@joelpittet: I can not create a new issue and there is no code improvement to do in core at this point, since code is not in core yet.

Your changes are an improvement that would be great to have but if this issue didn't make it in for whatever reason it would be nice to still have those changes don't you think?

My proposed changes are not necessary until code get committed, because the code I was improving was added in a previous patch in this same issue.

That said, I need this issue to get committed and after that, add a new issue and then update code which I thinks is unnecessary since code is not been committed yet.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

To be frank that injection change is much lower priority than removing the theme function. If we can reduce the scope the patch is easier to review and maintain as @joelpittet said.

It's also not totally uncommon to open a follow-up and upload a patch that requires another patch to be applied first. But realistically I don't think that injection change is dependent on this conversion, unless there is no translation at all before conversion which I find hard to believe unless there is a bug.

jmolivas’s picture

jmolivas CreditAttribution: jmolivas at FFW commented
FileSize
2507243-21.patch12.27 KB

Uploading patch form #21 this does not included injecting dependency.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Status: Needs work » Needs review

Thanks @jmolivas, it's a tough balance but in my experience smaller scope = greater chance of completion :)

Setting to needs review.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Status: Needs review » Needs work 
+++ b/core/modules/system/src/Form/ModulesUninstallForm.php
@@ -118,41 +135,51 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+        $disabled_message = SafeMarkup::format('<div class="admin-requirements">@disabled_message</div>', ['@disabled_message' => $disabled_message]);
...
+        $disabled_message = SafeMarkup::format('<div class="validation-reasons">@disabled_message</div>', ['@disabled_message' => $disabled_message]);

These are likely better off as an inline template. My brain hasn't really grasped the reasons why, but they are getting accepted with open arms and because they in the render api they can be altered a bit easier.

akalata’s picture

akalata CreditAttribution: akalata commented
Status: Needs work » Closed (duplicate)
Related issues: +#2151113: Convert theme_system_modules_uninstall() to Twig

Closing in favor of #2151113: Convert theme_system_modules_uninstall() to Twig.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented
Status: Closed (duplicate) » Needs review

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

akalata’s picture

akalata CreditAttribution: akalata commented
Status: Needs review » Closed (duplicate)