Conversation
📝 WalkthroughWalkthroughDefault platform initialization was changed to use Config::getParam('platform', []) when payload.platform is absent. Several places now compute a protocol (http vs https) from _APP_OPTIONS_FORCE_HTTPS and derive an API endpoint as "{protocol}://{platform['apiHostname']}/v1". Code that previously used platform['endpoint'] was updated to use this computed endpoint (affecting function/site environment variables, messaging image URLs, and worker/function execution contexts). The platform resource registration was simplified to return Config::getParam('platform', []) directly (removed Request-based resolver logic). Build-related code also adds several deployment/project/runtime/capacity environment variables for functions and sites. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Workers/Functions.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
✨ Benchmark results
⚡ Benchmark Comparison
|
loks0n
force-pushed
the
fix-fallback-platfor
branch
from
fafc223 to
3c3aa6d
Compare
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 (1)
app/controllers/api/messaging.php (1)
3538-3577: Fix undefined$currentScheduledAtwhen building push image URLIn this create‑push path,
$scheduleTime = $currentScheduledAt ?? $scheduledAt;uses$currentScheduledAt, which is never defined in this action, causing a PHP notice and relying on undefined‑var behavior. For new messages there is no prior schedule anyway, so you can just use$scheduledAthere; the new$protocol/$endpointlogic looks fine.- $scheduleTime = $currentScheduledAt ?? $scheduledAt; + $scheduleTime = $scheduledAt;
♻️ Duplicate comments (2)
app/controllers/general.php (1)
461-462: Add validation for the 'apiHostname' key (duplicate issue).This code has the same undefined array key issue as identified in
src/Appwrite/Platform/Workers/Functions.php. Line 462 accesses$platform['apiHostname']without validation, which could cause a runtime error if the key is missing.Apply the same defensive validation as suggested for the Functions.php file:
$protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') == 'disabled' ? 'http' : 'https'; -$endpoint = "$protocol://{$platform['apiHostname']}/v1"; +$endpoint = "$protocol://{$platform['apiHostname'] ?? 'localhost'}/v1";src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php (1)
371-372: Add validation for the 'apiHostname' key (duplicate issue).This code has the same undefined array key issue as identified in the other reviewed files. Line 372 accesses
$platform['apiHostname']without validation.Apply the same defensive validation:
$protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') == 'disabled' ? 'http' : 'https'; -$endpoint = "$protocol://{$platform['apiHostname']}/v1"; +$endpoint = "$protocol://{$platform['apiHostname'] ?? 'localhost'}/v1";
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
639-671: Consider guarding$platform['apiHostname']when computing$endpointThe new
$protocol/$endpointlogic and the added APPWRITE_* vars look good, but$endpoint = "$protocol://{$platform['apiHostname']}/v1";assumesapiHostnameis always present in the payload‑provided$platform. If a build message ever arrives without that field (e.g., from an older producer or misconfigured sender), you’ll get notices and malformedAPPWRITE_*_API_ENDPOINTvalues.To make this more robust and match the “fallback platform” goal of the PR, consider falling back to the config platform when
$platformis empty or missingapiHostname:- $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') == 'disabled' ? 'http' : 'https'; - $endpoint = "$protocol://{$platform['apiHostname']}/v1"; + $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') == 'disabled' ? 'http' : 'https'; + $platformConfig = $platform ?: Config::getParam('platform', []); + $apiHostname = $platformConfig['apiHostname'] ?? null; + if (empty($apiHostname)) { + throw new \Exception('Platform apiHostname is not configured'); + } + $endpoint = "$protocol://{$apiHostname}/v1";This keeps behavior for well‑formed payloads while failing fast (or you could choose a silent fallback) when platform data is missing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/controllers/api/messaging.php(3 hunks)app/controllers/general.php(2 hunks)app/init/resources.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(2 hunks)src/Appwrite/Platform/Workers/Functions.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Workers/Functions.php (1)
src/Appwrite/Event/Event.php (1)
getParam(319-322)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: scan
🔇 Additional comments (4)
app/init/resources.php (1)
167-169: LGTM! Clean refactor of platform resource.The platform resource now returns static configuration directly from
Config::getParam('platform', []), removing the previous Request-based endpoint construction logic. This simplifies the resolver and properly delegates dynamic endpoint computation to the consumers (as seen in the other files where protocol and endpoint are computed based on_APP_OPTIONS_FORCE_HTTPS).src/Appwrite/Platform/Workers/Functions.php (2)
90-90: Good fix for the type mismatch.The fallback now correctly defaults to
Config::getParam('platform', [])instead of an empty string, aligning with thearray $platformparameter type at line 353.
494-495: The code at lines 494-495 is safe and does not require validation. TheapiHostnamekey is guaranteed to exist in the platform configuration array, which is statically defined inapp/config/platform.phpwith a fallback default value of'localhost'.Likely an incorrect or invalid review comment.
app/controllers/api/messaging.php (1)
4566-4573: Endpoint derivation for push image update looks consistentDeriving
$protocolfrom_APP_OPTIONS_FORCE_HTTPSand buildingAPPWRITEstorage URLs via$endpoint = "$protocol://{$platform['apiHostname']}/v1"is consistent with the create‑push path and the rest of the PR; the updatedimage['url']construction is correct.
2 participants
No description provided.