appwrite/sdk-for-php#44 - write warnings to error log instead of stdout #1176
Conversation
WalkthroughThe PHP client template at templates/php/src/Client.php.twig changes how warnings are emitted. Instead of printing warnings to stdout using echo (e.g., "Warning: ..."), the code now invokes \trigger_error($warning, E_USER_WARNING) for each warning extracted from x-<title>-warning. The iteration over warnings remains unchanged, and warnings are still processed individually (e.g., semicolon-separated values handled as before). No exported or public API signatures are modified. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
templates/php/src/Client.php.twig (2)
162-173: Header parsing lowercases values and drops duplicatesLowercasing the entire header alters values (e.g., warning text) and multiple headers overwrite earlier ones. Preserve case for values and join duplicates.
- curl_setopt($ch, CURLOPT_HEADERFUNCTION, function($curl, $header) use (&$responseHeaders) { - $len = strlen($header); - $header = explode(':', strtolower($header), 2); - if (count($header) < 2) { // ignore invalid headers - return $len; - } - $responseHeaders[strtolower(trim($header[0]))] = trim($header[1]); - return $len; - }); + curl_setopt($ch, CURLOPT_HEADERFUNCTION, function($curl, $rawHeader) use (&$responseHeaders) { + $len = strlen($rawHeader); + $parts = explode(':', $rawHeader, 2); + if (count($parts) < 2) { // ignore invalid headers + return $len; + } + $name = strtolower(trim($parts[0])); + $value = trim($parts[1]); + if (isset($responseHeaders[$name])) { + $responseHeaders[$name] .= ', ' . $value; + } else { + $responseHeaders[$name] = $value; + } + return $len; + });
196-200: Content-Type parsing breaks when no ";" presentstrpos can return false, making substr(..., 0, 0) and missing JSON decode. Parse robustly.
- switch(substr($contentType, 0, strpos($contentType, ';'))) { + $mediaType = strtolower(trim(explode(';', $contentType, 2)[0] ?? '')); + switch ($mediaType) { case 'application/json': $responseBody = json_decode($responseBody, true); break; }
🧹 Nitpick comments (2)
templates/php/src/Client.php.twig (2)
152-161: Avoid mutating $headers while iteratingBuilding cURL headers by modifying the array in-place during foreach is brittle. Use a separate array.
- foreach ($headers as $i => $header) { - $headers[] = $i . ':' . $header; - unset($headers[$i]); - } + $curlHeaders = []; + foreach ($headers as $i => $header) { + $curlHeaders[] = $i . ':' . $header; + } ... - curl_setopt($ch, CURLOPT_HTTPHEADER, $headers); + curl_setopt($ch, CURLOPT_HTTPHEADER, $curlHeaders);
216-218: Guard missing Location headerPrevent an undefined index notice when Location is absent.
- return $responseHeaders['location']; + return $responseHeaders['location'] ?? '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
templates/php/src/Client.php.twig(1 hunks)
🔇 Additional comments (1)
templates/php/src/Client.php.twig (1)
190-194: Tighten warning handling: trim, sanitize, and avoid falsey skipUse a strict non-empty check, trim and filter out blank entries, replace any CR/LF in each warning, then call trigger_error. Confirm desired behavior if your environment converts warnings into exceptions.
- $warnings = $responseHeaders['x-{{spec.title | caseLower}}-warning'] ?? ''; - if ($warnings) { - foreach(explode(';', $warnings) as $warning) { - \trigger_error($warning, E_USER_WARNING); - } - } + $warnings = $responseHeaders['x-{{spec.title | caseLower}}-warning'] ?? ''; + if ($warnings !== '') { + foreach (array_filter(array_map('trim', explode(';', $warnings)), 'strlen') as $warning) { + $warning = preg_replace('/[\r\n]+/', ' ', $warning); + \trigger_error($warning, E_USER_WARNING); + } + }
What does this PR do?
This changes the way how API warnings are handled within the php client.
Originally the warning was directly sent to stdout and so could cause unwanted output within apps or websites, that may break json / html output etc.
This now uses the trigger_error method, so warnings will be sent to the target that is configured within the php installation.
Test Plan
I made a call to on of the current database endoints that contain a "deprecated" warnings message within the x-appwrite-waring header.
Related PRs and Issues
appwrite/sdk-for-php#44
Have you read the Contributing Guidelines on issues?
yes
Summary by CodeRabbit