Problem
Using the module filter on the module list page a module can only be found by system name if the description ends in a dot or other word boundary separator. This is caused by the fact that we concatenate together the module description and machine name without adding any spaces. Therefore the Regex in Drupal.behaviors.tableFilterByText showModuleRow which uses word boundaries only recognizes the word if the description contains a period to end the sentence and make the machine name its own word.
This is why for example searching for "dblog" works currently but searching for "yoast" doesn't show the Real-Time SEO module in the 8.x-2.0-alpha3 release.
Proposed Solution
Make the showModuleRow slightly more accurate by ensuring the values of the different possible search sources are separated by a space.
| Comment | File | Size | Author |
|---|---|---|---|
| #28 | 2978498-28.patch | 1.54 KB | gauravvvv |
| #26 | 2978498-26.patch | 1.34 KB | rishabh vishwakarma |
| #16 | core-2978498-16.patch | 1.97 KB | star-szr |
Issue fork drupal-2978498
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
Comment #2
kingdutchThe attached patch uses map (es5 feature with wide support: https://caniuse.com/#feat=es5) to return the text of each element which can then be combined with a join using a space. This ensures there's no hidden requirements on the description field for the system name to be searchable.
Added related issue for the uninstall page that may suffer from a similar issue.
Comment #4
joachim commented> This is why for example searching for "dblog" works currently but searching for "yoast" doesn't show the Real-Time SEO module in the 8.x-2.0-alpha3 release.
I don't understand why the problem happens. What is different about 'dblog' and 'yoast'? Can you explain in more detail please?
Comment #5
kingdutchThe description for the Real-Time SEO module is:
"Adds Real-time SEO page analysis and configuration"
This description does not end in a period. For historic reasons the module has a different name from its system name (system name wasn't changed for BC reasons). Because the description does not end with a period, when I search for the term "yoast" (part of the module system name) then this will not match because the search string contains "Adds Real-time SEO page analysis and configurationyoast" which puts "configurationyoast" together.
The description for the dblog module is:
"Logs and records system events to the database."
Neither the module name nor the description contains the word "dblog". However, when you search for "dblog" in the module filter then the search text that is used is "Logs and records system events to the database.dblog". The current search uses word boundaries and the dot in the module description causes "dblog" to be a separate word. This causes the dblog module to be findable for its system name.
------
The minimal module that could reproduce not being searchable by system name:
name (system name) - Description not ending in a period
Some Module (different) - This is a module that can not be found by its system name
The same module searchable by system name:
name (system name) - Description ending in a period
Some Module (different) - This is a module that can be found by its system name.
The patch ensures that both of the above cases can be found.
Comment #6
kingdutchOops. Patch is attached so status should be Needs Review.
Comment #7
kingdutchWe now have automated Javascript testing that can help me demonstrate this and show that the fix works. I'll take a crack at it this weekend or next week :)
Comment #8
avpadernoComment #15
jonathan1055 commentedThis is still an error in 10.1. The javascript has changed but the fault was replicated.
@kingdutch - this issue is assigned to you. Would you like to work on it? If not then I will.
Comment #16
star-szrI ran into this as well, coming from #3264940: Add optional parameter to Nightwatch drupalInstallModule command to enable module dependencies.
New patch, so no interdiff. Test-only, then test + fix.
Comment #17
star-szrUpdating the issue summary a bit.
Comment #18
star-szrI want to add that we could certainly add a test module that is dedicated to testing this scenario, and I can do so if requested rather than arbitrarily using
comment_base_field_test.Comment #20
smustgrave commentedVerified the issue by removing the period from the description of dblog
Searching for dblog didn't return anything.
Apply patch
Searching for dblog returns the module even without a period at the end of the description.
Comment #21
jonathan1055 commentedNow that this is marked as RTBC there's a flag by the test-only patch saying it will be retested every two days. But that's not going to help anything, as we know it fails (by design). So I'm hiding that patch, just to see if the good one gets tagged with automated retesting instead.
[edit - yes that seemed to work, the flag is now on the good patch]
Comment #23
lauriiiThanks for the fix @Cottser. Happy to see you in the queues again! 👋😊
Committed 753eef8 and pushed to 10.1.x. Thanks!
Comment #24
jonathan1055 commentedThanks for the commit. Would you consider backporting to 9.x? This is such a simple fix to make and I'd be happy to do a MR with the same changes if no one else wants to? (can't be cherry-picked directly because the js file is very slightly different)
Comment #25
lauriiiI didn't originally backport this because I thought there could be a small risk of some Nightwatch tests failing in contrib because this might make modules visible in the searches that weren't visible before. That said, I don't think this is huge risk and there aren't that many contributed modules using Nightwatch. Based on that, I think we could backport this.
Comment #26
rishabh vishwakarma commentedAdding patch for 9.5.x
Comment #27
jonathan1055 commented@Rishabh Vishwakarma I had just created a new issue branch and was working on this. The implication from #24 was that I would do this work unless someone volunteered. So I have just wasted my time now. I guess I should have assigned this issue to myself before starting it. Or you should have. I was also working on adding test coverage at 9.5.
Comment #28
gauravvvv commentedRe-rolled patch #16, for 9.5.x
Comment #29
smustgrave commented@jonathan1055 what MR? If there is one we should be doing the fix there.
Patch #28 dropped the test cases.
Comment #30
gauravvvv commentedWe don't have test case file in 9.5.x
core/modules/system/tests/src/FunctionalJavascript/ModuleFilterTest.phpComment #31
jonathan1055 commentedThat's correct, and it is what I was doing as a follow-up to #24 and #25 when the new patch was uploaded in #26. I had created the issue branch and was working locally but had not pushed it up to d.o. so not made a MR yet. But I stopped work when I thought that @Rishabh Vishwakarma was taking on the tasks.
Also, just a quick question for @Gauravvvv, patch #28 is not precisely a re-roll of #16.
In #16 the change for
core/modules/system/js/system.modules.jshasbut in patch #28 this is became
There may be a good reason for this, but I just want to know. We should aim to keep 9.x code in line with 10.x code where possible as that makes future maintainance and backporting easier.
Comment #32
gauravvvv commentedI have not made any chnages manually into
core/modules/system/js/system.modules.jsfile. All these changes are compiled by yarn.Also both of these lines of code achieve the same result, which is concatenating a space character and the text content of the item variable to the sourcesConcat variable
Comment #33
avpadernoComment #35
jonathan1055 commentedThe change was committed to 10.1.x in March 2023. It was not back-ported to D9 but that does not matter now, so I think this can be marked as 'Fixed'
Comment #36
jonathan1055 commentedShorter title.
Adding parent issue for reference.