Support new lazy asset collection optimizers & deprecate the serve_* options

On code review:

1. https://git.drupalcode.org/project/flysystem/-/merge_requests/26/diffs#2...
The File System service is already available on the parent class, instead of loading the service again statically, use $this->fileSystem instead.

2. https://git.drupalcode.org/project/flysystem/-/merge_requests/26/diffs#2...
Let's put the original "try/catch" from src/Asset/CssCollectionOptimizer.php in here as well

3. https://git.drupalcode.org/project/flysystem/-/merge_requests/26/diffs#6...
The File System service is already available on the parent class, instead of loading the service again statically, use $this->fileSystem instead.

2. https://git.drupalcode.org/project/flysystem/-/merge_requests/26/diffs#6...
Let's put the original "try/catch" from src/Asset/JsCollectionOptimizer.php in here as well

Note: The Logger called in the "catch" is not called legally, this is a static anonymous function, and as such does not have access to "$this" as part of its scope. The logger statement needs to be as follows:

\Drupal::service('logger.factory')->get('flysystem')->error($e->getMessage());

The logger statements for src/Asset/JsCollectionOptimizer.php and src/Asset/CssCollectionOptimizer.php will be patched in a separate MR.

It looks like I copied the wrong code, but this is the core CssCollectionOptimizerLazy::deleteAll implementation:

/**
   * {@inheritdoc}
   */
  public function deleteAll() {
    $this->state->delete('drupal_css_cache_files');
    $this->fileSystem->deleteRecursive('assets://css');
  }

I'll take that and add a try/catch around the deleteRecursive method call and do the same for JsCollectionOptimizerLazy.

Still looking at this one, and also looking at the original implementation.

Couple of things:

===The README documentation for configuring the scheme===

https://git.drupalcode.org/project/flysystem/-/blob/2.1.x/README.md#conf...

specifically the following lines:

     'serve_js' => TRUE, // Serve Javascript or CSS via this stream wrapper.
     'serve_css' => TRUE, // This is useful for adapters that function as CDNs like the S3 adapter.

This is managed in the current module through the implementation of the SchemeExtensionTrait, which your proposed patch ignores.

This doesn't really have any effect if you're using the local file system for storing optimized CSS and JS, but it DOES affect the use of remote file systems and object stores such as S3 for storing and serving optimized CSS / JS.

