feat(php): invocation-only dynamic snippets + clientImport - #17452
feat(php): invocation-only dynamic snippets + clientImport#17452cadesark wants to merge 2 commits into
Conversation
![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 clientImport to the PHP invocation snippet response by rendering a throwaway reference to the root client class and harvesting its use statement. The mechanism works, but the clientVariableName option is now language-sigil-sensitive in PHP (callers must pass $foo, unlike C#'s foo), and the client-reference rendering is more indirect than it needs to be.
- 🟡 2 warning(s)
- 🔵 1 suggestion(s)
| }): php.MethodInvocation { | ||
| return php.invokeMethod({ | ||
| on: php.codeblock(CLIENT_VAR_NAME), | ||
| on: php.codeblock(clientVariableName ?? CLIENT_VAR_NAME), |
There was a problem hiding this comment.
🟡 warning
clientVariableName is passed through verbatim, so a caller must remember to include the $ for PHP (the test uses "$mailchimp") while the C# generator in this same PR takes a bare mailchimp. A shared Options field with per-language sigil rules is a footgun — passing "mailchimp" here silently emits invalid PHP. Normalize instead:
| on: php.codeblock(clientVariableName ?? CLIENT_VAR_NAME), | |
| on: php.codeblock(clientVariableName ? (clientVariableName.startsWith("$") ? clientVariableName : `$${clientVariableName}`) : CLIENT_VAR_NAME), |
| const clientReference = php.codeblock((writer) => { | ||
| writer.writeNode( | ||
| php.classReference({ | ||
| name: this.context.getRootClientClassName(), | ||
| namespace: this.context.rootNamespace | ||
| }) | ||
| ); | ||
| }); | ||
| const { imports: clientImport } = clientReference.toStringWithoutImports({ | ||
| namespace: SNIPPET_NAMESPACE, | ||
| rootNamespace: SNIPPET_NAMESPACE, | ||
| customConfig: this.context.customConfig ?? {} | ||
| }); |
There was a problem hiding this comment.
🔵 suggestion
php.classReference(...) is itself an AstNode, so the codeblock wrapper (and the discarded code) is unnecessary indirection. Also worth hoisting getRootClientClassName() since it's called again on line 129.
| const clientReference = php.codeblock((writer) => { | |
| writer.writeNode( | |
| php.classReference({ | |
| name: this.context.getRootClientClassName(), | |
| namespace: this.context.rootNamespace | |
| }) | |
| ); | |
| }); | |
| const { imports: clientImport } = clientReference.toStringWithoutImports({ | |
| namespace: SNIPPET_NAMESPACE, | |
| rootNamespace: SNIPPET_NAMESPACE, | |
| customConfig: this.context.customConfig ?? {} | |
| }); | |
| const clientImportOnly = php.classReference({ | |
| name: this.context.getRootClientClassName(), | |
| namespace: this.context.rootNamespace | |
| }); | |
| const { imports: clientImport } = clientImportOnly.toStringWithoutImports({ | |
| namespace: SNIPPET_NAMESPACE, | |
| rootNamespace: SNIPPET_NAMESPACE, | |
| customConfig: this.context.customConfig ?? {} | |
| }); |
| const body = writer.toString(true); | ||
| const imports = writer.importsToString() ?? ""; | ||
| return { | ||
| code: formatter != null ? formatter.formatSync(body) : body, |
There was a problem hiding this comment.
🟡 warning
formatter.formatSync(body) — does AbstractFormatter expose a sync variant? Every other call site in this file uses the async format(...). If formatSync doesn't exist this won't compile; if it does, note that imports is returned unformatted while code is formatted, which can produce inconsistent indentation when the caller concatenates them.
Mirrors the finalized TypeScript structured contract (PR #17393) and the Python (#17402) / Go (#17403) / Java (#17404) / C# (#17405) ports: alongside the full snippet, the generator now returns InvocationSnippetResponse = { snippet, imports, clientName, errors } for callers (e.g. docs code templates) that render the invocation inside code they already own. - snippet: the bare call (honoring options.clientVariableName), no client construction, no `<?php` prefix or `namespace ...;` header, and no trailing `;`. - imports: the PHP `use ...;` block the call references, captured separately via a new AstNode.toStringWithoutImports helper (backed by Writer.importsToString) — the PHP analogue of the TS AST's toStringWithoutImports and the C#/Java AST helpers. Empty string when the call needs no imports; populated when the invocation constructs types from another namespace inline (e.g. an inlined request class or a DateTime body value). - clientName: the generated client class name (context.getRootClientClassName()). - errors: preserved from the existing error reporter. Co-Authored-By: Claude <noreply@anthropic.com>
Populates the optional clientImport field on InvocationSnippetResponse for the PHP dynamic-snippets invocation generator, mirroring the TypeScript port. Renders a bare reference to the generated root client class and captures the `use ...;` statement it requires via AstNode.toStringWithoutImports (the same helper used for the existing `imports` field). Distinct from `imports`, which only carries the imports the bare call itself references. Empty string when the client needs no import. Lets docs render the client import (e.g. `use Acme\AcmeClient;`) without hand-authoring it. Co-Authored-By: Claude <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1531d98 to
907806e
Compare
cadesark
changed the title
1 participant
Summary
Populates the optional
clientImportfield onInvocationSnippetResponsefor the PHP dynamic-snippets invocation generator, mirroring the TypeScript port on the base branch.The PHP generator now renders a bare reference to the generated root client class and captures the
use ...;statement it requires viaAstNode.toStringWithoutImports— the same helper that already backs the existingimportsfield. This is distinct fromimports, which only carries imports the bare call itself references;clientImportis the client's own import. Empty string when the client needs no import. This lets docs render the client import (e.g.use Acme\AcmeClient;) without hand-authoring it.Changes
generators/php/dynamic-snippets/src/EndpointSnippetGenerator.ts: build a bare root-client-class reference and setclientImportfrom its rendereduse ...;block.generators/php/dynamic-snippets/src/__test__/InvocationSnippet.test.ts: assertclientImport === "use Acme\\AcmeClient;".Testing
pnpm turbo run compile --filter @fern-api/php-dynamic-snippets— passes.clientImportassertion). No snapshot churn.Generated with Claude Code