Skip to content

fix: Multipart support #660

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

Merged
merged 9 commits into from
Jul 15, 2022
Merged

Conversation

erickskrauch
Copy link
Contributor

Fixes #657

This PR adds support for the multipart/form-data to upload files. It also introduces the File entity to allow the HTTP client adapter to determine that request must be made as a multipart. The current implementation allows to upload file by file path, by resource obtained via fopen or by raw file contents. It also allows to use any other file contents format, supported by the specific HTTP adapter implementation (for example PSR-7 StreamInterface implementation for Guzzle).

The problem is that Twilio's backend doesn't recognize file, uploaded with the multipart/form-data :( I have tested my implementation with the different backend implementations and in every case I successfully managed to parse and store uploaded files. Twilio just don't recognize files. I already compared body formed by my implementation with a body formed with the raw curl command (see this comments). I will gladly adjust body format to suit necessary Twilio's format if you will tip me how :)

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

@thinkingserious thinkingserious added status: code review request requesting a community code review or review from Twilio type: community enhancement feature request not on Twilio's roadmap labels Oct 3, 2020
@thinkingserious
Copy link
Contributor

Hello @erickskrauch,

Thanks for creating the PR! I am adding an internal identifier to track progress. We will update this ticket when we have a solution: DI-647

In the meantime, please see the associated issue for the current workarounds.

Thank you!

With best regards,

Elmer

@erickskrauch
Copy link
Contributor Author

erickskrauch commented Oct 3, 2020

@thinkingserious, thanks!

Any chance to have a multipart fix on Twilio's backend side soon? I want to use this PR on the job's project to finish the task.

Copy link
Contributor

@childish-sambino childish-sambino left a comment

Choose a reason for hiding this comment

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

The Twilio backend does actually support file uploads, it's just that we use a different subdomain for this. The support document upload example here uses numbers-upload.twilio.com (instead of numbers.twilio.com).

We're currently investigating including such upload subdomains in this library, but it will mostly be generated code from an internal API definition schema.

Given all this, I'm inclined to review/approve the changes if all the regulatory-specific changes are reverted (since that particular API does not support file uploads, and it will be blown away on the next release anyway).

@erickskrauch
Copy link
Contributor Author

erickskrauch commented Oct 10, 2020

@childish-sambino, hey, thanks a lot for the mention about the different domains. I didn't pay attention to it and didn't understand why my implementation doesn't work and the curl command works.

I removed all changes to the generated code as you requested. I also created a separate branch where I made the necessary changes to support document uploading. I checked my implementation: the document is uploaded successfully! I'm sure my solution isn't perfect, but it's more than enough to use in the job's project until the support of documents uploading will be included in the SDK.

@@ -213,10 +213,6 @@ public function request(string $method, string $uri, array $params = [], array $
' (PHP ' . PHP_VERSION . ')';
$headers['Accept-Charset'] = 'utf-8';

if ($method === 'POST' && !\array_key_exists('Content-Type', $headers)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this is a generated file (should be refactored so this particular part isn't generated) so we'll need to update the generator before merging.

@erickskrauch
Copy link
Contributor Author

Is there a chance to have this merged?

# Conflicts:
#	src/Twilio/Http/GuzzleClient.php
# Conflicts:
#	src/Twilio/Rest/Client.php
@childish-sambino
Copy link
Contributor

Moving this up the internal backlog. Should have a look to implement the upstream changes in a few sprints.

@claudiachua claudiachua merged commit 5c7708a into twilio:main Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: code review request requesting a community code review or review from Twilio type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to create a supporting document with a file?
4 participants