- 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 moduleWhat 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
, addingattach_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.
- Status changed to Needs review
4 months ago 11:26am 21 July 2024 - 🇬🇧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
4 months ago 12:55am 25 July 2024 - 🇨🇦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 .
- Status changed to Fixed
4 months ago 1:12am 28 July 2024 - 🇫🇷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!
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Follow-up/related: 📌 SDC *.component.yml metadata is cached aggressively, gets in the way of component development Active .
Automatically closed - issue fixed for 2 weeks with no activity.