Problem/Motivation

In D8 HEAD, it's impossible to list or delete all aggregate assets using an API. One must manipulate private data: State entries. But contrib-provided overrides of the default asset.(css|js).collection_optimizer services may choose not to use State at all, so this is a gap in the asset API.

It's also a blocker for #2258313: Add license information to aggregated assets (specifically see #17 there).

Proposed resolution

Add this to AssetCollectionOptimizerInterface:

  /**
   * Returns all optimized asset collections assets.
   *
   * @return string[]
   *   URIs for all optimized asset collection assets.
   */
  public function getAll();

  /**
   * Deletes all optimized asset collections assets.
   */
  public function deleteAll();

Remaining tasks

Review.

User interface changes

None.

API changes

  • API addition: AssetCollectionOptimizerInterface::getAll()
  • API addition: AssetCollectionOptimizerInterface::deleteAll()
  • API removal: drupal_clear_css_cache(), drupal_clear_js_cache() — this should use \Drupal::service('asset.(css|js).collection_optimizer')->deleteAll() instead now.
CommentFileSizeAuthor
#12 interdiff.txt1.3 KBWim Leers
#12 asset_collection_optimizer_listing_deleting-2307419-12.patch17.69 KBWim Leers
#9 interdiff.txt1.25 KBWim Leers
#9 asset_collection_optimizer_listing_deleting-2307419-9.patch17.52 KBWim Leers
#7 interdiff.txt3.13 KBWim Leers
#7 asset_collection_optimizer_listing_deleting-2307419-7.patch17.52 KBWim Leers
#3 interdiff.txt2.44 KBWim Leers
#3 asset_collection_optimizer_listing_deleting-2307419-3.patch16.48 KBWim Leers
#1 asset_collection_optimizer_listing_deleting-2307419-1.patch14.41 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue summary: View changes
Status: Active » Needs review
FileSize
asset_collection_optimizer_listing_deleting-2307419-1.patch14.41 KB

Status: Needs review » Needs work

The last submitted patch, 1: asset_collection_optimizer_listing_deleting-2307419-1.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs work » Needs review
FileSize
asset_collection_optimizer_listing_deleting-2307419-3.patch16.48 KB
interdiff.txt2.44 KB
moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented 
+function locale_cache_flush() {
+  \Drupal::state()->delete('system.javascript_parsed');
+}

This code moved from drupal_clear_js_cache() to locale module. Is it really only needed when locale is enabled?

Status: Needs review » Needs work

The last submitted patch, 3: asset_collection_optimizer_listing_deleting-2307419-3.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#4: yes, it is.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs work » Needs review
FileSize
asset_collection_optimizer_listing_deleting-2307419-7.patch17.52 KB
interdiff.txt3.13 KB

Should be green now.

Status: Needs review » Needs work

The last submitted patch, 7: asset_collection_optimizer_listing_deleting-2307419-7.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs work » Needs review
FileSize
asset_collection_optimizer_listing_deleting-2307419-9.patch17.52 KB
interdiff.txt1.25 KB

Stupid me. This will be green.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

OK, RTBC then. I reviewed this earlier.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record 
+++ b/core/includes/common.inc
@@ -1327,28 +1327,6 @@ function drupal_pre_render_styles($elements) {
 /**
- * Deletes old cached CSS files.
- */
-function drupal_clear_css_cache() {
-  \Drupal::state()->delete('drupal_css_cache_files');
-  file_scan_directory('public://css', '/.*/', array('callback' => 'drupal_delete_file_if_stale'));
-}
-
-/**
- * Deletes files modified more than a set time ago.
- *
- * Callback for file_scan_directory() within:
- * - drupal_clear_css_cache()
- * - drupal_clear_js_cache()
- */
-function drupal_delete_file_if_stale($uri) {
-  // Default stale file threshold is 30 days.
-  if (REQUEST_TIME - filemtime($uri) > \Drupal::config('system.performance')->get('stale_file_threshold')) {
-    file_unmanaged_delete($uri);
-  }
-}

@@ -2462,15 +2440,6 @@ function drupal_attach_tabledrag(&$element, array $options) {
- * Deletes old cached JavaScript files and variables.
- */
-function drupal_clear_js_cache() {
-  \Drupal::state()->delete('system.javascript_parsed');
-  \Drupal::state()->delete('system.js_cache_files');
-  file_scan_directory('public://js', '/.*/', array('callback' => 'drupal_delete_file_if_stale'));
-}
-

These are all API changes - I'd prefer to deprecate. See:

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record +Drupalaton 2014
FileSize
asset_collection_optimizer_listing_deleting-2307419-12.patch17.69 KB
interdiff.txt1.3 KB

Done.

Change record created: https://www.drupal.org/node/2317841.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed d7d5872 and pushed to 8.0.x. Thanks!

  • alexpott committed d7d5872 on 
    Issue #2307419 by Wim Leers: AssetCollectionOptimizerInterface should...

Status: Fixed » Closed (fixed)

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

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: -front-end performance +frontend performance

changing to use the more common tag, so the less common one can be deleted, so it does not show up in the auto complete and confuse people.