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

Add "after" callback for the Transfer class #2723

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

madalindk
Copy link

@madalindk madalindk commented Jul 9, 2023

Fixes #2725

Description of changes:
Added possibility to execute a callback function after a transfer is fulfilled when using the Transfer class. Implemented similarly to the way the 'before' callback works.

While using the transfer class, particularly the upload methods, I could see that there is a "before" callback I can use but there is no "after" callback. I needed such an "after" callback to cleanup the files as they are getting uploaded. Because of this I have implemented an "after" callback similar to the "before" callback which uses the fulfill callback option. It now can be used as follows:

$s3Client->uploadDirectory($path, $bucket, null, [
    'after' => function(\Aws\Result $result, int $index, \GuzzleHttp\Promise\Promise $aggregatePromise) {
        // Do something with $result or check $aggregatePromise
    }
]);

I think such a feature might be helpful to others in the future as well. I have created this PR to see if you also agree having an "after" callback would be a good idea and the code is just a fast copy paste from the "before" callback. If there is agreement that this would be a good idea, I can/will write some more tests and proper documentation about this new callback.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@stobrien89
Copy link
Member

stobrien89 commented Jul 11, 2023

Hi @Mastertrap21,

I think we'd be open to accepting something like this. Seems like a perfectly reasonable way to do this and I'd imagine others may like the option to perform a task after a transfer operation.

We generally like to have a feature request (in the form of an issue) open before we decide to merge a community requested feature, so if you could do that (feel free to copy over some of the description from this PR and link it as well), it'd be greatly appreciated. Some people pay more attention to issues on our repository and we like to give others the chance upvote or chime in about these sorts of things.

@madalindk
Copy link
Author

Hi @stobrien89,

Thank you for your quick response, openness and guidance regarding this feature, it's always nice to contribute to a well maintained project.

I have now opened a feature request in the form of an issue here: #2725
I will write some specific tests for the new callback tomorrow when I get some extra time.
I'm not sure about the existing checks, I can see that it is failing because of the added nextrelease changelog JSON. Is that something I should look into?

@madalindk madalindk force-pushed the feat-transfer-afterCallback branch from 14bbdfa to 868a96a Compare July 1, 2024 20:52
Copy link
Member

@stobrien89 stobrien89 left a comment

Choose a reason for hiding this comment

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

@madalindk,

LGTM, just need to move that changelog entry file to .changes/nextrelease/. from the SDK base directory. Looks like it ended up in some old build artifacts.

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