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

Attachments now automatically get base64 encoded if they are not already #618

Merged
merged 6 commits into from
Aug 6, 2018
Merged

Attachments now automatically get base64 encoded if they are not already #618

merged 6 commits into from
Aug 6, 2018

Conversation

martijnmelchers
Copy link
Contributor

@martijnmelchers martijnmelchers commented May 20, 2018

Fixes

Fixes #611

Checklist

  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the master 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 in line documentation to the code I modified

Short description of what this PR does:

  • When an attachement is added it first checks whether or not it has already been base64 encoded, if not it encodes its.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label May 20, 2018
@SendGridDX
Copy link

SendGridDX commented May 20, 2018

CLA assistant check
All committers have signed the CLA.

@martijnmelchers martijnmelchers changed the title Attachements now automatically get base64 encoded if they are not already Attachments now automatically get base64 encoded if they are not already May 20, 2018
@thinkingserious
Copy link
Contributor

Nicely done @martijnmelchers!

We just need to add some tests to get this one merged. Do you have time to do that?

@martijnmelchers
Copy link
Contributor Author

Sure will do that today!

@martijnmelchers
Copy link
Contributor Author

@thinkingserious In what file should the tests be created?

@thinkingserious
Copy link
Contributor

thinkingserious commented May 22, 2018

How about Attachments.php in /test/unit/

BTW, have you been able to get the tests running locally using Docker?

@martijnmelchers
Copy link
Contributor Author

I'm not able to get the Docker thing running, so I might not be able to run tests. How should I proceed?

@thinkingserious
Copy link
Contributor

I will help you with that. Please email me at dx@sendgrid.com and let me know where you are stuck. Thanks!

@martijnmelchers
Copy link
Contributor Author

There seems to be a difference in PHP 7.1 and up with base64_decode any solutions you can think of @thinkingserious

@martijnmelchers
Copy link
Contributor Author

Ok cool! I have figured it out! Will finish some PHPDocs and then you will be able to merge it!

@thinkingserious
Copy link
Contributor

Awesome! Is this ready to go?

@martijnmelchers
Copy link
Contributor Author

martijnmelchers commented May 23, 2018 via email

@thinkingserious thinkingserious added type: community enhancement feature request not on Twilio's roadmap difficulty: medium fix is medium in difficulty labels Aug 6, 2018
@thinkingserious thinkingserious merged commit d86ef48 into sendgrid:master Aug 6, 2018
@thinkingserious
Copy link
Contributor

Hello @martijnmelchers,

Thanks again for the PR!

We appreciate your contribution and look forward to continued collaboration. Thanks!

Team SendGrid DX

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium fix is medium in difficulty 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.

Detect if an attachment is Base64 encoded, if not, automatically encode it
3 participants