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

  • Debug to understand the core issue. See #7
  • Adjust the module_installer service to clear the library discovery cache on install.. See MR

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.

Issue fork drupal-3460598

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:

About issue forks

Comments

m4olivei created an issue. See original summary.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Title: Single directory component CSS asset library not picked up in Claro immediately after module install without cache clear » Single directory component CSS asset library not picked up in admin theme immediately after module install without cache clear
catch’s picture

catch
he/him
Primary language English
commented

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.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

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

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Issue summary: View changes

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.

finnsky’s picture

finnsky commented
Priority: Normal » Major

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

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

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.

catch opened merge request !8862

catch’s picture

catch
he/him
Primary language English
commented

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.

catch’s picture

catch
he/him
Primary language English
commented

Found #3462871: Deprecate LibraryDiscovery and use LibraryDiscoveryCollector instead while looking at this.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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 get the actual result, eg. immediately after install, the expected CSS assets from the SDC in the sdc_cache_issue 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. Although my read on the policy seems to indicate its not necessary.

catch’s picture

catch
he/him
Primary language English
commented

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 #3463657: Add service tag(s) for cache clearing.

  • 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...

nod_ closed merge request !8862

nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

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!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3464388: SDC *.component.yml metadata is cached aggressively, gets in the way of component development

Follow-up/related: #3464388: SDC *.component.yml metadata is cached aggressively, gets in the way of component development.

Status: Fixed » Closed (fixed)

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