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

tests(ext-curl): fix HTTP/2 Server Push tests #10669

Closed
wants to merge 9 commits into from

Conversation

dunglas
Copy link
Contributor

@dunglas dunglas commented Feb 22, 2023

  • Use Caddy to expose local URLs supporting Server Push
  • Unskip tests for Server Push
  • Add a new test for Server Push

@dunglas dunglas force-pushed the tests/unskip-server-push-tests branch 4 times, most recently from e36f8f4 to 0f52bfb Compare February 22, 2023 22:51
.github/actions/setup-caddy/action.yml Outdated Show resolved Hide resolved
.github/actions/setup-caddy/action.yml Show resolved Hide resolved
.github/workflows/push.yml Outdated Show resolved Hide resolved
@dunglas dunglas force-pushed the tests/unskip-server-push-tests branch 5 times, most recently from 93516ef to 43a4d5e Compare February 25, 2023 14:44
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Great! I'm not well versed with HTTP and Curl, I had to look up how these options work but from what I can tell the test looks good. Thank you 🙂

ext/curl/tests/bug77535.phpt Outdated Show resolved Hide resolved
@dunglas dunglas force-pushed the tests/unskip-server-push-tests branch from b61d731 to 834673d Compare February 27, 2023 10:21
@iluuu1994
Copy link
Member

iluuu1994 commented Mar 7, 2023

Sorry for the late response. I see the check was changed to fsockopen("localhost") (instead of trying for /serverpush). If there's a local server running the test will attempt to run as well. If there's a problem with testing for /serverpush an env variable might be better after all. Sorry for the back and forth.

@dunglas
Copy link
Contributor Author

dunglas commented Mar 7, 2023

The main problem is that file_get_contents doesn't have support for TLS (and so HTTS) by default, at least on my machine.

That's why I used fsockopen instead.

I could try to use curl instead of file_get_contents, but the check will take more lines of code.

@iluuu1994
Copy link
Member

I guess that could be moved to a shared skipif file if too long. If it doesn't work well I won't object to a env var.

@dunglas dunglas force-pushed the tests/unskip-server-push-tests branch from 9505fb1 to fa79f6d Compare March 10, 2023 23:11
@dunglas dunglas force-pushed the tests/unskip-server-push-tests branch from fa79f6d to a6bc8a8 Compare June 11, 2023 20:57
@dunglas
Copy link
Contributor Author

dunglas commented Jun 11, 2023

@iluuu1994 is there anything more to do on this patch?

@staabm
Copy link
Contributor

staabm commented Jun 14, 2023

Fwiw, most browsers and servers drop support for http2 server push.

Not sure whether keeping it in php is the right direction

See e.g.

I am no expert, just mention what I found by accident

@dunglas
Copy link
Contributor Author

dunglas commented Jun 14, 2023

Indeed Chrome recommends 103 Early Hints (status in PHP: #7025) now, but Server Push is still supported by many tools (server-side and client-side) and will not be removed from the spec.

Anyway, enabling these tests is not related to the status of Server Push. As long as the code is part of PHP (and of curl) and officially supported, it should be tested.

If we (or curl) remove this feature at some point, then we'll remove the corresponding tests.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Hi @dunglas! Sorry for the late response, again... I get some segfaults running this locally, do you also get these? The output is from building with --enable-address-sanitizer.

ext/curl/tests/Caddyfile Show resolved Hide resolved
ext/curl/tests/bug76675.phpt Show resolved Hide resolved
@dunglas dunglas force-pushed the tests/unskip-server-push-tests branch from a6bc8a8 to 1606f89 Compare June 29, 2023 13:49
@dunglas dunglas force-pushed the tests/unskip-server-push-tests branch from 1606f89 to b0911cc Compare June 29, 2023 14:34
@dunglas
Copy link
Contributor Author

dunglas commented Jul 3, 2023

Segfaults have been fixed in libcurl 8.1.0. I skipped these tests if libcurl version is older.

@iluuu1994 iluuu1994 closed this in 47d4788 Jul 7, 2023
@iluuu1994
Copy link
Member

At last 😄 Thanks @dunglas!

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