Skip to content

fix: Adjust payload format for PHP endpoints#467

Merged
keelerm84 merged 1 commit into
mainfrom
mk/sdk-1023/php
Jan 2, 2025
Merged

fix: Adjust payload format for PHP endpoints#467
keelerm84 merged 1 commit into
mainfrom
mk/sdk-1023/php

Conversation

@keelerm84
Copy link
Copy Markdown
Member

No description provided.

@keelerm84 keelerm84 requested a review from mike-zorn December 24, 2024 17:28

router.Handle("/all", GetProjectKeyFromAuthorizationHeader(http.HandlerFunc(StreamServerAllPayload)))

router.PathPrefix("/sdk/flags/{flagKey}").
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The GetServerFlags method already has a branch of logic that handles the individual flag request if the flagKey variable is set. However, we never defined that route so the variable would in fact get set.

}
} else {
body = ServerAllPayloadFromFlagsState(allFlags)
body = ServerFlagsFromFlagsState(allFlags)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For PHP, the /sdk/flags endpoint should return only the flags in an array, not the full flags + segments payload.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh wait a sec. Will this break non-php sdks? I think we need this to have a 3rd case where we do just flags and not all?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't believe this is a problem. This function only serves two routes -- /sdk/flags/{flagKey} and /sdk/flags.

These are routes that should exclusively be used by PHP SDKs. Non-PHP SDKs don't make flag only requests such as these.

I think we need this to have a 3rd case where we do just flags and not all?

Line 25 is serves all flags. Line 20 serves a single flag (assuming the flag key parameter is set).

The /all endpoint is serviced by the StreamServerAllPayload handler.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The /all endpoint is serviced by the StreamServerAllPayload handler.

Ahhh, OK. Cool. I forgot about that.

Copy link
Copy Markdown

@mike-zorn mike-zorn left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@mike-zorn mike-zorn self-requested a review January 2, 2025 16:58
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