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

Fix builds #30

Merged
merged 4 commits into from
Jan 19, 2020
Merged

Fix builds #30

merged 4 commits into from
Jan 19, 2020

Conversation

Xerkus
Copy link
Member

@Xerkus Xerkus commented Jan 17, 2020

Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA yes

Description

Fixing tests broken during Laminas migration

src/Headers.php Outdated
$class = ($this->getPluginClassLoader()->load(str_replace('-', '', $key))) ?: 'Laminas\Http\Header\GenericHeader';
$class = (
$this->getPluginClassLoader()->load(str_replace('-', '', $key))
) ?: 'Laminas\Http\Header\GenericHeader';
Copy link
Member

Choose a reason for hiding this comment

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

Brackets around expression are redundant, so I'd suggest the following formatting:

 $class = $this->getPluginClassLoader()->load(str_replace('-', '', $key))
    ?: 'Laminas\Http\Header\GenericHeader';

Copy link
Member Author

Choose a reason for hiding this comment

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

They are. They increase readability however, in my opinion

@Xerkus
Copy link
Member Author

Xerkus commented Jan 17, 2020

All tests failed successfully.

Remaining failures are linked to ssl* and tls1.0 disabled by default in test web server.

2) LaminasTest\Http\Client\ProxyAdapterTest::testUsesProvidedArgSeparator
stream_socket_enable_crypto(): SSL operation failed with code 1. OpenSSL Error messages:
error:14077410:SSL routines:SSL23_GET_SERVER_HELLO:sslv3 alert handshake failure

/home/travis/build/laminas/laminas-http/src/Client/Adapter/Proxy.php:289
/home/travis/build/laminas/laminas-http/src/Client/Adapter/Proxy.php:165
/home/travis/build/laminas/laminas-http/src/Client.php:1457
/home/travis/build/laminas/laminas-http/src/Client.php:945
/home/travis/build/laminas/laminas-http/test/Client/CommonHttpTests.php:1090
/home/travis/build/laminas/laminas-http/test/Client/ProxyAdapterTest.php:125

I believe proper way to fix it would be to remove ssl and use tls only.

$modes = [
STREAM_CRYPTO_METHOD_TLS_CLIENT,
STREAM_CRYPTO_METHOD_SSLv3_CLIENT,
STREAM_CRYPTO_METHOD_SSLv23_CLIENT,
STREAM_CRYPTO_METHOD_SSLv2_CLIENT,
];


Following curl failure needs to be debugged further, potential bug exposure:

1) LaminasTest\Http\Client\CurlTest::testNoCaseSensitiveHeaderName

Laminas\Http\Client\Adapter\Exception\RuntimeException: Error in cURL request: transfer closed with outstanding read data remaining
/home/travis/build/laminas/laminas-http/src/Client/Adapter/Curl.php:475
/home/travis/build/laminas/laminas-http/src/Client.php:1457
/home/travis/build/laminas/laminas-http/src/Client.php:945
/home/travis/build/laminas/laminas-http/test/Client/CurlTest.php:453

@michalbundyra
Copy link
Member

@Xerkus I am really unsure about failing tests here.
See the latest results of zend-http:
https://travis-ci.org/zendframework/zend-http

All was green there (18h ago last run). I think it might be something wrong with the migration here.

@Xerkus
Copy link
Member Author

Xerkus commented Jan 18, 2020

Difference is test uses $request->setUri('https://getlaminas.org');
And previously it was a server for zf site which supported tls 1.0.

@Xerkus Xerkus force-pushed the hotfix/builds branch 3 times, most recently from 11307e4 to 45a0c99 Compare January 18, 2020 05:36
@Xerkus
Copy link
Member Author

Xerkus commented Jan 18, 2020

It would have been easier if I bothered to setup test webserver locally in docker.

Newer curl complains about chunked transfer that was not properly terminated with zero length chunk. Fixed test asset to emit correct chunked response.

@Xerkus Xerkus closed this in 00ed4f5 Jan 19, 2020
@Xerkus Xerkus merged commit ec43bdc into laminas:master Jan 19, 2020
Xerkus added a commit that referenced this pull request Jan 19, 2020
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

2 participants