S3fs StreamWrapper: Problem with public files delivery through Drupal with root folder

Very quick glancing the code:
Probably a bug.

I do not recall ever designed a concept for the Bucket to tell StreamWrapper that there was a “path prefix”. The design is *suppose* to be that the StreamWrapper plugins are oblivious to how the bucket stores the files.

https://git.drupalcode.org/project/s3fs/-/blob/ef7d09953f5380b83ea1aa197... probably should be $target not $s3_key

Super nice of you Conrad! 🙏

Just pasting here the comment I intended to post before you beat me to the punch 😅
 

Once again, I would definitely like to run this one by you Conrad (@cmlara), when you have some time:

I have no clue whether this is the correct way to fix this...

This patch is really a quick fix, which works in my current setup, but could potentially break in many other different types of setups I haven't had the time or chance to test (for example, bucket with Cname, ACL config, etc...).

These changes seem to fix the issue in my case and the files are delivered properly:
The generated path doesn't change, for example: /s3/files/public/s3fs-public/2024-07/text-example.txt
But the correct file is returned properly.

The code changes could maybe be further optimized, but before going any further and spending more time on this I wanted to know if I was looking at the right piece of code.

If I am mistaken and there is a different way this could be achieved and fixed, perhaps through configuration or Nginx rules/config, I would greatly appreciate if you could please give me some pointers and guidance.

Once again, I'm setting this in Needs review so you could perhaps let me know at a first glance if there is anything I've changed that could cause other potential issues.

Based on your advice, feedback, suggestions and recommendations, I would definitely be very happy to help testing, or making any change to the merge request, if that could help you in any way.

Otherwise, feel free to let me know if you have any questions or would need more information on the bug itself, the ticket in general, or the merge request, I would surely try answering as soon as possible.
Thanks very much in advance!

Re #3:

Thanks a lot!

https://git.drupalcode.org/project/s3fs/-/blob/ef7d09953f5380b83ea1aa197...
probably should be $target not $s3_key

I'll take a look at this, give it a quick test and report back!

Thanks again!

You rock Conrad! 🎉

Once again, thank you very much for your great help and prompt reply!

I made the changes in module's files directly for testing on the target development environment and it worked great!

The generated URL has changed and is now:
/s3/files/public/2024-07/text-example.txt

without the root folder in the path anymore, just as suggested in the issue summary and based on the tests I initially did, modifying the URL in the browser.

I've updated the merge request accordingly and will be using this as the patch instead, until you are able to review the MR and potentially consider getting this into the dev branch.

Feel free to let me know if there is anything else I could do to help getting this rolled in module's code base, or whether you would need any additional information, or more changes to the made to the MR, I would defintely be very happy to help.

Thanks again! 🙏

Looks good to me.

I would like there to be a test on this as it appears I missed the scenario, would be nice to avoid a regression in future.

I believe something like the following added to S3FsCustomStreamWrapperPluginTest::providerGetExternalUrl() would proof this (though with the current state of code it will be hard for the CI lab to validate this)

      'Use Drupal Delivery with root_folder' => [
        [
          'use_drupal' => TRUE,
          'root_folder' => 'folder_prefix',        
        ],
        's3://test/object.txt',
        TRUE,
        '/s3/files/s3/test/object.txt',
      ],

(Not actually tested)

Thanks again Conrad for the prompt reply and detailed instructions on the test to add, it really helped! 🙂

The corresponding changes have been made to the merge request, which is ready to be reviewed once again.

I've placed the provided piece of code right after the other Drupal Delivery related piece.

Feel free to let me know if there is anything else you would see that would need testing or changing in order for the merge request to be added to the dev branch, I would certainly try answering as soon as possible.
Thanks in advance!

Downloaded to local lab to confirm the updated test fails without the fix as the MR did not allow running the fail-only test job. Test does indeed fail as expected.

Merging to mainline. Thank you!

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