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

Fixed the handling of HttpSocket's response body #52

Closed

Conversation

angelxmoreno
Copy link
Contributor

HttpSocket returns an object with body holding the response. Not the response itself. Maybe at some point it returned what we now expect HttpSocket->body to be. Also, as per http://book.cakephp.org/2.0/en/core-utility-libraries/xml.html#importing-data-to-xml-class and http://book.cakephp.org/2.0/en/core-utility-libraries/xml.html#transforming-a-xml-string-in-array I made use of how we now ( as in 2.x ) convert XML to array

@jippi
Copy link
Contributor

jippi commented Jul 17, 2013

Can you please provide a test to prove there was a problem initially? and to avoid us breaking something in the future :)

@angelxmoreno
Copy link
Contributor Author

merged the recent PR from 2.3 and added a test for the request method. in 2.3 the entire Httpsocket object is being treated as the http response and it is assumed to be XML response. This makes it always return an empty array. The test passes in the PR and fails in 2.3. All future fixes will include a test ;-)

* @return void
* @access public
*/
function testRequestReturnsExpectedData() {
Copy link
Member

Choose a reason for hiding this comment

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

Should be public function. And we dont use "@access" anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

honestly i just choose to do so because its the style of the entire class. i have in my notes to do a proper clean. this class was ported from some 1.x branch. I figured fixing the ugly php4 coding would be a seperate PR altogether.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I just saw the diff :)

@renan renan closed this Dec 29, 2013
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.

None yet

5 participants