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

Support overwriting existing headers & response exception updates #55

Merged
merged 7 commits into from
Feb 2, 2022

Conversation

Ndiritu
Copy link
Contributor

@Ndiritu Ndiritu commented Jan 24, 2022

Changes:

  • Use array_merge to allow customers to overwrite our default header values. Appending values will be at the discretion of the developer
  • Support string response payloads from the Graph for 4xx/5xx errors. We had only anticipated JSON payloads from the Graph

Some breaking changes here:

  • GraphResponseException takes in a StreamInterface as opposed to an array
  • getRawResponseBody() return type changes from array to StreamInterface

Closes #53
Closes #54

@Ndiritu Ndiritu marked this pull request as ready for review January 24, 2022 14:24
@Ndiritu
Copy link
Contributor Author

Ndiritu commented Jan 24, 2022

@MIchaelMainer @SilasKenneth just confirming that the versioning is ok for breaking changes in a preview release?

Copy link
Contributor

@MIchaelMainer MIchaelMainer left a comment

Choose a reason for hiding this comment

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

We should also start an upgrade document to capture what changes are breaking. We will link to it in release notes and blog post announcements.

src/Exception/GraphResponseException.php Show resolved Hide resolved
src/Exception/GraphResponseException.php Outdated Show resolved Hide resolved
src/Exception/GraphResponseException.php Outdated Show resolved Hide resolved
@MIchaelMainer
Copy link
Contributor

The version change is fine since it is still RC for a major version change..

@Ndiritu
Copy link
Contributor Author

Ndiritu commented Jan 26, 2022

The version change is fine since it is still RC for a major version change..

Should we update the existing v1 & beta RC's to use versions of core <= 2.0.0-RC2 & release new RC's for v1, beta that use this new core release?
Trying to think of a way of reducing the blast radius of the breaking changes

@Ndiritu
Copy link
Contributor Author

Ndiritu commented Jan 26, 2022

We should also start an upgrade document to capture what changes are breaking. We will link to it in release notes and blog post announcements.

Can we leverage this guide and add a section at the top for upgrading between the current RC versions?

@MIchaelMainer
Copy link
Contributor

Should we update the existing v1 & beta RC's to use versions of core <= 2.0.0-RC2 & release new RC's for v1, beta that use this new core release? Trying to think of a way of reducing the blast radius of the breaking changes

Yes, the sooner the better.

Can we leverage this guide and add a section at the top for upgrading between the current RC versions?

Yes, good idea.

@MIchaelMainer
Copy link
Contributor

Once the change to upgrade guide is added we should be good here.

@Ndiritu
Copy link
Contributor Author

Ndiritu commented Feb 2, 2022

Once the change to upgrade guide is added we should be good here.

@MIchaelMainer Please see the related PR's on v1 and beta

@Ndiritu Ndiritu merged commit 5122761 into dev Feb 2, 2022
@Ndiritu Ndiritu deleted the fix/content-type branch February 2, 2022 12:13
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