The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
The overridden file system introduced in #3058063: Issue with setting permissions on a private bucket presents a regression in file upload widgets using ajax (e.g., using core's media library) and incorrectly implements the decorator pattern. There are alternative methods for forcing the bitmask on uploaded files that do not require overriding the file system.
Proximately, injecting the Settings service creates a serialization error ("Settings can not be serialized...") as the s3 file system service definition requires Settings. This could be avoided by using static methods on that service, however Drupal core is moving away from Settings in DI for this type of condition. See #2443351: Ensure that settings can't be serialized by not injecting them / replace stuff with container parameters..
Proposed resolution
Revert https://git.drupalcode.org/project/flysystem_s3/commit/f946cfb and use $settings['file_chmod_file'] or FlySystem's public_mask configuration option, instead.
Remaining tasks
Review/revert.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
n/a
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 3133318-2.patch | 646 bytes | spadxiii |
| revert.patch | 3.65 KB | bradjones1 |
Comments
Comment #2
spadxiii commentedHere is another patch that solves the serialization issue without reverting the whole patch.
Added DependencySerializationTrait, which solves the issue.
Comment #3
slasher13Had the same problem (file upload widgets using ajax).
Before
Applied patch #2 and file upload works!
Comment #4
bradjones1Adding
DependencySerializationTraitdoes address this, however I still maintain that the addition of the new service isn't necessary and is a good candidate for a revert, to keep the implementation here as simple as possible.Comment #5
bradjones1Comment #6
jordandukart commentedSo to move this issue along @bradjones1 what were you looking to see refined here or does there need to be discussion around how https://www.drupal.org/project/flysystem_s3/issues/3058063 can be accomplished without decorating? Definitely ran into a scenario with flysystem_s3 that hit https://www.drupal.org/project/drupal/issues/2896993 as well.
Comment #7
ericpughSince the new service created a new "major" bug, wouldn't it make the most sense to first revert that change, and then create a separate issue for handling private buckets with all the proposed resolutions from this thread?
Comment #8
jordandukart commentedI mean I'd like to see the above or at least some visibility / discussion to this. For the time being for the project I'm working on we are just nuking the decorator as we aren't using private buckets in our implementation as of current.
Comment #9
mangy.fox commentedAs an aside - the reversion patch conflicts with https://www.drupal.org/project/flysystem_s3/issues/3159928, which is required for D9 use. Patch #2 does apply and fixes the issue though.
Comment #10
3cwebdev commentedI spent days troubleshooting an error that was causing media assets to fail when being inserted via the media widget with the below watchdog error. Patch #2 resolved the issue and allowed us to be able to insert new media assets again.
Comment #11
leon kessler commentedI do agree with #4, decorating an entire service just to override some chmod settings seems like an unnecessary burden for this module.
However, setting
$settings['file_chmod_file']may not be a good solution for everyone, particularly those who are using a combination of different file systems. That's what the decorated service is able to do (only alter the chmod settings when it really needs to).I couldn't find anything on flysystem's
public_masksetting (no mention of it in either the flysystem module or on the flysystem docs/github). I did fine PortableVisibilityConverter, but this appears to be only on version 2/3 of Flysystem.I would agree to revert the original change, but as this has now been in the module for almost 2 years now. I think it may be safer to keep things how they are and incorporate patch from #2.
Comment #13
leon kessler commentedPatch from #2 has now been merged in, this should now prevent errors when used with the media module.
However I will leave this issue open, in case anyone fancies resolving this in a way that doesn't require overriding the file system.
Comment #14
bradjones1I'm going to be bold and close this because it has run its course, however the decorator pattern here is plain incorrect. Decorators should not extend the class they are decorating, but rather wrap them. I ran into this in a rather insidious way, today, which I will document on a new issue.
To the extent something got fixed here I think also setting the status to Fixed will generate proper issue credit.
For the record I still don't like this really at all, but to Leon's point, this is part of the module now for quite a while and people depend on this functionality (e.g., me) even if the actual decorator implementation is incorrect. That, we can fix.