Problem/Motivation

We noticed on a project that downloading files from s3 through the stream wrapper no longer worked, and tracked it down to a change in the flysystem s3 dependency. See https://github.com/thephpleague/flysystem-aws-s3-v3/issues/218, which is think is about the same change between 1.0.25 and 1.0.28.

Steps to reproduce

Proposed resolution

Workaround: add a conflict on > 1.0.25 until this is resolved/reverted.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 3172969-force-skipping-stream-http-option-9.patch1.42 KBimiksu
#7 3172969-force-skipping-stream-http-option-7.patch1.08 KBimiksu

Issue fork flysystem_s3-3172969

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

Berdir created an issue. See original summary.

pawel_r’s picture

pawel_r commented

Thank you, downgrade league/flysystem-aws-s3-v3 to 1.0.25 solved my issues. ImageStyle could not create derivate for "random", not corrupted files.

bradjones1’s picture

bradjones1
Primary language English
commented
Priority: Normal » Critical

iMiksu made their first commit to this issue’s fork.

iMiksu opened merge request !4

imiksu’s picture

imiksu
Location Finland
commented
Status: Active » Reviewed & tested by the community

Merge request ready for peer-review.

(FYI this is my first merge request in Drupal, hopefully I'm doing it right)

imiksu’s picture

imiksu
Location Finland
commented
StatusFileSize
new 3172969-force-skipping-stream-http-option-7.patch1.08 KB

Gotta provide the old way patch too, since I really need to patch this against 2.0.0-rc1.

By the way: How I generate text patch/diff files from a merge request?

imiksu’s picture

imiksu
Location Finland
commented
Status: Reviewed & tested by the community » Needs review

Whoops, I accidentally put wrong state for the issue. Putting "Needs review".

imiksu’s picture

imiksu
Location Finland
commented
StatusFileSize
new 3172969-force-skipping-stream-http-option-9.patch1.42 KB

After good sleep, I also realized that fast workaround for this is to configure your S3 settings by adding this HTTP option (it should come down eventually to this problematic part of library source code), but I have not tested this.

Maybe someone can confirm that adding @http section would help:

    'config' => [
      'region' => 'eu-west-1',
      'bucket' => '<your-bucket>',
      'key' => '<your-key>',
      'secret' => '<your-secret>',
      'prefix' => 'files',
      'protocol' => 'https',
      'public' => TRUE,
      'options' => [
        'ACL' => 'public-read',
        '@http' => [
          'stream' => false,
        ]
      ],
    ],

I still think this patch #7 should be added because it is required configuration in Drupal and it follows the original behavior. Hence we don't want to require users to configure to disable HTTP streams.

Version 1.0.26 is problematic, because it doesn't allow you to use options for specifying if stream should be disabled, so I thought we should block the version completely.

EDIT: Disregard this. I realised after reading more closely 1.0.26 code, that actually @http options will overwrite hardcoded stream enabling, so no problem with 1.0.26 either.

Do not apply this patch, use #7 instead.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
leon kessler’s picture

leon kessler commented
Version: 2.0.0-rc1 » 2.0.x-dev

I think stream reads just applies if you have not set 'public' => TRUE, in your config? Am I correct?

Can anyone confirm this patch is working ok? I can test this myself, but on my project we use 'public' => TRUE, so I don't have experience running it in production.

Also, there is no docblock on the new function __construct in the patch. But maybe this is okay, as there isn't one on the flysystem-aws-s3-v3 library either. If we add one to ours we are documenting external code.

sheise’s picture

sheise commented

The patch from #7 fixed the problem for me. Thanks.

  • iMiksu authored 8e0c9c8 on 2.0.x 
    Issue #3172969 by iMiksu: file stat/size/download broken with flysystem-...
leon kessler’s picture

leon kessler commented
Status: Needs review » Fixed

Tested the patch and confirms this fixes the issue.

Thanks everyone.

Status: Fixed » Closed (fixed)

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

Eli-T made their first commit to this issue’s fork.

Eli-T closed merge request !4