feat(php): support server URL variables (region/edge routing)#16982
feat(php): support server URL variables (region/edge routing)#16982devin-ai-integration[bot] wants to merge 5 commits into
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![nitpickybot[bot]](https://avatars.githubusercontent.com/in/2901896?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Adds PHP server URL variable support. Core logic is sound, but the reserved-name de-collision has a gap and the docs generation could leak into a broken URL for edge cases. Main functional concern is that the interpolation condition and default-filling logic can produce incorrect URLs when only some variables are provided.
- 🟡 2 warning(s)
- 🔵 1 suggestion(s)
| const optionName = RESERVED_OPTION_NAMES.has(camel) | ||
| ? caseConverter.camelSafe(`server url ${getOriginalName(variable.name)}`) | ||
| : camel; |
There was a problem hiding this comment.
🟡 warning
De-collision only checks against RESERVED_OPTION_NAMES, not against other server-variable option names. If two variables camelCase to the same name (e.g. server url and serverUrl), or if a variable is literally named server url environment, you can produce duplicate constructor parameter names and generate invalid PHP. Consider tracking already-emitted optionNames and suffixing on collision.
| switch (environments.type) { | ||
| case "singleBaseUrl": { | ||
| const templatedEnvironment = environments.environments.find((env) => env.urlTemplate != null); | ||
| if (templatedEnvironment?.urlTemplate == null) { | ||
| return; | ||
| } | ||
| const phpString = urlTemplateToPhpString(templatedEnvironment.urlTemplate, serverVariableOptions); | ||
| writer.controlFlow("if", php.codeblock(condition)); | ||
| writeDefaults(); | ||
| writer.writeTextStatement(`$this->${this.context.getClientOptionsName()}['baseUrl'] = ${phpString}`); | ||
| writer.endControlFlow(); | ||
| writer.writeLine(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 warning
The singleBaseUrl branch reads urlTemplate from the first environment that has one, but the client may be constructed with a different $environment. If multiple environments have distinct templates, interpolation always uses the first template's host regardless of which environment the user selected. The multipleBaseUrls branch has the same issue (picks first templated env). Worth confirming this matches TS/Python behavior — otherwise selecting a non-default environment silently gets the wrong base URL.
| for (const option of serverVariableOptions) { | ||
| const fallback = | ||
| option.variable.default != null ? this.escapeSingleQuoted(option.variable.default) : ""; | ||
| writer.writeTextStatement(`$${option.optionName} ??= '${fallback}'`); |
There was a problem hiding this comment.
🔵 suggestion
When a variable has no default, $region ??= '' substitutes an empty string into the URL, producing a malformed URL like https://api..example.com. Since these are exposed as optional params, a user setting only one of several variables will silently get a broken URL for the others. Consider requiring all-or-nothing, or documenting that variables without defaults are effectively required together.
SDK Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Docs Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on
Docs generation runs |
…server-url-variables
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Description
Linear ticket: Refs
Adds OpenAPI server URL variable support to the PHP SDK generator for region/edge routing. Server variables become optional constructor parameters and are interpolated into single- or multi-base URLs, using the IR defaults when omitted. Allowed values remain documentation-only, matching the Java and Python generators.
Changes Made
EnvironmentsConfig, preserving static URLs where a multi-base URL has no template.$this->options['baseUrl']orEnvironments::custom(...)when any server-variable option is supplied.serverUrlplus numeric suffixes.featchangelog entry.Testing
pnpm turbo run test --filter @fern-api/php-sdk(2 files, 11 tests)pnpm compile(160/160 tasks)pnpm lint:biomeandpnpm format:checkserver-url-templatingfixtures passed with--local --skip-scripts; regeneration produced no source snapshot changesfernapi/php-seed(PHP 8.4) — syntax, PHPUnit (75 tests / 284 assertions per fixture), and PHPStan passed for both fixturesLink to Devin session: https://app.devin.ai/sessions/2670cdc59f3f40a786bf824bcf5f3916
Requested by: @cadesark