Skip to content

feat: uses guzzle/psr and psr-7 instead of http-foundation #28

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 29, 2020
Merged

feat: uses guzzle/psr and psr-7 instead of http-foundation #28

merged 3 commits into from
May 29, 2020

Conversation

bshaffer
Copy link
Collaborator

@bshaffer bshaffer commented May 13, 2020 •
edited
Loading

Addresses #23

@bshaffer bshaffer requested a review from grant May 13, 2020 22:18
@bshaffer bshaffer mentioned this pull request May 13, 2020
Closed
@bshaffer bshaffer requested a review from grayside May 13, 2020 22:27
grant
grant approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

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

Looks fine to me. I'm not a PHP expert though.

  • The functions in src/Emitter.php could use some documentation/comments.
  • The headers function looks really custom.
  • Did we want to write tests or TODOs for tests?

@bshaffer
Copy link
Collaborator Author

@grant I'll add tests for Emitter before I merge. The headers function isn't really custom, it should be implementing the bare minimum for the HTTP spec... although it's a little more complex than I'd like.

@grant
Copy link
Contributor

grant commented May 13, 2020

implementing the bare minimum for the HTTP spec
Yes, it looks like that.

I would assume there'd be a standard library or something for the raw HTTP spec that we could reuse. Maybe there isn't one.

Anyways, looks good to me.

@grayside
Copy link

Agree with Grant that a library to handle the emitter functionality would be good, but it's just simple enough that avoiding the dependency probably outweighs it. Seems like something which could be changed without a b/c break later.

@bshaffer
Copy link
Collaborator Author

bshaffer commented May 14, 2020 •
edited
Loading

@grant unfortunately there is not a "standard". There are libraries which handle this, but I don't feel comfortable integrating them into our product.

@grayside yes, that was my reasoning. Even relying on guzzlehttp/psr7 is not optimal IMHO.

@bshaffer bshaffer merged commit 4e59cea into master May 29, 2020
@bshaffer bshaffer deleted the support-guzzle-psr7 branch May 29, 2020 18:18
copybara-service bot pushed a commit to GoogleCloudPlatform/buildpacks that referenced this pull request Jul 9, 2020
@mtraver @copybara-github
Bump PHP functions framework version from ^0.2 to ^0.3
f0f4377 
0.3.0 uses PSR-7 HTTP messages instead of symfony/http-foundation. See
GoogleCloudPlatform/functions-framework-php#23 and
GoogleCloudPlatform/functions-framework-php#28. As such
the tests are updated.

PiperOrigin-RevId: 320480573
Change-Id: I93ea64984313ef75b7f326c648f65f729763c418
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grayside grayside grayside approved these changes

+1 more reviewer

@grant grant grant approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bshaffer @grant @grayside