Skip to content

Fix | V1 Encoded Files In Fixtures#155

Merged
Sammyjo20 merged 3 commits intov1from
fix/v1-encoded-files-in-fixtures
Feb 6, 2023
Merged

Fix | V1 Encoded Files In Fixtures#155
Sammyjo20 merged 3 commits intov1from
fix/v1-encoded-files-in-fixtures

Conversation

@Sammyjo20
Copy link
Copy Markdown
Member

@Sammyjo20 Sammyjo20 commented Feb 6, 2023

There was an issue which was raised in discussion #148 where if a request returned a downloadable file, the fixture could not be recorded because it threw an exception "Malformed UTF-8 characters, possibly incorrectly encoded"

@derheyne Came up with a brilliant idea to base64_encode() the response body if it cannot detect UTF-8 encoding. This is also backwards compatible because of the added encoding option.

@what-the-diff
Copy link
Copy Markdown

what-the-diff bot commented Feb 6, 2023

  • Added a new test to check that the file data is recorded correctly.
  • Updated FixtureData class so it can handle binary files and encode them in base64 if they are not UTF-8 encoded.
  • Fixed deprecation message for invalid request keys, as this was causing tests to fail on PHP 8+.

Copy link
Copy Markdown
Member Author

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

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

Self review

@Sammyjo20
Copy link
Copy Markdown
Member Author

@derheyne Let me know if you're happy with this and I'll roll it out

@derheyne
Copy link
Copy Markdown

derheyne commented Feb 6, 2023

Looks good to me! Thank you so much for the fast implementation :)

Before you merge it let me quickly test it against our code base.

@derheyne
Copy link
Copy Markdown

derheyne commented Feb 6, 2023

Works perfectly in our code base!

@Sammyjo20
Copy link
Copy Markdown
Member Author

Great thank you for testing!

@Sammyjo20 Sammyjo20 merged commit 480c027 into v1 Feb 6, 2023
@Sammyjo20 Sammyjo20 deleted the fix/v1-encoded-files-in-fixtures branch February 8, 2023 22:26
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.

2 participants