Single directory component CSS asset library not picked up in admin theme immediately after module install without cache clear

Created on 10 July 2024, about 2 months ago
Updated 12 August 2024, 27 days ago

Problem/Motivation

After installing a new module with one or more single directory components that are immediately used and visible on the page, the CSS that comes with the SDC is not included in the admin theme.

The core issue here is the library discovery cache needs to be cleared on install. For more details see #7.

Steps to reproduce

I've created a minimal reproducible module to illustrate the issue here: https://git.drupalcode.org/sandbox/m4olivei-3460588. Clone the project an put it in your modules folder.

  • Install Drupal from scratch. eg. drush si -y standard
  • Using the UI, navigate to Admin > Extend
  • Enable the SDC Cache Issue module

Expected result

A hotpink bar should appear at the top of every page with the text "beep boop foo" aligned right. The bar being hotpink and aligned right are indicators that the componet CSS was included on the page.

Actual result

The bar is included on the page but is unstyled. The expected CSS is not included on the page.

Also note that if you navigate to the front-end theme (assuming it's different from the admin theme), the component is styled and does include the expected CSS.

In order for the expected styles to be included on the page, a cache clear is required.

Proposed resolution

Adjust the module_installer service to clear the library discovery cache on install.

Remaining tasks

User interface changes

None.

API changes

The module_installer service will now clear the library discovery cache on install. This allows assets for SDC to be correctly discovered and available immediately after install, without necessitating a cache clear.

Data model changes

None.

Release notes snippet

None.

🐛 Bug report
Status

Fixed

Version

10.3

Component
single-directory components 

Last updated 4 days ago

Created by

🇨🇦Canada m4olivei Grimsby, ON

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

Merge Requests

Comments & Activities

  • Issue created by @m4olivei
  • 🇬🇧United Kingdom catch

    Install Drupal from scratch. eg. drush si -y standard
    Using the UI, navigate to Admin > Extend
    Enable the SDC Cache Issue module

    What happens if the module is installed via drush instead of the UI?

    I'm wondering if this is a race condition somehow between the sdc being rendered immediately after a form submission vs. the libraries being discovered - i.e. is the first SDC somehow rendered before the library exists as such on the system.

    However I'm not finding in the code where the component libraries are actually attached (as in #attached) by component rendering to see how that happens.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    @catch If it helps, that happens AFAIK in ComponentNodeVisitor::leaveNode, adding attach_library to the compiled twig.

  • 🇨🇦Canada m4olivei Grimsby, ON

    What happens if the module is installed via drush instead of the UI?

    It depends. The following produces the expected result:

    $ ddev drush si -y minimal
    $ ddev drush en -y sdc_cache_issue
    $ ddev drush uli

    However the following reproduces the bug noted here:

    $ ddev drush si -y minimal
    $ ddev drush uli
    [use login link to view the site]
    $ ddev drush en -y sdc_cache_issue

    So it would appear that there is some core code managing the front-end theme cache when the module gets turned on, but not the admin theme, and so the admin theme cache as it concerns SDC assets is stale.

  • 🇷🇸Serbia finnsky

    Setting prio to major because it affects Navigation module after install.

  • 🇬🇧United Kingdom catch

    Thanks for the testing module. Was able to reproduce and track down.

    Module install doesn't clear the library discovery cache - in general module install tries to be selective about which caches it clears to avoid completely cold starts when simple modules are installed.

    Usually, this is fine for the library discovery cache because library discovery is in a cache collector: there is no cache key for the newly installed module yet, it will be treated as a cache miss, and then libraries from the module get discovered.

    However, single directory components are auto-discovered by library discovery, and not only that, they're added to the library definitions from 'core', not from the extensions that they're discovered in. Because 'core' has already been discovered, this logic just doesn't run until the cache is cleared.

    I wondered if we should try to discover sdcs by extension and add them to the libraries for that extension, but because plugin discovery is global, not by extension, that wouldn't really work without a huge refactor. Also plugin discovery has its own alter hooks, I don't know if we allow that for sdc but if we did it'd get even more complex. So it's easiest just to clear the library discovery cache.

    I think there is probably an existing pre-SDC bug where hook_library_info_alter() wouldn't get picked up too, however because this would only be a bug when a module is enabled, that alters the library definitions of a library that's from a module that's already installed, and it'd fix itself after a cache clear, this may never have been noticed before.

    Should probably add tests - ideally we can piggy back on an existing sdc functional or kernel test if there is one, should be possible just to call Drupal::service('library_discovery')->getLibraryByName() and see if it returns the library we'll generate for a component in a newly installed module - or something close to that.

  • Merge request !8862Clear the library discovery cache on module install. → (Closed) created by catch
  • Status changed to Needs review about 2 months ago
  • 🇬🇧United Kingdom catch

    Tried to add some test coverage to the asset kernel test but failed - couldn't get it to fail with the code in HEAD so a bit of a useless test.

  • 🇬🇧United Kingdom catch

    Found 📌 LibraryDiscoveryCollector caches unnecessarily Needs review while looking at this.

  • Status changed to RTBC about 2 months ago
  • 🇨🇦Canada m4olivei Grimsby, ON

    Looks great!

    Rationale in #7 is sound and vibes with the time that I spent in xdebug around the issue.

    Following the steps to reproduce in the issue summary, I not get the actual result, eg. immediately after install, the expected CSS assets from the SDC in the sdc_cache_isse module are served to the client.

    I've updated the issue summary to better describe the actual issue, letting #7 do the work, as it's so well written.

    RTBC for me. Happy to write up a CR if necessary.

  • 🇬🇧United Kingdom catch

    This doesn't need a CR since it's just a bug fix.

    We might want test coverage, but my attempt to add that in a kernel test for hook_library_info_alter() failed. It would probably be possible to set up a kernel or functional test with a test module that provides an SDC, similar to the one provided for manual testing here, but that's quite a lot of test infrastructure for a one line cache clear.

    Overall I think it would be a good idea if we could revert the relationship here - i.e. the library.discovery service should be able to tag itself as needing a cache clear on module install, and so should plugin.cache_clearer, so that we can get rid of lines like:\Drupal::getContainer()->get('plugin.cache_clearer')->clearCachedDefinitions(); altogether from module install and just have a single line that loops over the tagged services and calls an interface method. That would also allow us to replace similar code in drupal_flush_all_caches().

    Went ahead and opened 📌 Add service tag(s) for cache clearing Active .

    • nod_ committed 56949be1 on 10.3.x
      Issue #3460598 by catch, m4olivei, penyaskito, finnsky: Single directory...
    • nod_ committed ce7628b4 on 10.4.x
      Issue #3460598 by catch, m4olivei, penyaskito, finnsky: Single directory...
    • nod_ committed d1b82290 on 11.0.x
      Issue #3460598 by catch, m4olivei, penyaskito, finnsky: Single directory...
    • nod_ committed 2cf5bf32 on 11.x
      Issue #3460598 by catch, m4olivei, penyaskito, finnsky: Single directory...
  • Status changed to Fixed about 1 month ago
  • 🇫🇷France nod_ Lille

    Committed and pushed 2cf5bf3270 to 11.x and d1b82290b2 to 11.0.x and ce7628b484 to 10.4.x and 56949be14d to 10.3.x. Thanks!

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

Production build 0.71.5 2024