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

Allow changing boundary string in upload:multipartFormData callback #2705

Closed

Conversation

ondrejstocek
Copy link

Goals ⚽

Allow changing boundary string in upload:multipartFormData callback

Implementation Details 🚧

Changes MultipartFormData::boundary definition from constant let to var

Testing Details 🔍

No test added

@jshier
Copy link
Contributor

jshier commented Mar 6, 2019

Thanks for the PR! Can you explain why you want this change? Additionally, since this would be newly mutable, we'd want some relevant tests over the functionality.

@jshier jshier self-assigned this Mar 6, 2019
@cnoon
Copy link
Member

cnoon commented Mar 26, 2019

Hi @ondrejstocek,

I'm going to go ahead and close this PR out for now since we haven't heard back. We don't want to keep these PRs open forever if we can help it. If you comment back on why this should be re-opened at some point, we can get it opened back up and pushed to master if it makes sense to do so.

Cheers. 🍻

@cnoon cnoon closed this Mar 26, 2019
@ondrejstocek
Copy link
Author

ondrejstocek commented Mar 26, 2019

Hello @cnoon,
some servers, I have to work with, don’t have multipart upload implemented correctly, and search just for 4-dash prefix instead of whole boundary string. It is possible because ALL main browsers generate boundary with at least two dashes at beginning. I can’t have any chance to change this behaviour on server side and it is literally NO chance to override upload class without changing significant part of Alamofire source code. This minor change alows to change the boundary string in uploading initial callback.

@cnoon
Copy link
Member

cnoon commented Mar 26, 2019

Thank you for that explanation @ondrejstocek, that does indeed make sense. I'm going to re-open this PR and work to get it through today.

@cnoon cnoon reopened this Mar 26, 2019
@cnoon cnoon self-requested a review March 26, 2019 17:03
@cnoon cnoon modified the milestones: 5.0.0-beta.4, 4.8.2 Mar 26, 2019
cnoon pushed a commit that referenced this pull request Mar 26, 2019
@cnoon
Copy link
Member

cnoon commented Mar 26, 2019

Okay @ondrejstocek, @jshier and I decided that we're going to push this change out in AF 4.x (will ship in 4.8.2), and that we're going to re-think the upload APIs around MultipartFormData to better support this use case. I just pushed up your change in 0f2135b to hotfix while maintaining your attribution, and we'll get it released shortly.

I'm going to go ahead and close this PR out for now, and I'll link the new PR in AF5 that addresses this issue just for visibility.

Cheers. 🍻

@cnoon
Copy link
Member

cnoon commented Mar 26, 2019

Hi @ondrejstocek, I just pushed up #2764 which is a better long-term solution to this issue. We're also considering moving MultipartFormData to take an Encoder, but that will be a separate effort.

@ondrejstocek
Copy link
Author

Thank you @cnoon,
looking into #2764 diff and changes look great.

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

Successfully merging this pull request may close these issues.

3 participants