-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
ZendClient breaks when receiving an HTTP/2 response #19441
Comments
Hi @ryanreeves-taxjar. Thank you for your report.
Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:
where @ryanreeves-taxjar do you confirm that you was able to reproduce the issue on vanilla Magento instance following steps to reproduce?
|
Hi @engcom-backlog-nazar. Thank you for working on this issue.
|
Hi @ryanreeves-taxjar thank you for you report, please look -> #19143 |
@engcom-backlog-nazar Could you please re-open this issue? This issue is not resolved by #19143 because it affects Zend_Http_Client via cURL adapter, not cURL client. Thank you! |
@fastdivision ok, i'm will retest |
@fastdivision The support to http/2 will be available soon |
@ryanreeves-taxjar Hey im getting this error when getting using taxjar, is there any work around?
|
Hi @crantron. This is the Github for Magento 2 issues. Please contact our support team at support@taxjar.com and we'll help you out! |
@ryanreeves-taxjar on it! |
Still got this problem on current 2.2.7 installation. This line explicitly only checks for "HTTP/X.X" pattern, "HTTP/X" is rejected: vendor/magento/zendframework1/library/Zend/Http/Response.php
|
@josefbehr this will be fixed by this PR #19845 |
@engcom-backlog-nazar thanks for the hint, but I don't see how this fixes the problem. Can you elaborate on that? And when can we expect the fix to arrive? |
@josefbehr you can set HTTP2/version -> |
I see, and default is 1.1. Thanks. |
How does this help? Still doesn't match HTTP/2 |
@engcom-backlog-nazar unfortunately, this does not help. I had to overwrite Zend Client.php and Http.php to get it working. Please re-open this issue unless you can definitely show that it is fixed. |
As suggested by the devs at TaxJar I "fixed" this by modifying magento/zendframework1/library/Zend/Http/Response.php in two places: Line 185
Line 518
Two other suggestions provided by TaxJar are
Message from TaxJar:
I don't really get how issue 19442 fixes this though (which modified vendor/magento/framework/HTTP/Adapter/Curl.php). If you were to use \Zend_Http_Client and \Zend_Http_Response directly as TaxJar does the issue still exists. Seems as though it needs to be fixed in magento/zendframework1 (magento/zf1). |
@Jimgitsit Yes, exactly. Just applying the change from #19442 to Curl.php does not resolve the issue. I also had to change Response.php, and I think another file, too, in a similar manner to get Magento to accept HTTP/2 responses. I'll re-open this ticket then. Unfortunately, it is a problem in Zf1, which should be replaced anyway. So I'm not sure if a PR changing these files would be accepted. |
I believe this issue is related to PR #21549 and should be resolved once that PR is merged in. |
@davidalger |
@josefbehr The ZF1 library does not support HTTP/2 and that is why the regex is the way it is (if you look at Did you look at the code which demonstrates reproducing the same exception due to the same regex that doesn't work with HTTP/2 responses? The issue here isn't the regex, but that the curl adapter (i.e. the Magento framework's implementation that overrides the working one in ZF1) was doing nothing with the default
|
@davidalger That "fix" did not fix the problem for me (and apparently others), when applied, so either there is a secondary problem in some circumstances, or this change does not completely fix the occurrence of this error. At least that's my conclusion. If you insist, we can close this issue, but won't that kinda dismiss the other experiences? I might open another issue for that, though. |
@josefbehr I don’t want to argue, but I see nothing in any of the above indicating anyone else on this thread tried the change in PR #21549. This is a different change than the other mentioned PRs and it does fix calling out to HTTP2 endpoints from a system with HTTP2 enabled curl libraries. There is sample code on the PR that proves this. It doesn’t do it by attemping to fix the parsing of HTTP2 responses, but by properly setting 1.0 or 1.1 on the curl resources so the server does not reply with HTTP2 to begin with. Mind taking another look at PR #21549, and showing some sample code which causes this exception with the fix on that PR in place? |
Hi @ryanreeves-taxjar according to #19441 (comment) this problem related to zendframework, you can create a ticket here -> https://github.com/magento/community-features to use new version of zend framework, thanks for collaboration. |
Hi All,
Copy the vendor/magento/zendframework1/library/Zend/Http/Response.php to lib/Zend/Http/Response.php Edit the following lines. Then Run The issue is Zend_Http_Response doesn't support HTTP/2. It only support HTTP/1.1. What I have done here is load the file in lib/Zend/Http/Response.php instead vendor/magento/zendframework1/library/Zend/Http/Response.php and edit the regular expression to accept the HTTP/2. |
Preconditions (*)
Steps to reproduce (*)
Expected result (*)
Actual result (*)
Solution
if ($index === 0 && preg_match('#^HTTP/\d+(?:\.\d+) [1-5]\d+#', $line)) {
is where string "HTTP/2 200" breaks. The regex should optionally enforce the period since 2.0 was dropped from HTTP/2. Sample fixed regex: '#^HTTP/\d+(?:.\d+)? [1-5]\d+#'
The text was updated successfully, but these errors were encountered: