Skip to content

Allow Request to be extended by changing class name 'Request' to static#231

Closed
bluehaoran wants to merge 19 commits into
nategood:devfrom
bluehaoran:master
Closed

Allow Request to be extended by changing class name 'Request' to static#231
bluehaoran wants to merge 19 commits into
nategood:devfrom
bluehaoran:master

Conversation

@bluehaoran

Copy link
Copy Markdown

New PR because I didn't target Dev last time.

Fixed a typo, and added static dereferencing, allowing Request to be extended (so I could add automatic HMAC signing on send()).

@KarimGeiger

Copy link
Copy Markdown

I too would love to see this PR since I'm trying to extend Request as well. +1

Comment thread composer.json Outdated
{
"name": "nategood/httpful",
"description": "A Readable, Chainable, REST friendly, PHP HTTP Client",
"name": "bluehaoran/httpful",

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these composer changes for the PR. Thanks.

*/
public function offsetExists($offset)
{
return isset($this->headers[strtolower($offset)]);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I believe this would be a breaking change. It's one I'm for but I believe we'll need to wait to do this until the next major release.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's been three years since the last mayor release, and about one year since the last release. I think it's time for some breaking changes :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have no strong feelings about this one, one way or another.

@bluehaoran

Copy link
Copy Markdown
Author

Composer.json rolled back as requested.

@bluehaoran

Copy link
Copy Markdown
Author

Is there anything else we need to do to get this PR over the line?

@bluehaoran

bluehaoran commented Oct 17, 2017

Copy link
Copy Markdown
Author

Replaced by #264 .

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.

5 participants