Manage "Expect" header with cURL transport #454

carlalexander · 2021-03-12T18:38:02Z

Fixes #453

carlalexander · 2021-03-12T18:40:14Z

I'm not sure how you prefer to handle the strlen calculation since data can be an array or a string. I did it inside the strlen happy to change it to something else.

jrfnl · 2021-03-12T19:28:35Z

@carlalexander Thanks for finding this and for the PR!

Do you think this could be covered by unit tests in some way ?

I'm not sure how you prefer to handle the strlen calculation since data can be an array or a string. I did it inside the strlen happy to change it to something else.

For code readability and comprehension, I'd much prefer it if it would be pulled apart and calculated before the strlen(), but that's just me.

Also, for the multi-line comment, you may want to use multi-comment style /*... */ ;-)

carlalexander · 2021-03-12T19:55:12Z

Normally, I do multi-line comments that way too. I'm just trying to keep the same coding style as the one below. Happy to switch it to the /*.

I can also create a function to get the get the strlen.

For testing, I'd like to add tests. I just didn't really see anything for testing cURL code. The cURL transport has 4 tests to check for some thrown exceptions. That's it. Did you have something in mind for testing it? Normally, I use a testing library to mock PHP functions. But I don't think that will work here since there's no namespace.

jrfnl · 2021-03-12T20:09:11Z

For testing, I'd like to add tests. I just didn't really see anything for testing cURL code.

I see where your confusion comes from. Most of the tests for the cURL code are in the RequestsTest_Transport_Base test class which the RequestsTest_Transport_cURL test class extends.
The RequestsTest_Transport_cURL class only contains the cURL specific tests, so I imagine a test for this cURL specific issue would be best placed in that file.

If you run the tests with phpunit --testdox --filter RequestsTest_Transport_cURL, you can see that the cURL class is tested with a lot more tests than just those in the one file.

jrfnl · 2021-03-12T20:10:39Z

Normally, I do multi-line comments that way too. I'm just trying to keep the same coding style as the one below. Happy to switch it to the /*.

I hear you. We've cleaned up the code style recently, cleaning up the comments is on the to do list still, ;-)

carlalexander · 2021-03-12T20:30:01Z

Alright, I'll take a look at adding tests over the weekend.

I've done another pass and fixed up the comment and moved the strlen code to its own method. I added the method at the end because I can't seem to figure out how methods are ordered. Just let me know where you'd like it.

carlalexander · 2021-03-14T17:58:16Z

Alright, so I've tried to write some tests, but the Heroku application that we run tests against doesn't seem to return the Expect header back. I've tried with both an empty header and one with 100-Continue.

Is the code for that Heroku application anywhere? Also it could be that Heroku strips the header.

jrfnl · 2021-03-15T03:50:34Z

@carlalexander I'm not sure at all, but I suspect it might be this repo: https://github.com/rmccue/httpbin

@rmccue Ryan, could you please confirm ? (or point to the real source)

carlalexander · 2021-03-15T04:32:29Z

Yeah, I'm trying against https://httpbin.org/headers and it's not returning the header regardless that its set or not.

schlessera · 2021-03-15T10:23:00Z

I think the source of the server is this one: https://github.com/RequestsPHP/test-server

rmccue · 2021-03-15T13:07:58Z

I think the source of the server is this one: https://github.com/RequestsPHP/test-server

That's the one, yep :)

jrfnl · 2021-03-19T12:49:16Z

@carlalexander Just checking in - will you have time to update the PR for the tests ? And if so, what timeline are we looking at ?

carlalexander · 2021-03-19T17:49:39Z

@jrfnl Hoping to sit down and hammer at it some more this weekend. 😄 I haven't looked at it this week.

carlalexander · 2021-03-20T18:17:35Z

Ok, so I have some tests running, but they only work with @rmccue's server. I'm just running it locally atm. Are there tests that use it? It seems everything uses httpbin.org and it doesn't return the header.

Another good news is you can clearly see the 1 sec timeout with the tests. So that's great.

jrfnl · 2021-03-20T20:20:46Z

@carlalexander I hope I understood your question correctly, but AFAICS, all tests which use a function call to httpbin() to set the address use the custom test server.

Also see #315 for some additional context.

codecov · 2021-03-20T21:04:13Z

Codecov Report

Merging #454 (5b99e01) into master (675728b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #454      +/-   ##
============================================
- Coverage     93.06%   93.05%   -0.01%     
- Complexity      763      771       +8     
============================================
  Files            21       21              
  Lines          1788     1800      +12     
============================================
+ Hits           1664     1675      +11     
- Misses          124      125       +1