(Personally I don't like caching optimized CSS and JS on the remote object store, my experience is that it has been problematic at best, and the whole override of the deleteAll() method in the JsCollectionOptimizer / CssCollectionOptimizer for this module has been no exception.)

So...if we don't care about serving optimized CSS and JS from a remote file store and streaming those files through Flysystem, then overriding the core JsCollectionOptimizer / CssCollectionOptimizer,, and the implementation of the SchemeExtensionTrait is wholly unnecessary and can be removed.

BUT, if we do care about this, then your patch will need some work.

I'm fine eliminating it (and it takes care of some of the open issues in the issue queue if we do), but that will likely break some user implementations that exist currently.

Since Drupal 10 now has the assets:// stream wrapper → built in, I think the serve_js and serve_css options have become obsolete. My suggestion:

• Leave the options in 2.1.x but log a deprecation message if they are still used while the assets:// stream wrapper exists. Change this issue to target 2.1.x.
• Remove the options in 3.x. We'll create a separate issue for this, targeting 3.x.

I am on board with your suggestion. One thing to keep in mind:

The Assets StreamWrapper was introduced in 10.1, so if we're going to deprecate using `serve_js` and `serve_css` where the Assets StreamWrapper is available, we'll need to keep support for `serve_js` and `serve_css` in place for the duration of version 10.0 support.

Additional Information related to this issue:

* https://www.drupal.org/node/2888767 →
* https://github.com/skpr/image-nginx/pull/16/files

A key note: Drupal Core's AssetDumper (Drupal\Core\Asset\AssetDumper) also received a new interface and the API has changed. This module includes an extension of Core's AssetDumper, similarly to the way this module extended `JsCollectionOptimizer` and `CssCollectionOptimizer`, as well as loads all three override classes as the core service provider here:

https://git.drupalcode.org/project/flysystem/-/blob/2.1.x/src/FlysystemS...

Why don’t we set the minimum core version for 3.0.x to 10.1 then? I don’t see why that would be a problem, especially since 2.1.x supports 10.0.

We still need to deprecate the serve_js and serve_css options here.

I'd like to get this working for 10.1, currently it kills the site and it's then not (easily) possible to uninstall the module due to:

TypeError: Drupal\Core\Asset\CssCollectionOptimizer::__construct(): Argument #3 ($dumper) must be of type Drupal\Core\Asset\AssetDumperInterface, Drupal\Core\Theme\ThemeManager given, called in /Users/adamclarey/Sites/iyield/liqquid/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 262 in Drupal\Core\Asset\CssCollectionOptimizer->__construct() (line 69 of /Users/adamclarey/Sites/iyield/liqquid/web/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php).

I had a look through the code but I'm not familiar enough with it to find a quick fix

I've just hit the same issue as @Adam Clarey at #23. This is needed for client work too so I may take a run at it at well.

@Adam Clarey, I've just used cwegans/composer-patches and gitlabs handy "make this MR a patch" option to patch the current RC. The error above that we are seeing has now gone away for me.

I don't know if this is ready, but it's a lot further along that it was so I consider this to be RTBC. I would like more people to test the patch though. For reference, this is the patch I'm applying.

https://git.drupalcode.org/project/flysystem/-/merge_requests/26.patch

A note for others. As I understand it any new commits on that MR will appear in the patch so this should not be considered a snapshot in time.

One comment: Implementing this is an API change, so ideally we should create a new version, rather than implement this on version 2.1. The reason for this is that it could cause some of the contrib "submodules" in the Flysystem module ecosystem to break due to the API change.

This should be, at a minimum, version 2.2, with the constraint that it is Drupal 10.1 or higher.

I'm also OK with making this a 3.0 version, and moving the current 3.0 dev branch (which has gone stale) to a 4.0 version (intended for adoption of the new version of the Flysystem library.

Fixed, merged into newly created 2.2.x branch.

This is broken in upcoming Drupal 10.2 https://www.drupal.org/project/drupal/issues/3301573 📌 Remove the aggregate stale file threshold and state entry Fixed
They have removed the state property https://git.drupalcode.org/project/drupal/-/commit/405f8b33a7d0643ba39bf...

I'm getting

PHP Fatal error: Uncaught Error: Call to a member function delete() on null in

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

I have the same problem as @Robin.Houtevelts in comment #30.

This is the error I get once I have removed the dependency with state

───────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: 3376399.patch
───────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ diff --git a/src/Asset/JsCollectionOptimizerLazy.php b/src/Asset/JsCollectionOptimizerLazy.php
   2   │ index 361bad0..33ed3c4 100644
   3   │ --- a/src/Asset/JsCollectionOptimizerLazy.php
   4   │ +++ b/src/Asset/JsCollectionOptimizerLazy.php
   5   │ @@ -16,7 +16,6 @@ class JsCollectionOptimizerLazy extends DrupalJsCollectionOptimizerLazy {
   6   │     * {@inheritdoc}
   7   │     */
   8   │    public function deleteAll() {
   9   │ -    $this->state->delete('system.js_cache_files');
  10   │      try {
  11   │        $this->fileSystem->deleteRecursive($this->getSchemeForExtension('js') . '://js');
  12   │      } catch (FileException $fileException) {

TypeError: Drupal\Core\Asset\JsCollectionOptimizer::__construct(): Argument #3 ($dumper) must be of type Drupal\Core\Asset\AssetDumperI
  nterface, Drupal\Core\Theme\ThemeManager given in Drupal\Core\Asset\JsCollectionOptimizer->__construct() (line 69 of /var/www/cms/docroot/core/lib/Drupal/Core/Asset/JsCollectionO
  ptimizer.php) #0 [internal function]: Drupal\Core\Asset\JsCollectionOptimizer->__construct(Object(Drupal\Core\Asset\JsCollectionGrouper), Object(Drupal\Core\Asset\JsOptimizer), O
  bject(Drupal\Core\Theme\ThemeManager), Object(Drupal\Core\Asset\LibraryDependencyResolver), Object(Symfony\Component\HttpFoundation\RequestStack), Object(Drupal\flysystem_s3\File
  \FlysystemS3FileSystem), Object(Drupal\Core\Config\ConfigFactory), Object(Drupal\Core\File\FileUrlGenerator), Object(Drupal\Component\Datetime\Time), Object(Drupal\language\Confi
  gurableLanguageManager))

@luigisa are you definitely on the latest 2.2.x? This error does not happen for me with b80a666.

I created 🐛 CssCollectionOptimizerLazy and JsCollectionOptimizerLazy use removed inherited member state Needs review to delete the state references from JsCollectionOptimizerLazy and CssCollectionOptimizerLazy as this issue is closed.

@Eli-T
It was a mistake, because there was a custom code in the project that caused the problem.
Now I'm ok with the patch you sent.