Skip to content

fix(php,csharp): order parameters with default values after required parameters#16005

Open
davidkonigsberg wants to merge 6 commits into
mainfrom
devin/1779277415-fix-php-param-ordering
Open

fix(php,csharp): order parameters with default values after required parameters#16005
davidkonigsberg wants to merge 6 commits into
mainfrom
devin/1779277415-fix-php-param-ordering

Conversation

@davidkonigsberg
Copy link
Copy Markdown
Contributor

@davidkonigsberg davidkonigsberg commented May 20, 2026

Description

Refs #15998

PR #15998 (feat(openapi): Add better support for x-fern-base-path) introduced path parameters with clientDefault values (e.g. apiVersion = 'v1beta'). This caused two issues:

  1. PHP SDK: Generated method signatures placed required parameters after parameters with default values, triggering PHPStan's PHP 8.0 deprecation: Required parameter $request follows optional parameter $apiVersion
  2. C# SDK: Dynamic snippets passed positional arguments in the wrong order, causing type mismatch compilation errors
  3. Both generators: Dynamic snippets emitted positional args in the old order even after the method signature was corrected

Changes Made

  • generators/php/codegen/src/ast/Method.ts: Updated parameter ordering logic to also treat parameters with an initializer (default value) as optional, placing them after required parameters in the generated method signature
  • generators/php/dynamic-snippets/src/EndpointSnippetGenerator.ts: Separated root-level path parameters (which may have defaults from x-fern-base-path) from endpoint-specific path parameters, placing root-level params after the body argument to match the corrected method signature ordering
  • generators/csharp/dynamic-snippets/src/EndpointSnippetGenerator.ts: Applied the same root/endpoint path parameter separation for both getMethodArgsForBodyRequest and getMethodArgsForInlinedRequest, matching AbstractEndpointGenerator's ordering of [...requiredPathParams, requestParam, ...optionalPathParams]
  • Updated seed snapshots for both php-sdk and csharp-sdk api-wide-base-path-with-default fixtures
  • Added changelog entries for both PHP and C# SDK generators

Testing

  • Seed test php-sdk --fixture api-wide-base-path-with-default passes
  • Seed test csharp-sdk --fixture api-wide-base-path-with-default passes
  • Verified generated PHP signature: create(Widget $request, string $apiVersion = 'v1beta', ?array $options = null)
  • Verified generated C# snippet: CreateAsync(new Widget { Name = "name" }, "v1beta")
  • Verified generated PHP snippet: create(new Widget([...]), 'v1beta')

Link to Devin session: https://app.devin.ai/sessions/8c891e927684448ea73ec4c26f9cd87a
Requested by: @davidkonigsberg

Co-Authored-By: David Konigsberg <davidakonigsberg@gmail.com>
@devin-ai-integration
Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

SDK Generation Benchmark Results

Comparing PR branch against median of 5 nightly run(s) on main (latest: 2026-05-20T05:20:23Z).

Full benchmark table (click to expand)
Generator Spec main (generator) main (E2E) PR (generator) Delta
csharp-sdk square 67s (n=5) 105s (n=5) 68s +1s (+1.5%)
php-sdk square 54s (n=5) 81s (n=5) 56s +2s (+3.7%)

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 fern generate). main (E2E): full customer-observable time including build/test scripts (nightly baseline, informational). Delta is computed against generator-only baseline.
⚠️ = generation exited with a non-zero exit code (timing may not reflect a successful run).
Baseline from nightly runs on main (latest: 2026-05-20T05:20:23Z). Trigger benchmark-baseline to refresh.
Last updated: 2026-05-20 13:08 UTC

devin-ai-integration Bot and others added 3 commits May 20, 2026 12:20
Root-level path parameters (e.g. from x-fern-base-path) may have default
values, making them optional in the generated method signature. The dynamic
snippets generator now places these args after the body argument to match
the Method.ts parameter reordering (required before defaulted).

Co-Authored-By: David Konigsberg <davidakonigsberg@gmail.com>
Apply the same fix as PHP to C# dynamic snippets: root-level path
parameters (from x-fern-base-path with defaults) are placed after the
body argument to match AbstractEndpointGenerator parameter ordering.

Also fixes biome formatting in PHP dynamic snippets.

Co-Authored-By: David Konigsberg <davidakonigsberg@gmail.com>
Co-Authored-By: David Konigsberg <davidakonigsberg@gmail.com>
@devin-ai-integration devin-ai-integration Bot changed the title fix(php): order parameters with default values after required parameters fix(php,csharp): order parameters with default values after required parameters May 20, 2026
devin-ai-integration Bot and others added 2 commits May 20, 2026 12:50
…rrors

Co-Authored-By: David Konigsberg <davidakonigsberg@gmail.com>
Co-Authored-By: David Konigsberg <davidakonigsberg@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant