Modules uninstall filter does not filter by machine name

One minor inconsistency I noticed (in code, not in functionality) is the way that the drupal.tableresponsive library is added. The Install page form already had $form['#attached']['library'][] = 'core/drupal.tableresponsive'; but this was missing from the uninstall form. It does not appear to be needed for manual interactive functioning of the filter, but it is required to allow the javascript tests to react to changing input. It was only when the uninstall form got it's first javascript test (in this issue) that the library was added. However, it was added via {{ attach_library('core/drupal.tableresponsive') }} in templates/system-modules-uninstall.html.twig

So is there a preference for adding the library via the $form or via the twig template? Even if there is no preference, we should make the two consistent, so that we don't ask this question again. I inadvertantly also added the library to the uninstall $form in this MR (because I was working on 🐛 Uninstall form does not add drupal.tableresponsive library Closed: duplicate so we at least need to remove one of them as it is unnecessary to do it twice.

We need to resolve #73 before committing this.

Thanks @alexpott. I have removed the attach from the twig template, and left it in the $form. This also matches what is in the install form.

Retested following steps from #72

Good job @jonathan1055!

I've looked at the design of this and I'm not sure what was implemented in #11 is the best we can do. I think we should do what the install form does and add the machine name to the description so that it appears visually (unlike patches before which added it as hidden text).

The machine name is not at all important when uninstalling a module - it's really only useful as something you might know to search for. Yet we're placing in in front of the description which can contain important things like why a module can not be uninstalled.

Compare...

With...

I agree with @alexpott. Adding the new column just to get the machine name on the page is not the best way to do it. It wastes a lot of screen space, meaning more scrolling, and shows duplicated info for 90% of the modules. This is an admin page, and making admin tasks quick to achieve is a primary goal. It seems from comments #10 and #11 that the decision to make it a new column was done for ease of coding. It's a backwards step in usability and we should be able to improve it to be more like the install page.

In the first instance the machine name can be added into the description cell. In the longer term, we could have a details element, collapsed by default, to match the install page.

Moving to NW for that change then. Makes sense to me.

Happy to give this a try, unless anyone else wants to do it? No need for more than one person to do duplicate work.

Argh, unintentional change (stale-form data?). Back to NW

First draft. Based the details element on the install page. Themeing was OK but not looking quite right, then I realised that Claro has it's own custom twig template for the install page, so I also made one for the uninstall page.

I puposely left the modified twig template indentation wrong on the first commit, so that it is eaaier to see which lines are changed and which have not been altered but merely indented.

If/when this design change is accepted I will update the issue summary.

The test failures were because I missed copying {{ form|without('filters', 'modules', 'uninstall') }} when creating the Claro template, which meant that the Uninstall button was not in the form. I based it on the install template, which does not have this. Interesting that it was only picked up by HelpTopicSearchTest which checks for uninstalling. Or maybe other tests use the system twig template? The HelpTopic test has protected $defaultTheme = 'stark' so I'm not sure why the Claro template was being used anyway. But lucky that it was, otherwise we would have have green passing tests for a major error in the code.

Are these test failures random and/or unrelated to the chnages in this MR?

Failure 26 Jan 2023 at 01:46 GMT https://www.drupal.org/pift-ci-job/2576971 →

Drupal\Tests\ckeditor5\FunctionalJavascript\MediaLibraryTest::testButton
Failed asserting that a NULL is not empty.

Rebase against 10.1.x but no code changes on this MR, we get a different fail
26 Jan 2023 at 11:28 GMT https://www.drupal.org/pift-ci-job/2577847 →

Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5AllowedTagsTest::testMediaElementAllowedTags
Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "filters[media_embed][settings][allowed_view_modes][view_mode_2]" not found.

Branch test at 26 Jan 2023 at 12:42 GMT passes OK.

I think the failures here are unrelated because there was a branch test at 25 Jan 2023 at 11:48 GMT with the same failure in ests/src/FunctionalJavascript/CKEditor5AllowedTagsTest
https://www.drupal.org/pift-ci-job/2576653 →