Impacted Files	Coverage Δ	Complexity Δ
library/Requests/Transport/cURL.php	`95.00% <100.00%> (+0.28%)`	`74.00 <5.00> (+8.00)`
library/Requests/Transport/fsockopen.php	`93.78% <0.00%> (-0.57%)`	`69.00% <0.00%> (ø%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 675728b...5b99e01. Read the comment docs.

carlalexander · 2021-03-20T21:23:04Z

Ok, this helped @jrfnl! I tried with requests-php-tests.herokuapp.com and it also strips the header. But Travis seems to run the tests against the test server running with the built-in PHP server. So I pushed the tests up and they seem to work on Travis!

jrfnl · 2021-03-20T21:39:29Z

@carlalexander Awesome! I'll have a more detailed look soon. What about annotating the tests with the performance fix with @small to ensure they fail if the extra second would be added ?

jrfnl · 2021-03-20T21:42:30Z

@carlalexander Was just looking at Travis and I noticed there does still seem to be an issue:

1) RequestsTest_Transport_cURL::testGETWithNestedData

Array to string conversion

/home/travis/build/WordPress/Requests/library/Requests/Transport/cURL.php:573
/home/travis/build/WordPress/Requests/library/Requests/Transport/cURL.php:332
/home/travis/build/WordPress/Requests/library/Requests/Transport/cURL.php:135
/home/travis/build/WordPress/Requests/library/Requests.php:381
/home/travis/build/WordPress/Requests/tests/Transport/Base.php:111

2) RequestsTest_Transport_cURL::testPOSTWithNestedData

Array to string conversion

/home/travis/build/WordPress/Requests/library/Requests/Transport/cURL.php:573
/home/travis/build/WordPress/Requests/library/Requests/Transport/cURL.php:332
/home/travis/build/WordPress/Requests/library/Requests/Transport/cURL.php:135
/home/travis/build/WordPress/Requests/library/Requests.php:381
/home/travis/build/WordPress/Requests/library/Requests.php:270
/home/travis/build/WordPress/Requests/tests/Transport/Base.php:222

carlalexander · 2021-03-20T21:47:12Z

@jrfnl That's my bad. I was using the new function signature for implode. Fixed it. Also added @small tags.

carlalexander · 2021-03-20T21:50:30Z

Ok, well the old signature makes phpcs fail now lol

jrfnl · 2021-03-20T21:58:28Z

using the new function signature for implode.

There is no "new" function signature. There is a cross-version compatible signature and a second signature which was previously supported, but isn't anymore.

The function should be called like so: implode ( string $separator , array $array )

Ok, well the old signature makes phpcs fail now lol

And rightfully so as using the reverse order will throw a deprecation notice on PHP 7.4 (and fail the tests because of it) and be a fatal error on PHP 8.0.

carlalexander · 2021-03-20T22:07:13Z

Yeah, I'm reverting the change. I understand what's going on now. I'm also going to add some more tests.

jrfnl · 2021-03-20T22:17:14Z

@carlalexander Appreciated! You're a ⭐

carlalexander · 2021-03-20T22:55:38Z

Everything is green except for PHP 8.0. It doesn't seem to be an issue with the tests or code. It seems to be PHPUnit related.

jrfnl · 2021-03-20T22:58:03Z

Everything is green except for PHP 8.0. It doesn't seem to be an issue with the tests or code. It seems to be PHPUnit related.

Correct - see #439

carlalexander · 2021-03-20T23:07:08Z

Perfect, should be good to merge once you've done your review. Let me know if you need anything else 😄

library/Requests/Transport/cURL.php

carlalexander · 2021-03-27T20:04:52Z

Pushed @schlessera's recommended changes 😄

library/Requests/Transport/cURL.php

carlalexander · 2021-03-28T16:28:02Z

Really great suggestion @TimothyBJacobs! I've reworked the method to early return if we hit the 1MB limit.

schlessera · 2021-04-16T17:15:59Z

Dammit! I broke the PR when I wanted to do a rebase. When will I learn not to touch PRs that were done directly in master?

@carlalexander Are you able to push the change again rebased on latest master? If not, let me know, and I'll recreate your PR.

schlessera · 2021-04-16T17:31:46Z

I recreated the PR here: #469

jrfnl added the Component: Transports label Mar 12, 2021

jrfnl added this to the 1.7.1 milestone Mar 12, 2021

carlalexander force-pushed the master branch from 27dbddf to 4736f51 Compare March 12, 2021 20:26

carlalexander force-pushed the master branch 3 times, most recently from 011baa6 to 98eb7a6 Compare March 20, 2021 21:00

carlalexander force-pushed the master branch from 98eb7a6 to 7be4143 Compare March 20, 2021 21:21

carlalexander mentioned this pull request Mar 20, 2021

Update httpbin endpoint on tests #315

Open

carlalexander force-pushed the master branch 2 times, most recently from bdc1053 to 11df650 Compare March 20, 2021 21:45

carlalexander force-pushed the master branch from 11df650 to 4ab6206 Compare March 20, 2021 21:53

carlalexander force-pushed the master branch 2 times, most recently from 7031301 to 7b32a7b Compare March 20, 2021 22:42

schlessera requested changes Mar 23, 2021

View reviewed changes

library/Requests/Transport/cURL.php Outdated Show resolved Hide resolved

library/Requests/Transport/cURL.php Outdated Show resolved Hide resolved

carlalexander force-pushed the master branch from 7b32a7b to 6ec6bde Compare March 27, 2021 20:01

TimothyBJacobs reviewed Mar 28, 2021

View reviewed changes

library/Requests/Transport/cURL.php Outdated Show resolved Hide resolved

carlalexander force-pushed the master branch from 6ec6bde to 74152c9 Compare March 28, 2021 16:26

carlalexander force-pushed the master branch from 74152c9 to 5b99e01 Compare March 28, 2021 16:37

schlessera closed this Apr 16, 2021

schlessera force-pushed the master branch from 5b99e01 to 519ac37 Compare April 16, 2021 17:06

schlessera mentioned this pull request Apr 16, 2021

Manage "Expect" header with cURL transport #469

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manage "Expect" header with cURL transport #454

Manage "Expect" header with cURL transport #454

carlalexander commented Mar 12, 2021

carlalexander commented Mar 12, 2021

jrfnl commented Mar 12, 2021

carlalexander commented Mar 12, 2021 •

edited

Loading

jrfnl commented Mar 12, 2021

jrfnl commented Mar 12, 2021

carlalexander commented Mar 12, 2021

carlalexander commented Mar 14, 2021

jrfnl commented Mar 15, 2021

carlalexander commented Mar 15, 2021

schlessera commented Mar 15, 2021

rmccue commented Mar 15, 2021

jrfnl commented Mar 19, 2021

carlalexander commented Mar 19, 2021

carlalexander commented Mar 20, 2021

jrfnl commented Mar 20, 2021

codecov bot commented Mar 20, 2021 •

edited

Loading

carlalexander commented Mar 20, 2021

jrfnl commented Mar 20, 2021

jrfnl commented Mar 20, 2021 •

edited

Loading

carlalexander commented Mar 20, 2021

carlalexander commented Mar 20, 2021

jrfnl commented Mar 20, 2021

carlalexander commented Mar 20, 2021

jrfnl commented Mar 20, 2021

carlalexander commented Mar 20, 2021

jrfnl commented Mar 20, 2021

carlalexander commented Mar 20, 2021

carlalexander commented Mar 27, 2021

carlalexander commented Mar 28, 2021 •

edited

Loading

schlessera commented Apr 16, 2021

schlessera commented Apr 16, 2021

Manage "Expect" header with cURL transport #454

Manage "Expect" header with cURL transport #454

Conversation

carlalexander commented Mar 12, 2021

carlalexander commented Mar 12, 2021

jrfnl commented Mar 12, 2021

carlalexander commented Mar 12, 2021 • edited Loading

jrfnl commented Mar 12, 2021

jrfnl commented Mar 12, 2021

carlalexander commented Mar 12, 2021

carlalexander commented Mar 14, 2021

jrfnl commented Mar 15, 2021

carlalexander commented Mar 15, 2021

schlessera commented Mar 15, 2021

rmccue commented Mar 15, 2021

jrfnl commented Mar 19, 2021

carlalexander commented Mar 19, 2021

carlalexander commented Mar 20, 2021

jrfnl commented Mar 20, 2021

codecov bot commented Mar 20, 2021 • edited Loading

Codecov Report

carlalexander commented Mar 20, 2021

jrfnl commented Mar 20, 2021

jrfnl commented Mar 20, 2021 • edited Loading

carlalexander commented Mar 20, 2021

carlalexander commented Mar 20, 2021

jrfnl commented Mar 20, 2021

carlalexander commented Mar 20, 2021

jrfnl commented Mar 20, 2021

carlalexander commented Mar 20, 2021

jrfnl commented Mar 20, 2021

carlalexander commented Mar 20, 2021

carlalexander commented Mar 27, 2021

carlalexander commented Mar 28, 2021 • edited Loading

schlessera commented Apr 16, 2021

schlessera commented Apr 16, 2021

carlalexander commented Mar 12, 2021 •

edited

Loading

codecov bot commented Mar 20, 2021 •

edited

Loading

jrfnl commented Mar 20, 2021 •

edited

Loading

carlalexander commented Mar 28, 2021 •

edited

Loading