Problem/Motivation

When #2895388: Modules uninstall filter does not filter by machine name is committed this module can also add the machine name when filtering the uninstall page.

But instead of waiting for that issue, which could take a long time, we can apply the patch from that MR to the core files via module_filter's drupalci.yml and .gitlab-ci.yml files, so that we can finish this work and include it in testing.
The patch file is https://git.drupalcode.org/project/drupal/-/merge_requests/3273.diff

Remaining tasks

  1. Add machine_name to the js text selector [done]
  2. Expand text coverage for the new value [done]
  3. Patch core files in our testing configuration [done]
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

Title: Add machine_name to the uninstall page filter » Add machine name to the uninstall page filter
Issue summary: View changes

Linked to the wrong issue.

jonathan1055’s picture

Issue summary: View changes

Sorry, having trouble typing. I have cut my finger so using the wrong hand (well that's my excuse). Also forgetting to 'preview' :-)

jonathan1055’s picture

Status: Active » Needs review

Added test first. This should fail.

jonathan1055’s picture

The log has cd ${SOURCE_DIR} && sudo -u www-data curl https://git.drupalcode.org/project/drupal/-/merge_requests/3273.diff | sudo -u www-data patch -p1 --verbose but there is no actual confirmation that the patch was sucessful. Anyway, lets try with the modified filter.

smustgrave made their first commit to this issue’s fork.

smustgrave’s picture

Status: Needs review » Postponed
jonathan1055’s picture

The second patch I added here includes the small change in the first patch, and the patching failed, so I removed the first one.

jonathan1055’s picture

That's good in a way. We now have the drupalCI test failure matching the gitlab pipeline test failure, so we are at least testing the same thing and getting the same outcome.

Drupal\Tests\module_filter\FunctionalJavascript\ModuleFilterJavascriptUninstallPageTest::testUninstallPageFiltering

Behat\Mink\Exception\ResponseTextException: The text "Roses" was not found anywhere in the text of the current page.

/builds/issue/module_filter-3335419/tests/src/FunctionalJavascript/ModuleFilterJavascriptUninstallPageTest.php:33

The above failure is the new bit added to the uninstall test, to demonstrate the new functionality.

The DrupalCI tests were passing in Jan 2023 but failing in Nov 2023. Same D10.1, php 8.1.

Attached are screen shots of the downloaded html output. The filtered page is indeed not showing any modules at all.

jonathan1055’s picture

Title: Add machine name to the uninstall page filter » Add machine name to the uninstall page filter [module_filter]
Related issues: +#2895388: Modules uninstall filter does not filter by machine name
StatusFileSize
new204.79 KB

The previous image did not have the description opened. This one shows the 'red' text which should be found and filtered on.

jonathan1055’s picture

Tested this in the UI on 10.1 and 10.2 and it fails exactly like the javascript test. It also does not seem to filter on the module readable name either. But it does filter on the description (which is really weird). Using core modules for testing:

Readable name

  • Enter "link" and we get the 'Contextual Links' and 'Custom Menu Links' modules
  • Enter "cust" and we get just the Path module, which has description containing "custom". We do not get the 'Custom Menu Links' module

Machine name

  • Enter "log" and we get the 'Database Logging' module
  • Enter "logg" and we get nothing. It is the 'log' in the description which matches.
  • Enter "dblog" and we get nothing. The machine name 'dblog' in not found

This used to work properly, so I don't know what has changed and whats gone wrong.

jonathan1055’s picture

Status: Postponed » Needs review

I have found and fixed the problems:

  1. the .module-machine-name used to be on the td cell itself, but has now been moved into the span element.
  2. to cater for the Claro theme we need to also add .module-list__module-name. It remains as .module-name in Olivero [the commit msg should say Claro not Olivero]

Tests pass. Needs review.

smustgrave’s picture

Status: Needs review » Needs work

Don't seem to be able to filter by machine name. Did a fresh install and searched for block_content and got no results.

jonathan1055’s picture

Status: Needs work » Needs review
StatusFileSize
new110.93 KB

"block_content" It works for me. Although I am having real problems with getting any chnages to .js to be recognised in a timely fashion. I do not have javascript aggregated, and have no caching, I clear the cache via Devel, via admin/config/development/performance and via drush, and the updated .js is not acted on. It makes development impossible. I even restart my local server. But then I wait 5 mins, do something else, and come back to module_filter and the updated code is active. Its driving me nuts!

jonathan1055’s picture

oops, did not mean to change the status. I just refreshed the page, made my comment, after reading yours, but the stale form setting got saved.

jonathan1055’s picture

Issue summary: View changes

I forgot to ask what theme(s) you are using to test? I checked this lastest change for Claro and Olivero in 10.1 and 10.2.

Actually there may be a safer, more consistent and future-proof way to identify the elements for text filtering. There is a class table-filter-text-source which would seem to be designed specifically for this type of filtering. It is on the module name and machine name in both themes, but it is not on the description. But that will still simplyfy the coding.

smustgrave’s picture

Status: Needs review » Needs work

Needs a rebase, also we can do it here or a follow up but think going to drop drupalCi file in favor of just gitlab.

jonathan1055’s picture

Status: Needs work » Needs review

Rebased locally and pushed. Ready for review.

From your problem in #14 and my question in #17 - What theme are you using when you can't get the filter to work correctly?

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Long time coming but welcomes addition!

smustgrave’s picture

Status: Reviewed & tested by the community » Fixed

I should of marked Fixed

jonathan1055’s picture

Issue summary: View changes

Updated IS to include a note about patching core in our test files.

Status: Fixed » Closed (fixed)

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