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

Copy Files From External Storage To Local Storage for Import #149

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

wattnpapa
Copy link
Contributor

Hi i had some promblems loading Snapshots gzipped from S3.
So i implementent this solution where i first copy the file to local tmp Folder.

This solution works for me quite well

@wattnpapa
Copy link
Contributor Author

@freekmurze anything i have to do fo a merge?

src/Snapshot.php Outdated Show resolved Hide resolved
@wattnpapa wattnpapa force-pushed the loadLocalTemp branch 3 times, most recently from ea3a2ae to 9c96295 Compare December 30, 2024 11:22
@freekmurze
Copy link
Member

Could you rebase against main to fix the conflicts?

@wattnpapa
Copy link
Contributor Author

@freekmurze I think now it looks better ?

src/Snapshot.php Outdated Show resolved Hide resolved
Copy link
Member

@freekmurze freekmurze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could a test be added that makes sure that this code works?

src/Snapshot.php Outdated Show resolved Hide resolved
src/Snapshot.php Outdated Show resolved Hide resolved
@wattnpapa wattnpapa force-pushed the loadLocalTemp branch 2 times, most recently from 66f841a to 68adf55 Compare December 30, 2024 15:30
src/Snapshot.php Outdated Show resolved Hide resolved
@xHeaven
Copy link
Contributor

xHeaven commented Dec 30, 2024

Please don't forget adding the proper types in your method definitions (return and parameter).

@xHeaven
Copy link
Contributor

xHeaven commented Dec 30, 2024

Everywhere where you specified mixed $stream, you can actually use the resource type instead of mixed.

@wattnpapa
Copy link
Contributor Author

wattnpapa commented Dec 31, 2024

@xHeaven Done i think
i found no way to use resource i always get errors
resource is not a valid type hint in PHP for method signatures; it’s a reserved type that can’t be used directly.

@wattnpapa
Copy link
Contributor Author

@freekmurze anything to do for me?

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.

3 participants