Radio buttons on Available updates report do nothing

Created on 9 January 2024, 12 months ago
Updated 7 February 2024, 11 months ago

Problem/Motivation

The radio buttons provided by this module on the Available updates report (admin/reports/updates) do not work and a JS error is logged to the console.

Steps to reproduce

- Enable module_filter 4.1.0 on Drupal 10.2.0 or later.
- Visit the Available updates report (admin/reports/updates).
- Toggle one of the radio buttons provided by this module, e.g. 'Update available'.
- Note nothing changes and JS error logged to the browser console.

On page load:

Uncaught TypeError: Cannot read properties of undefined (reading 'toLowerCase')
    at ce.fn.init.val (jquery.min.js?v=3.7.1:2:67761)
    at HTMLInputElement.<anonymous> (module_filter.update_status.js?v=10.2.1:118:26)
    at HTMLInputElement.dispatch (jquery.min.js?v=3.7.1:2:40035)
    at v.handle (jquery.min.js?v=3.7.1:2:38006)
    at Object.trigger (jquery.min.js?v=3.7.1:2:70124)
    at HTMLInputElement.<anonymous> (jquery.min.js?v=3.7.1:2:70726)
    at Function.each (jquery.min.js?v=3.7.1:2:3129)
    at ce.fn.init.each (jquery.min.js?v=3.7.1:2:1594)
    at ce.fn.init.trigger (jquery.min.js?v=3.7.1:2:70701)

Each time a radio button is toggled:

Uncaught TypeError: Cannot read properties of undefined (reading 'toLowerCase')
    at ce.fn.init.val (jquery.min.js?v=3.7.1:2:67761)
    at HTMLInputElement.<anonymous> (module_filter.update_status.js?v=10.2.1:118:26)
    at HTMLInputElement.dispatch (jquery.min.js?v=3.7.1:2:40035)
    at v.handle (jquery.min.js?v=3.7.1:2:38006)

I have confirmed this issue is not present in 4.0.1 on a fresh D10.2.1 instance with no other contrib modules except this one and its dependencies.

πŸ› Bug report
Status

Fixed

Version

4.0

Component

Code

Created by

πŸ‡΅πŸ‡ΉPortugal dubois

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @dubois
  • πŸ‡΅πŸ‡ΉPortugal dubois
  • πŸ‡΅πŸ‡ΉPortugal dubois

    This issue and 3413414 πŸ› t.nodeName is undefined Active possible have a common cause but different symptoms based on core version; see https://www.drupal.org/project/module_filter/issues/3413414#comment-1539... πŸ› t.nodeName is undefined Active .

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    I suspect the problem could be due to the big changes in πŸ› Remove the additional X clear button Fixed It will be useful to try and prove that, maybe by getting .diff from that MR and reverse-applying it.

    I know that the 'availbale updates' page does not have any test coverage. We are slowly improving, see πŸ“Œ Expand the phpunit test coverage for module_filter Active , but we could have avoided this breaking fix is we had more tests. That is something that would be good to work on too.

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    When using module_filter 4.0.1 the radio buttons do not work on core 10.2 and the t.nodeName error is shown.
    The new Module Filter 4.1.0 on core 10.1 works fine, the buttons efffect the page filtering and there is no console error.

    So the problem is with Core 10.2 and not the changes recently made in module filter.

  • πŸ‡ΊπŸ‡ΈUnited States wolffereast

    This was caused by a change in the `this` scope between the anonymous function and the lambda function. In the anonymous function `this` is the currently selected input, in the lambda function `this` is the attach abject. Adding the event argument and using the current target correctly pulls the currently selected input and allows the function to behave as it used to.

    I'm testing this in 10.1.7 prior to upgrading to 10.2

  • Status changed to RTBC 12 months ago
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 12 months ago
    3 pass, 1 fail
  • πŸ‡¨πŸ‡¦Canada leducdubleuet Chicoutimi QC

    I can confirm the patch in comment #6 fixes the issue on core 10.2.1, thank you very much!

  • πŸ‡ΊπŸ‡ΈUnited States wolffereast

    Am I reading the failure correctly that there is an issue in the test that is not related to this ticket? It looks like it's a quick update to the form class instantiation, though I don't have phpunit running yet to fire the test off locally. I'm not seeing a separate ticket, would that be helpful? This is similar to what I am seeing in some of the ConfigFormBase Tests in core.

    diff --git a/docroot/modules/contrib/module_filter/tests/src/Kernel/Form/ModuleFilterSettingsFormTest.php b/docroot/modules/contrib/module_filter/tests/src/Kernel/Form/ModuleFilterSettingsFormTest.php
    index f8d62f514..bd5ef7cf6 100644
    --- a/docroot/modules/contrib/module_filter/tests/src/Kernel/Form/ModuleFilterSettingsFormTest.php
    +++ b/docroot/modules/contrib/module_filter/tests/src/Kernel/Form/ModuleFilterSettingsFormTest.php
    @@ -27,7 +27,8 @@ protected function setUp(): void {
    parent::setUp();

    $this->moduleFilterSettingsForm = new ModuleFilterSettingsForm(
    - $this->container->get('config.factory')
    + $this->container->get('config.factory'),
    + $this->container->get('config.typed')
    );
    }

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    @wolffereast yes the test failure is unrelated and happens on the main branch. It started when d.o. moved from 10.1 to 10.2 testing. Yes please open a separate issue for that, as it needs fixing. Our gitlab-ci pipeline testing at 10.2 did not flag up the deprecation, that's why it got missed. We need to find out how it could have been detected earlier.

    Also note that the Updates page is not covered by any phpunit tests yet, so any change to that page will have to be checked manually, we can't reply on a green test pass.

  • πŸ‡ΊπŸ‡ΈUnited States wolffereast

    @jonathan1055 sounds good, I have a ticket up with a quick patch and green tests that should allow this patch to get through once it's on dev.

  • Status changed to Needs review 12 months ago
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 12 months ago
    Build Successful
  • πŸ‡ΊπŸ‡ΈUnited States wolffereast

    My first shot at a browser test for the updates page. This patch does not include the fix for this ticket and should fail in testUpdateStatusPageUpdateShowChange. I'm getting odd behavior while testing locally, checking the test bot to see if it agrees or is something is wrong on my side.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 12 months ago
    3 pass, 3 fail
  • πŸ‡ΊπŸ‡ΈUnited States wolffereast

    Fixed the coding standard errors and added the fix for this issue into the patch to see if the filter change will succeed. I'm getting a failure in testUpdateStatusPageTextFiltering that doesn't make sense to me.
    It mimics the behavior in the other tests by updating the text filter and looking to see if the Drupal Core listing has disappeared. Dumping the value of the field shows that the field value matches what was set, and testing that value in the browser behaves as expected. Assuming this fails in that method again I would appreciate it if someone with more experience in writing browser unit tests could take a moment to review.

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    Excellent work @wolffereast Thank you for adding this new test.
    One idea I had about why the test fails - when it filters out, you have $this->waitForNoText('Drupal core'); and the text still seems to be shown. I recently discovered on πŸ› Permissions filter test - waitForNoText() also needs waitForText() Fixed that we needed an extra waitForNoText() in 10.2 which was not required before. It seems that the refresh of the screen is slower somehow and the test processing starts looking immediately, and see the text before it has been filtered out.

    You can also add screengrabs in the javascript tests, by adding

    $this->createScreenshot(\Drupal::root() . '/sites/default/files/simpletest/screen1.png');
    

    Then you can see what the test is seeing. Hope that helps. Apologies if you already knew this.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 12 months ago
    4 pass, 2 fail
  • Status changed to Needs review 12 months ago
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 12 months ago
    5 pass
  • πŸ‡ΊπŸ‡ΈUnited States wolffereast

    @jonathan1055 Thanks for the review and especially the pointer about the screenshots, that was key to figuring this one out. As an aside, is there a way to dump console errors from a running test? That would have made finding the culprit here much easier.

    This was a bit of a rabbit hole. The issue ended up being an undeclared dependency on the once library killing the entire bind on the update status page. Adding that dependency allowed the winnow bind to fire and the tests started clearing. I added the dependency to all libraries that use it.

    The show test is correctly failing when I remove the original fix, reapplying the fix allows the test to complete with no issues.

    I moved the show test from its own method into the main test method, it looks like tests are combined like this in the core tests I was using as a reference.

    I'm clearly doing something wrong trying to push these updates to the merge request so I'm uploading another patch. The patch following naming conventions is the full issue patch, the txt file is the updates to the merge request.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Can the fix be added to the MR.

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    Before we fix anything, we must understand what caused it. I am not certain it was a core change between 10.1 and 10.2, it could very well have been one of the commits here, given that we did not previously have any test coverage for this page.

    I would like to go back through the commit log, and check this. It might make sense to fix the module code in a different way? Or at least it will give us confidence that we know what caused the problem. It may therefore allow a new 2.x release which can be compatible with D9 and is not broken, then we can gracefully turn off D9 support if we really want to.

  • πŸ‡ΊπŸ‡ΈUnited States wolffereast

    The issue was introduced in the coding standards update, the anonymous function to lambda swap was likely flagged in one of the automated tools.

    commit 0e023524be0d6efd063f68871cede70c23f27b47
    
        Issue #3338336: Javascript coding standards (uninstall, permissions, update status)

    I just spun up a simplytest.me instance on 9.5.11 with module filter 4.1.0 and the no test patch from #6 and have no issues using the filters on the update status page. This patch should fix the issues on 9 as well as 10, the patch in 17 tests cleanly against 9 and 10.

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    Excellent, thank you @wolffereast for the investigation and info. That is really helpful. I did suspect that it was the coding standards fix.

    @smustgrave I will create a MR for this as requested, since @wolffereast mentioned in #17 that their set-up is not quite working for that yet.

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    MR altered to just have the new correctly running test, as per #17. This will fail on 10.1 and 10.2.
    The test run on 9.5 will also fail (separately) on the machine_name filtering because we dont have a core patch/mr for that.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 11 months ago
    4 pass, 2 fail
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 11 months ago
    4 pass, 2 fail
  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    The screen grab was useful
    https://git.drupalcode.org/issue/module_filter-3413467/-/jobs/647451/art...
    It showed 'permission denied' because I had missed adding the extra permission (it was in the patch, but I only added the new test file)

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 11 months ago
    4 pass, 2 fail
  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    That's better, we now get down to screen grab2 which shows that filter is not working.
    Testing in the UI locally the title "Drupal Core" is still shown even when the core item is filtered out, so it will be interesting to see if this is different on the gitlab pipeline. Next commit will add the full fix as per patch #17

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 11 months ago
    5 pass
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 11 months ago
    5 pass
  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    Tests pass at 10.1 and 10.2
    The screen grabs show that the title 'Drupal Core' is also removed. That's good. I was testing locally on Gin theme (which is based on Claro) but the section titles were not removed. Luckily Claro does do it right.

    The pipeline failure at 9.5 is for the uninstall filtering because Core 9.5 has no patch for that.

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    Ha ha. The test at 9.5 has failures. The new updates report test failed because it detected 9.5 as 'unsupported' when testing the radio button, because the test was expecting "no result".

    That part of the new test could be altered so that it can run at 9.5. I think it is useful to be able to know that this module is still compatible with Core 9.5 until it comes time to do otherwise.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    I'm really not going to sweat it if 9.5.x tests are failing.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 11 months ago
    4 pass, 2 fail
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 11 months ago
    5 pass
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 11 months ago
    5 pass
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 11 months ago
    5 pass
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    If you're confident in the gitlab changes I can merge. Only concern is the gitlab file gets complex enough that it starts to break more.

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    Yes I am, but thanks for asking. It was a good question. We did break something on the gitlab templates project recently, it was reported 26 mins after the commit, and was reverted within 6 mins. Fjgarlin is quick on the spot. Plus I have been working quite a bit there and know my way round the .gitlab-ci.yml file, so I'm always here to sort it out. Also we now have proper versioning - see #3404175: Adopt (semver) versioning for gitlab_templates β†’ , which means the new default is to use a stable proven version, not the latest fixes immediately.

  • Status changed to Fixed 11 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave
  • πŸ‡¨πŸ‡¦Canada sseto

    It looks like it's working on most of my websites except one site. I'm getting this error in Console:

    jquery.min.js?v=3.7.1:2 Uncaught TypeError: Cannot read properties of undefined (reading 'toLowerCase')
    at ce.fn.init.val (jquery.min.js?v=3.7.1:2:67761)
    at HTMLInputElement. (module_filter.update_status.js?v=10.2.2:118:26)
    at HTMLInputElement.dispatch (jquery.min.js?v=3.7.1:2:40035)
    at v.handle (jquery.min.js?v=3.7.1:2:38006)

    Any idea what could be the issue?

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

Production build 0.71.5 2024