Merged
Conversation
📝 WalkthroughWalkthroughUpdates to the PHP SDK and demos to support optional engineId, switch auth to X-API-Key, change API host to api.lingo.dev, introduce per-instance sessionId for requests, adapt endpoints to /process/* paths, remove the respect/validation dependency, and expand docs, demo scripts, and tests to cover new payload/headers and behaviors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/LingoDotDevEngineTest.php (1)
57-60: Consider extracting_httpClientinjection into one helper.The same reflection block is duplicated in two places, which makes future test-scaffold changes easier to miss.
♻️ Possible refactor
+ private function _injectHttpClient(LingoDotDevEngine $engine, Client $client): void + { + $reflection = new ReflectionClass($engine); + $property = $reflection->getProperty('_httpClient'); + $property->setAccessible(true); + $property->setValue($engine, $client); + } @@ - $reflection = new ReflectionClass($engine); - $property = $reflection->getProperty('_httpClient'); - $property->setAccessible(true); - $property->setValue($engine, $client); + $this->_injectHttpClient($engine, $client); @@ - $reflection = new ReflectionClass($engine); - $property = $reflection->getProperty('_httpClient'); - $property->setAccessible(true); - $property->setValue($engine, $client); + $this->_injectHttpClient($engine, $client);Also applies to: 94-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/LingoDotDevEngineTest.php` around lines 57 - 60, Extract the duplicated reflection block that sets the private _httpClient into a single test helper method (e.g., setHttpClientOnEngine($engine, $client)) and use it in both places instead of repeating the ReflectionClass logic; inside the helper perform the ReflectionClass($engine), getProperty('_httpClient'), setAccessible(true) and setValue($engine, $client) so both occurrences (the block using ReflectionClass, $property, setAccessible, setValue) are replaced with a single call to that helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/LingoDotDevEngine.php`:
- Around line 70-72: The current check only uses empty() on
$this->config['engineId'] in LingoDotDevEngine and can allow whitespace or
reserved chars that break URL paths; replace that with a trimmed value and
strict validation (e.g., ensure $engineId = trim($this->config['engineId']) and
validate against an allowed character set such as alphanumerics, hyphen,
underscore via a regex) and throw \InvalidArgumentException if it fails, and
when constructing the request path (the method that concatenates the engine ID
into the URL) use proper URL-encoding (rawurlencode) of the engine ID instead of
direct concatenation; apply the same validation/encoding at the other occurrence
referenced (line ~163).
- Around line 58-63: The client lacks default timeouts: update the config merge
that sets $this->config (the array_merge block) to include sensible defaults
such as 'timeout' and 'connect_timeout' (e.g. timeout => 10, connect_timeout =>
5) and ensure these keys are passed through to the Guzzle client constructor
where the Client is instantiated (the new Client(...) usage around the later
block referenced 88-96) so callers cannot block indefinitely; preserve any
user-supplied overrides from $config while adding these defaults.
In `@test-all-methods.php`:
- Around line 41-52: Change the catch from catching \Exception to catching
\Throwable so fatal PHP errors (TypeError, ParseError, etc.) are handled by the
test harness: replace "catch (\Exception $e)" with "catch (\Throwable $e)". Keep
the existing RequestException-specific logic (the instanceof RequestException &
hasResponse() block) but guard it safely when $e may not be an Exception (e.g.,
ensure the RequestException class is referenced correctly or fully qualified as
needed, so the block still runs for Guzzle's RequestException).
---
Nitpick comments:
In `@tests/LingoDotDevEngineTest.php`:
- Around line 57-60: Extract the duplicated reflection block that sets the
private _httpClient into a single test helper method (e.g.,
setHttpClientOnEngine($engine, $client)) and use it in both places instead of
repeating the ReflectionClass logic; inside the helper perform the
ReflectionClass($engine), getProperty('_httpClient'), setAccessible(true) and
setValue($engine, $client) so both occurrences (the block using ReflectionClass,
$property, setAccessible, setValue) are replaced with a single call to that
helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e9f60a6-4b21-42f2-b164-7f9cf5a0c109
📒 Files selected for processing (5)
README.mdcomposer.jsonsrc/LingoDotDevEngine.phptest-all-methods.phptests/LingoDotDevEngineTest.php
Comment on lines
58
to
63
| $this->config = array_merge( | ||
| [ | ||
| 'apiUrl' => 'https://engine.lingo.dev', | ||
| 'apiUrl' => 'https://api.lingo.dev', | ||
| 'batchSize' => 25, | ||
| 'idealBatchItemSize' => 250 | ||
| ], $config |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify timeout configuration in the SDK HTTP client
echo "=== Checking for Client instantiation ==="
rg -nP 'new\s+Client\s*\(' src/LingoDotDevEngine.php -A 15
echo ""
echo "=== Checking for timeout/connect_timeout references ==="
rg -nP "'(timeout|connect_timeout)'\s*=>" src/LingoDotDevEngine.php
echo ""
echo "=== Checking full config method to see default values ==="
sed -n '55,100p' src/LingoDotDevEngine.phpRepository: lingodotdev/sdk-php
Length of output: 2448
🏁 Script executed:
#!/bin/bash
# Verify the HTTP client class source and check for Guzzle imports
echo "=== Checking imports at top of file ==="
head -20 src/LingoDotDevEngine.php
echo ""
echo "=== Searching for Client class usage pattern ==="
rg -n "Client" src/LingoDotDevEngine.php | head -5
echo ""
echo "=== Check if other files use timeout configuration ==="
rg -rn "timeout|connect_timeout" src/ --type=phpRepository: lingodotdev/sdk-php
Length of output: 899
🌐 Web query:
GuzzleHttp Client timeout connect_timeout options configuration
💡 Result:
In Guzzle (GuzzleHttp\Client), timeout and connect_timeout are request options (floats in seconds) that you pass either per request or as defaults when constructing the client.
What they mean
timeout: total time allowed for the whole request (0 = wait indefinitely). (docs.guzzlephp.org)connect_timeout: time allowed to establish the TCP connection (0 = wait indefinitely). Note: only supported by Guzzle’s built-in cURL handler. (docs.guzzlephp.org)
Set per request
$client = new \GuzzleHttp\Client();
$response = $client->request('GET', 'https://example.com', [
'connect_timeout' => 2.0, // seconds to connect
'timeout' => 10.0, // total request seconds
]);(Examples/definitions match Guzzle’s request options docs.) (docs.guzzlephp.org)
Set defaults on the client (applies to all requests unless overridden)
$client = new \GuzzleHttp\Client([
'connect_timeout' => 2.0,
'timeout' => 10.0,
]);
// override for one call
$response = $client->request('GET', 'https://example.com', [
'timeout' => 3.0,
]);Related option you might want
read_timeout: timeout for individual read operations when streaming a response body (defaults to PHPdefault_socket_timeout). (docs.guzzlephp.org)
Practical note
Leaving timeouts at 0 (infinite) can cause requests to hang indefinitely; many projects recommend setting non-zero values in production. (docs.typo3.org)
Citations:
- 1: https://docs.guzzlephp.org/en/stable/request-options.html?utm_source=openai
- 2: https://docs.guzzlephp.org/en/stable/request-options.html?utm_source=openai
- 3: https://docs.guzzlephp.org/en/stable/request-options.html?utm_source=openai
- 4: https://docs.guzzlephp.org/en/stable/request-options.html?utm_source=openai
- 5: https://docs.typo3.org/c/typo3/cms-core/main/en-us/Changelog/10.4.x/Important-92655-MakeRequestTimeoutConfigurableForLinkvalidator.html?utm_source=openai
Add client timeouts to prevent indefinite network blocking.
The SDK client is created without timeout or connect_timeout options, allowing stalled upstream calls to block callers indefinitely. GuzzleHTTP supports these as constructor options and defaults to infinite wait time when unset.
🛡️ Proposed fix
$this->config = array_merge(
[
'apiUrl' => 'https://api.lingo.dev',
'batchSize' => 25,
- 'idealBatchItemSize' => 250
+ 'idealBatchItemSize' => 250,
+ 'timeout' => 30.0,
+ 'connectTimeout' => 10.0
], $config
);
@@
+ if (!is_numeric($this->config['timeout']) || $this->config['timeout'] <= 0) {
+ throw new \InvalidArgumentException('Timeout must be a positive number');
+ }
+
+ if (!is_numeric($this->config['connectTimeout']) || $this->config['connectTimeout'] <= 0) {
+ throw new \InvalidArgumentException('Connect timeout must be a positive number');
+ }
+
$this->_httpClient = new Client(
[
'base_uri' => $this->config['apiUrl'],
'headers' => [
'Content-Type' => 'application/json; charset=utf-8',
'X-API-Key' => $this->config['apiKey']
- ]
+ ],
+ 'timeout' => (float)$this->config['timeout'],
+ 'connect_timeout' => (float)$this->config['connectTimeout']
]
);Also applies to: 88-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LingoDotDevEngine.php` around lines 58 - 63, The client lacks default
timeouts: update the config merge that sets $this->config (the array_merge
block) to include sensible defaults such as 'timeout' and 'connect_timeout'
(e.g. timeout => 10, connect_timeout => 5) and ensure these keys are passed
through to the Guzzle client constructor where the Client is instantiated (the
new Client(...) usage around the later block referenced 88-96) so callers cannot
block indefinitely; preserve any user-supplied overrides from $config while
adding these defaults.
src/LingoDotDevEngine.php
Outdated
Comment on lines
+70
to
+72
| if (empty($this->config['engineId'])) { | ||
| throw new \InvalidArgumentException('Engine ID is required'); | ||
| } |
There was a problem hiding this comment.
Harden engineId validation before path construction.
engineId is only checked with empty(). Whitespace/reserved characters can still produce malformed request paths when concatenated into the URL.
🔒 Proposed fix
- if (empty($this->config['engineId'])) {
+ if (!is_string($this->config['engineId']) || trim($this->config['engineId']) === '') {
throw new \InvalidArgumentException('Engine ID is required');
}
+ $this->config['engineId'] = trim($this->config['engineId']);
@@
- $url = '/process/' . $this->config['engineId'] . '/localize';
+ $url = '/process/' . rawurlencode($this->config['engineId']) . '/localize';Also applies to: 163-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LingoDotDevEngine.php` around lines 70 - 72, The current check only uses
empty() on $this->config['engineId'] in LingoDotDevEngine and can allow
whitespace or reserved chars that break URL paths; replace that with a trimmed
value and strict validation (e.g., ensure $engineId =
trim($this->config['engineId']) and validate against an allowed character set
such as alphanumerics, hyphen, underscore via a regex) and throw
\InvalidArgumentException if it fails, and when constructing the request path
(the method that concatenates the engine ID into the URL) use proper
URL-encoding (rawurlencode) of the engine ID instead of direct concatenation;
apply the same validation/encoding at the other occurrence referenced (line
~163).
Comment on lines
41
to
52
| } catch (\Exception $e) { | ||
| echo "❌ Test failed!\n"; | ||
| echo "Error: " . $e->getMessage() . "\n"; | ||
|
|
||
| if ($e instanceof RequestException && $e->hasResponse()) { | ||
| $response = $e->getResponse(); | ||
| echo "Status Code: " . $response->getStatusCode() . "\n"; | ||
| echo "Response Body: " . $response->getBody() . "\n"; | ||
| } | ||
| $failed++; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and examine the relevant lines
if [ -f "test-all-methods.php" ]; then
echo "=== File found. Examining lines 35-55 for context ==="
sed -n '35,55p' test-all-methods.php | cat -n
else
echo "File not found at root. Searching for it..."
find . -name "test-all-methods.php" -type f
fiRepository: lingodotdev/sdk-php
Length of output: 915
🏁 Script executed:
# Run the exact grep commands suggested in the review
echo "=== Checking for Exception catches ==="
rg -n 'catch\s*\(\\Exception\s+\$e\)' test-all-methods.php || echo "No matches with exact pattern"
echo "=== Checking for Throwable catches ==="
rg -n 'catch\s*\(\\Throwable\s+\$e\)' test-all-methods.php || echo "No matches with exact pattern"
echo "=== Broader search for catch blocks ==="
rg -n 'catch\s*\(' test-all-methods.phpRepository: lingodotdev/sdk-php
Length of output: 276
🏁 Script executed:
# Check how runTest is called and if there's broader error handling
echo "=== Looking for runTest() calls ==="
rg -n 'runTest' test-all-methods.php
echo "=== Checking for set_error_handler or register_shutdown ==="
rg -n 'set_error_handler|register_shutdown_function|set_exception_handler' test-all-methods.php
echo "=== Looking for outer try-catch blocks ==="
rg -n 'try\s*{' test-all-methods.phpRepository: lingodotdev/sdk-php
Length of output: 816
🏁 Script executed:
# Check the full structure around the runTest calls to understand error handling
head -100 test-all-methods.php | tail -50Repository: lingodotdev/sdk-php
Length of output: 1474
🏁 Script executed:
# Check what comes after the catch block to see the summary logic
sed -n '50,80p' test-all-methods.php | cat -nRepository: lingodotdev/sdk-php
Length of output: 1162
🏁 Script executed:
# Check if the SDK might throw Errors or TypeErrors that wouldn't be caught
rg -n 'throw.*Error|TypeError' . --type php | head -20Repository: lingodotdev/sdk-php
Length of output: 307
Catch \Throwable to keep the harness failure-safe.
Right now, non-Exception errors (for example TypeError) bypass this block and can skip the final summary/exit logic.
🔧 Proposed fix
- } catch (\Exception $e) {
+ } catch (\Throwable $e) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (\Exception $e) { | |
| echo "❌ Test failed!\n"; | |
| echo "Error: " . $e->getMessage() . "\n"; | |
| if ($e instanceof RequestException && $e->hasResponse()) { | |
| $response = $e->getResponse(); | |
| echo "Status Code: " . $response->getStatusCode() . "\n"; | |
| echo "Response Body: " . $response->getBody() . "\n"; | |
| } | |
| $failed++; | |
| return false; | |
| } | |
| } catch (\Throwable $e) { | |
| echo "❌ Test failed!\n"; | |
| echo "Error: " . $e->getMessage() . "\n"; | |
| if ($e instanceof RequestException && $e->hasResponse()) { | |
| $response = $e->getResponse(); | |
| echo "Status Code: " . $response->getStatusCode() . "\n"; | |
| echo "Response Body: " . $response->getBody() . "\n"; | |
| } | |
| $failed++; | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-all-methods.php` around lines 41 - 52, Change the catch from catching
\Exception to catching \Throwable so fatal PHP errors (TypeError, ParseError,
etc.) are handled by the test harness: replace "catch (\Exception $e)" with
"catch (\Throwable $e)". Keep the existing RequestException-specific logic (the
instanceof RequestException & hasResponse() block) but guard it safely when $e
may not be an Exception (e.g., ensure the RequestException class is referenced
correctly or fully qualified as needed, so the block still runs for Guzzle's
RequestException).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/LingoDotDevEngine.php (1)
225-241:⚠️ Potential issue | 🟠 MajorChunking currently allows payloads to exceed configured word limits.
The record is added before the size check, so chunks can exceed
idealBatchItemSize(and README documents this as a max). This can create oversized requests.🔧 Proposed fix
- $currentChunkItemCount = 0; + $currentChunkItemCount = 0; + $currentChunkWordCount = 0; @@ - $currentChunk[$key] = $value; - $currentChunkItemCount++; - - $currentChunkSize = $this->_countWordsInRecord($currentChunk); - - if ($currentChunkSize > $this->config['idealBatchItemSize'] - || $currentChunkItemCount >= $this->config['batchSize'] - || $i === count($keys) - 1 - ) { - $result[] = $currentChunk; - $currentChunk = []; - $currentChunkItemCount = 0; - } + $itemWordCount = $this->_countWordsInRecord($value); + $wouldExceedWordLimit = !empty($currentChunk) + && ($currentChunkWordCount + $itemWordCount > $this->config['idealBatchItemSize']); + $wouldExceedBatchLimit = $currentChunkItemCount >= $this->config['batchSize']; + + if ($wouldExceedWordLimit || $wouldExceedBatchLimit) { + $result[] = $currentChunk; + $currentChunk = []; + $currentChunkItemCount = 0; + $currentChunkWordCount = 0; + } + + $currentChunk[$key] = $value; + $currentChunkItemCount++; + $currentChunkWordCount += $itemWordCount; + + if ($i === count($keys) - 1) { + $result[] = $currentChunk; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/LingoDotDevEngine.php` around lines 225 - 241, In LingoDotDevEngine's chunking loop the code adds $value to $currentChunk before checking size, allowing chunks to exceed config['idealBatchItemSize']; change the logic in the loop that iterates $keys so you compute the size impact before pushing the item: call _countWordsInRecord (or compute projected size) for the currentChunk plus the candidate $value and if that projected size would exceed config['idealBatchItemSize'] or currentChunkItemCount would hit config['batchSize'] then push the existing $currentChunk to $result and reset it before adding the new key/value; ensure the edge-case where a single payload entry itself exceeds idealBatchItemSize is handled by allowing it to be placed in its own chunk (i.e., if currentChunk is empty still add the oversized item and then push).
🧹 Nitpick comments (1)
tests/LingoDotDevEngineTest.php (1)
40-63: Consider consolidating mock-engine setup helpers to reduce duplication.Both helpers repeat client construction + reflection injection. A single helper with optional history wiring would simplify maintenance.
Also applies to: 74-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/LingoDotDevEngineTest.php` around lines 40 - 63, Consolidate the duplicated test setup by replacing _createMockEngine and the similar helper at lines 74-100 with a single helper (e.g., createMockEngine) that accepts $responses and an optional &$history/container parameter; inside the helper create the MockHandler and HandlerStack, and if a history container is provided, push Guzzle's Middleware::history($container) onto the HandlerStack before constructing the Client, then inject the Client into the LingoDotDevEngine instance via ReflectionClass targeting the _httpClient property (setAccessible(true) and setValue), and return the engine (and set the passed-in history container) so tests can optionally capture request history without duplicating client/reflection wiring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/LingoDotDevEngine.php`:
- Around line 225-241: In LingoDotDevEngine's chunking loop the code adds $value
to $currentChunk before checking size, allowing chunks to exceed
config['idealBatchItemSize']; change the logic in the loop that iterates $keys
so you compute the size impact before pushing the item: call _countWordsInRecord
(or compute projected size) for the currentChunk plus the candidate $value and
if that projected size would exceed config['idealBatchItemSize'] or
currentChunkItemCount would hit config['batchSize'] then push the existing
$currentChunk to $result and reset it before adding the new key/value; ensure
the edge-case where a single payload entry itself exceeds idealBatchItemSize is
handled by allowing it to be placed in its own chunk (i.e., if currentChunk is
empty still add the oversized item and then push).
---
Nitpick comments:
In `@tests/LingoDotDevEngineTest.php`:
- Around line 40-63: Consolidate the duplicated test setup by replacing
_createMockEngine and the similar helper at lines 74-100 with a single helper
(e.g., createMockEngine) that accepts $responses and an optional
&$history/container parameter; inside the helper create the MockHandler and
HandlerStack, and if a history container is provided, push Guzzle's
Middleware::history($container) onto the HandlerStack before constructing the
Client, then inject the Client into the LingoDotDevEngine instance via
ReflectionClass targeting the _httpClient property (setAccessible(true) and
setValue), and return the engine (and set the passed-in history container) so
tests can optionally capture request history without duplicating
client/reflection wiring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb35cca8-2ccf-47be-9c16-4b51d330a9de
📒 Files selected for processing (4)
README.mdsrc/LingoDotDevEngine.phptest-all-methods.phptests/LingoDotDevEngineTest.php
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
1 participant
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.
Summary
engineIdis now an optional configuration parameter (moved from URL path to request body)/process/localizeendpoint (replaces/process/{engineId}/localize)Changes
src/LingoDotDevEngine.phpengineIdvalidation — it is now optionalPOST /process/localize(replacesPOST /process/{engineId}/localize)engineIdis included in request body only when provided in configX-API-Key: {apiKey}sourceLocale,targetLocale)sessionId(generated once per engine instance) included in request bodytests/LingoDotDevEngineTest.phptestConstructorRequiresEngineId(no longer required)testConstructorWithoutEngineId— verifies engine creation with onlyapiKeytestLocalizeChunkUrlAndBody— URL assertion changed to/process/localize, addedengineIdbody assertiontestLocalizeChunkUrlAndBodyWithoutEngineId— verifies body omitsengineIdwhen not configuredtest-all-methods.phpengineIdis now an optional CLI argumentapiUrlas optional third CLI argumentphp test-all-methods.php <api_key> [engine_id] [api_url]README.mdengineIdmarked as optional in configuration tableengineIdas optionalengineIdTest plan
engineIdprovidedengineIdSummary by CodeRabbit
New Features
Documentation
Chores