[WEB-19] Add timeouts - #9
Open
PeterMarynczak wants to merge 2 commits into
Open
Conversation
PeterMarynczak
requested review from
arkadiuszroch,
marcinmdev and
mfrankiewicz
August 14, 2026 12:57
marcinmdev
reviewed
Aug 14, 2026
| xdebug-2.5.5 \ | ||
| && docker-php-ext-enable \ | ||
| xdebug | ||
| RUN pecl install xdebug \ |
There was a problem hiding this comment.
pecl jest awaryjny, lepiej xdebug jako pakiet z distro
marcinmdev
reviewed
Aug 17, 2026
Author
There was a problem hiding this comment.
To jest test utworzony jeszcze przez Meritoo w 2017,
Ten test pokrywa konstruktor, walidację, wartości domyślne i logikę pomocniczą.
Podpiąłem się pod niego bo zaktualizowałem liczbę argumentów z 5 na 7.
Wartość - zabezpiecza przed regresją.
|
Dlaczego ten PR leci od razu do mastera? |
There was a problem hiding this comment.
Pull request overview
This PR introduces configurable HTTP connect/request timeouts for the LimeSurvey JSON-RPC client, while also modernizing the project’s tooling and test suite for PHP 8 / PHPUnit 10.
Changes:
- Add optional connect/request timeout settings to
ConnectionConfigurationand apply them inJsonRpcClientManager. - Migrate tests to PHPUnit 10 APIs (
expectException,MockObject,setUp(): void) and updatephpunit.xml.dist. - Refresh dev environment/tooling (Docker image, Xdebug config) and raise runtime requirement to PHP >= 8.0.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Type/MethodTypeTest.php | PHPUnit API migration (setExpectedException → expectException). |
| tests/Service/SurveyServiceTest.php | PHPUnit API migration + mock builder updates; adjusts consecutive-call return values. |
| tests/Service/ParticipantServiceTest.php | PHPUnit API migration, typed setUp(): void, mock builder updates. |
| tests/Result/ResultTest.php | PHPUnit API migration, typed setUp(): void, updated MockObject types. |
| tests/Result/Processor/ResultProcessorTest.php | PHPUnit API migration (expectException). |
| tests/Result/Item/SurveyTest.php | Typed setUp(): void for PHPUnit 10 compatibility. |
| tests/Result/Item/QuestionTest.php | Typed setUp(): void for PHPUnit 10 compatibility. |
| tests/Result/Item/QuestionShortTest.php | Typed setUp(): void for PHPUnit 10 compatibility. |
| tests/Result/Item/ParticipantTest.php | Typed setUp(): void for PHPUnit 10 compatibility. |
| tests/Result/Item/ParticipantShortTest.php | Typed setUp(): void for PHPUnit 10 compatibility. |
| tests/Result/Collection/SurveysTest.php | Typed setUp(): void for PHPUnit 10 compatibility. |
| tests/Result/Collection/SurveysSummariesTest.php | PHPUnit API migration + typed setUp(): void. |
| tests/Result/Collection/ParticipantsTest.php | PHPUnit API migration + typed setUp(): void. |
| tests/Manager/SessionManagerTest.php | PHPUnit API migration + updated mock construction. |
| tests/Manager/JsonRpcClientManagerTest.php | Adds coverage for timeout behavior + PHPUnit 10 mock API updates. |
| tests/Configuration/ConnectionConfigurationTest.php | Updates constructor arity expectations + asserts new timeout getters. |
| tests/Client/ClientTest.php | PHPUnit API migration + updated mock construction + typed setUp(): void. |
| src/Manager/JsonRpcClientManager.php | Applies configured connect/request timeouts to underlying HTTP client. |
| src/Configuration/ConnectionConfiguration.php | Adds new timeout fields, constructor args, and getters. |
| README.md | Documents new timeout constructor arguments and default behavior. |
| phpunit.xml.dist | Updates configuration format/schema for PHPUnit 10. |
| docker/config/xdebug.ini | Updates Xdebug 3 configuration keys. |
| docker/config/Dockerfile | Updates Docker base image and tooling installation approach. |
| composer.json | Raises PHP requirement to >= 8.0 and bumps dev dependencies (incl. PHPUnit 10). |
| .gitignore | Ignores PHPUnit result cache file. |
Suppressed comments (1)
tests/Manager/JsonRpcClientManagerTest.php:130
- This test uses CURLOPT_TIMEOUT, which will fatally error when ext-curl is not installed. Add a curl availability guard and skip when needed.
static::assertEquals(2, $this->getHttpClientProperty($httpClient, 'timeout'));
static::assertEquals(7, $this->getHttpClientProperty($httpClient, 'options')[CURLOPT_TIMEOUT]);
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
114
to
+119
| $this->username = $username; | ||
| $this->password = $password; | ||
| $this->debugMode = $debugMode; | ||
| $this->verifySslCertificate = $verifySslCertificate; | ||
| $this->connectTimeout = $connectTimeout; | ||
| $this->requestTimeout = $requestTimeout; |
Comment on lines
+114
to
+121
| public function testGetRpcClientWithoutTimeoutsDoesNotTouchHttpClientDefaults() | ||
| { | ||
| $configuration = new ConnectionConfiguration('http://test.com', 'test', 'test'); | ||
| $httpClient = $this->getHttpClientOfManager(new JsonRpcClientManager($configuration)); | ||
|
|
||
| static::assertEquals(5, $this->getHttpClientProperty($httpClient, 'timeout')); | ||
| static::assertArrayNotHasKey(CURLOPT_TIMEOUT, $this->getHttpClientProperty($httpClient, 'options')); | ||
| } |
Comment on lines
+17
to
+21
| && cp /usr/share/zoneinfo/${TIMEZONE} /etc/localtime \ | ||
| && echo "${TIMEZONE}" > /etc/timezone | ||
|
|
||
| RUN set -x \ | ||
| && EXT_DIR="$(php -r 'echo PHP_EXTENSION_DIR;')" \ |
Comment on lines
+124
to
+129
| if (null !== $this->connectionConfiguration->getRequestTimeout()) { | ||
| $this | ||
| ->rpcClient | ||
| ->getHttpClient() | ||
| ->addOption(CURLOPT_TIMEOUT, $this->connectionConfiguration->getRequestTimeout()); | ||
| } |
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.