If they’re ckeditor5 functionalJavascript most likely random.

Yes, either random or unrelated to this MR, core branch did have a failure ealier. Re-ran after new rebase and all passes here now.

Are we in principle agreed that the expanding details element I added in #82 in response to @alexpott in #77 is the right solution? It works for me, but would be good to get confirmation as many others have looked at this issue before I started on it.

I am going to expand the test coverage for the uninstall page to check the Claro template in addition to the system default, as it was only by luck that the HelpTopic tests used the Claro theme and tested uninstalling, which showed the error in my change.

I have expanded the test to cover Claro theme in addition to the default. This will fail, demonstrating that the Claro template needs work. Also attempting to run only the 'system' module tests temporarily, for speed and efficiency.

As expected, we now have one test failure:

There was 1 error:
1) Drupal\Tests\system\Functional\Module\UninstallTest::testUninstallPage with data set #1 ('claro')
Behat\Mink\Exception\ElementNotFoundException: Element matching css "tr[data-drupal-selector="edit-module-test"] .module-machine-name.table-filter-text-source" not found.

This shows that the new claro template system-modules-uninstall.html.twig being tested when it was not before.

Tests now pass. Here are few other tweaks to the claro template, to make it align better with both the Claro install template, as I based the original template only on system uninstall. This is now ready for full review again.

Functional this works like a charm.

Searching for dynamic_page_cache I get nothing
Apply the fix and not I get the module.

Tagging for CR updates as there seems to be additional changes to the system template that others may need to adopt for their custom themes.

Updated the issue summary to match the solution.

I made some updates to the change record but I haven't fully updated it as suggested in #91.

Thanks. The change record still refers to adding a new column, but that is not how the solution is. Since #77 the description is now a details element, collapsed by default, but expandable to show machine name and requirements, similar to what is done on the install page.

Sorry I can't update the change record right now, happy for someone else to do it, or I will next week.

@jonathan1055 can you update the MR for 11.x

The tests on MR 3273 were OK but now we get one failure
https://git.drupalcode.org/issue/drupal-2895388/-/jobs/564727#L1443

Drupal\Tests\media_library\FunctionalJavascript\WidgetViewsTest::testWidgetViews
    "Loading grid view." not found
    Failed asserting that a boolean is not empty.

Is this related, or some other problem?

Akhil Babu → made their first commit to this issue’s fork.

Akhil Babu → changed the visibility of the branch 11.x to hidden.

I have created a new MR against 11.x along with some minor coding standard fixes. Also, upon reviewing the changes,
description table header of core/modules/system/templates/system-modules-uninstall.html.twig uses the priority-medium class while core/themes/claro/templates/admin/system-modules-uninstall.html.twig uses priority-low class. Was this intentional, or was it just a mistake?

Multiple MRs not sure what's to be reviewed. Should have 1 MR visible or specify which is the one to be reviewed.

jonathan1055 → changed the visibility of the branch 2895388-uninstall-machine-name-filter to hidden.

I have hidden MR3273 as that was the original one, on 10.1.x so will not be committed.

@Akhil Babu, can you confim that your first commit on the new MR 6065 is exactly the same as was on the 10.2 MR?

@jonathan1055 Yes, The first commit in 6065 has all the changes from MR 5496. The next 2 commits are for coding standard fixes.

Thanks @Akhil Babu, it looked like that, but better to ask the person who actually did it.

Your quesion in #102

description table header of core/modules/system/templates/system-modules-uninstall.html.twig uses the priority-medium class, while core/themes/claro/templates/admin/system-modules-uninstall.html.twig uses priority-low class. Was this intentional, or was it just a mistake?

I recall that it was difficult to decide which template to base the new one. Should the Claro uninstall match the Claro install or the System Uninstall? I used priority-low because Claro install had that. My thinking was that it was more important to be consistent with Claro, rather than between the two uninstall templates. But happy to change it if that was the wrong decision.

Moving to NW for change record updates.

Also believe we need a template for some of the other themes, stable9, maybe starterkit (not 100% on that).