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

Files: 
CommentFileSizeAuthor
#29 2507243-21.patch12.27 KBjmolivas
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,290 pass(es). View
#25 interdiff-2507243-21-22.txt3.25 KBjmolivas
#22 interdiff-2507243-21-22.txt3.25 KBjmolivas
#22 convert-2507243-22.patch14.15 KBjmolivas
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,130 pass(es). View
#21 interdiff-2507243-21.txt2.41 KBlokapujya
#21 2507243-21.patch12.27 KBlokapujya
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,068 pass(es). View
#15 interdiff-2507243-54.patch862 byteslokapujya
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,091 pass(es), 1 fail(s), and 0 exception(s). View
#15 2507243-54.patch12.19 KBlokapujya
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,072 pass(es). View
#10 interdiff-2507243-9.txt1.76 KBlokapujya
#10 2507243-9.patch11.35 KBlokapujya
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,031 pass(es), 1 fail(s), and 0 exception(s). View
#5 interdiff-2507243-1-5.txt1.94 KBlokapujya
#5 2507243-5.patch10.54 KBlokapujya
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,051 pass(es), 2 fail(s), and 0 exception(s). View
#3 2507243-3.patch10.51 KBlokapujya
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,023 pass(es), 2 fail(s), and 0 exception(s). View
#3 interdiff-2507243-3.txt1.21 KBlokapujya
#1 convert-2507243-1.patch10.26 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,926 pass(es), 3 fail(s), and 0 exception(s). View

Comments

joelpittet’s picture

Status: Active » Needs review
FileSize
10.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,926 pass(es), 3 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

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

lokapujya’s picture

Status: Needs work » Needs review
FileSize
1.21 KB
10.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,023 pass(es), 2 fail(s), and 0 exception(s). View

So, formatPlural needs safeMarkup into it?

Status: Needs review » Needs work

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

lokapujya’s picture

Status: Needs work » Needs review
FileSize
10.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,051 pass(es), 2 fail(s), and 0 exception(s). View
1.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

Assigned: Unassigned » jmolivas

I will give a try to fix the failing tests

lokapujya’s picture

Status: Needs work » Needs review
FileSize
11.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,031 pass(es), 1 fail(s), and 0 exception(s). View
1.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

ooo one fail left! Nice

jmolivas’s picture

lokapujya: awesome

joelpittet: I'll take care of the last one

lokapujya’s picture

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

Status: Needs work » Needs review
FileSize
12.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,072 pass(es). View
862 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,091 pass(es), 1 fail(s), and 0 exception(s). View

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

Status: Needs work » Needs review
jmolivas’s picture

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

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

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

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

jmolivas’s picture

FileSize
14.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,130 pass(es). View
3.25 KB

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

joelpittet’s picture

@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

Status: Needs review » Needs work
jmolivas’s picture

FileSize
3.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

@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

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

Cottser’s picture

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

FileSize
12.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,290 pass(es). View

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

Cottser’s picture

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

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

Status: Needs work » Closed (duplicate)
Related issues: +#2151113: Convert theme_system_modules_uninstall() to Twig
mr.baileys’s picture

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

Status: Needs review » Closed (duplicate)