Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore visibility by default on copy/move in Filesystem #1724

Conversation

jnoordsij
Copy link
Contributor

With the current behavior, when creating a Filesystem instance with default visibility set (through passing the config); this will be enforced on any copyor move operation, where I would expect the default the be like the default adapter behavior (e.g. for AwsS3V3Adapter: keep the current visibility). This is due to the forced config merge taking place e.g. here:

$config = $this->config->extend($config);

The following is a quick illustration of the somewhat confusing behavior:

// File foo.txt with public visibility
$adapter = new AwsS3V3Adapter(...);
$adapter->copy('foo.txt', 'adapter-copy.txt', new Config([])); // adapter-copy.txt has the same visibility as foo.txt (public)
$filesystem = new Filesystem($adapter, ['visibility' => 'private']);
$filesystem->copy('foo.txt', 'filesystem-copy.txt'); // filesystem-copy.txt will have private visibility

I noticed this within the context of Laravel, which by default creates a Filesystem wrapper around any adapter before exposing it to the application; see also https://github.com/laravel/framework/blob/10.x/src/Illuminate/Filesystem/Filesystem.php.

This is marked as draft as I'd like to have some input about possible approaches and wether or not this is something that is open for behavioral changes. Some ideas about this:

  • the current implementation is slightly hackish, by force-setting visibility to null
  • alternatively, some method could be added to the Config class which unsets an option, which could be called here

@frankdejonge frankdejonge marked this pull request as ready for review November 7, 2023 08:09
@frankdejonge
Copy link
Member

Hi, I think this makes sense, I'll touch up the PR after merging before the release 👌 I've added a method to removeSettings on the main branch and will overlay this behaviour with the retain_visibility option that was merged yesterday. Thanks for contributing!

@frankdejonge frankdejonge merged commit 11571a7 into thephpleague:3.x Nov 7, 2023
5 checks passed
@jnoordsij
Copy link
Contributor Author

Thanks a lot for your quick response (both here and on the other PRs), much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants