fix: php composer test resulting in error code 1

[ChiragAgg5k]

Summary by CodeRabbit

• Tests

  • CI PHP build now runs the project's automated test suite.

• Chores

  • Tightened PHP testing dependency version constraint.

• Bug Fixes

  • Generated PHP client: multipart/form-data methods no longer include an explicit onProgress parameter.
  • File upload flow: non-streaming uploads now read chunk data from the request parameter source.

• Compatibility

  • Generated PHP client adjusts parameter names that conflict with PHP reserved words.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

CI workflow for PHP now runs composer test instead of php -l. The PHP Composer template pins mockery/mockery to 1.6.12 (removed the caret). Generated PHP service method signatures use a new {{ method | methodParameters }} placeholder instead of per-parameter inline generation, removing the explicit multipart/form-data onProgress parameter. The PHP generator adds escapeKeyword and getMethodParameters helpers and registers a methodParameters Twig filter. The file upload request template falls back to reading the current parameter's data via ${{ parameter.name | caseCamel }}->getData() when no file handle exists. A public escapeKeyword method was added to the Language base class and SDK now delegates keyword escaping to the language implementation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly addresses the main change: replacing PHP linting with composer test to fix error code 1 behavior.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-composer-test

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 131649c and b8470cc.

📒 Files selected for processing (3)

• .github/workflows/sdk-build-validation.yml
• templates/php/composer.json.twig
• templates/php/src/Services/Service.php.twig

⏰ 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). (19)

• GitHub Check: build (8.3, Node18)
• GitHub Check: build (8.3, AppleSwift56)
• GitHub Check: build (8.3, Swift56)
• GitHub Check: build (8.3, Node20)
• GitHub Check: build (8.3, Python313)
• GitHub Check: build (8.3, FlutterStable)
• GitHub Check: build (8.3, DartBeta)
• GitHub Check: build (8.3, Android5Java17)
• GitHub Check: build (8.3, Python311)
• GitHub Check: build (8.3, KotlinJava17)
• GitHub Check: build (8.3, Android14Java17)
• GitHub Check: build (8.3, DartStable)
• GitHub Check: kotlin (server)
• GitHub Check: go (server)
• GitHub Check: cli (console)
• GitHub Check: flutter (client)
• GitHub Check: apple (client)
• GitHub Check: swift (server)
• GitHub Check: android (client)

🔇 Additional comments (2)

.github/workflows/sdk-build-validation.yml (1)

175-176: LGTM! Enhanced validation from linting to testing.

The change from php -l (syntax linting) to composer test (running the full PHPUnit test suite) provides more comprehensive validation of the generated PHP SDK. This aligns with the PR objective of fixing composer test errors and ensures that the generated code is not only syntactically correct but also functionally tested.

templates/php/composer.json.twig (1)

25-25: Downgrade is appropriate; the 1.6.x constraint is justified.

Mockery 1.6.8+ includes PHPUnit 10+ attribute support, ensuring full compatibility with phpunit/phpunit: ^10. The constraint 1.6.* properly pins to the stable 1.6.x series, which is more conservative than ^1.6.12 that would allow future major versions. No compatibility issues with phpunit 10 exist in the 1.6.x range.

[Copilot]

Pull request overview

This PR fixes PHP composer test failures by addressing type declaration issues in generated service method signatures. The main fix is making the onProgress callback parameter properly nullable to support modern PHP type checking (PHP 8.0+), along with refactoring the method signature generation logic and correcting a variable reference bug.

Key changes:

• Refactored method signature generation to use a centralized getMethodParameters method with proper nullable type support for the onProgress callback
• Corrected hardcoded variable reference from $file to the actual parameter name in file upload template
• Updated Mockery dependency version constraint and changed PHP build validation from syntax check to running full test suite

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
templates/php/src/Services/Service.php.twig	Replaced inline parameter generation with centralized methodParameters filter for cleaner, more maintainable code
templates/php/composer.json.twig	Changed Mockery version constraint from ^1.6.12 to 1.6.*
templates/php/base/requests/file.twig	Fixed hardcoded $file variable to use the actual parameter name from template context
src/SDK/Language/PHP.php	Added escapeKeyword and getMethodParameters methods, plus methodParameters filter for generating properly typed method signatures
.github/workflows/sdk-build-validation.yml	Updated PHP validation to run composer test instead of just syntax checking

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

templates/php/composer.json.twig (1)

25-25: Consider using ^1.6.12 instead of exact version pinning.

Pinning to exact version 1.6.12 prevents all updates, including patch releases that may contain bug fixes and security patches. This creates maintenance burden and potential security risks.

Using ^1.6.12 allows patch updates (e.g., 1.6.13, 1.6.14) while maintaining compatibility, which is the standard practice for dev dependencies.

🔎 Recommended change

-        "mockery/mockery": "1.6.12"
+        "mockery/mockery": "^1.6.12"

🧹 Nitpick comments (1)

src/SDK/Language.php (1)

161-174: Consider using strict comparison in in_array().

The keyword escape logic is correct. However, using strict comparison (in_array($value, $this->getKeywords(), true)) would be more defensive against edge cases involving type coercion.

🔎 Proposed refinement

 public function escapeKeyword(string $value): string
 {
-    if (in_array($value, $this->getKeywords())) {
+    if (in_array($value, $this->getKeywords(), true)) {
         return 'x' . $value;
     }

     return $value;
 }

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 10b6207 and ead79e8.

📒 Files selected for processing (4)

• src/SDK/Language.php
• src/SDK/Language/PHP.php
• src/SDK/SDK.php
• templates/php/composer.json.twig

🚧 Files skipped from review as they are similar to previous changes (1)

• src/SDK/Language/PHP.php

🧰 Additional context used

🧬 Code graph analysis (2)

src/SDK/SDK.php (1)

src/SDK/Language.php (1)

• escapeKeyword (167-174)

src/SDK/Language.php (9)

src/SDK/Language/PHP.php (1)

• getKeywords (53-123)

src/SDK/Language/DotNet.php (1)

• getKeywords (24-136)

src/SDK/Language/Go.php (1)

• getKeywords (23-45)

src/SDK/Language/Dart.php (1)

• getKeywords (41-108)

src/SDK/Language/JS.php (1)

• getKeywords (42-111)

src/SDK/Language/Python.php (1)

• getKeywords (39-77)

src/SDK/Language/Ruby.php (1)

• getKeywords (38-79)

src/SDK/Language/Kotlin.php (1)

• getKeywords (23-92)

src/SDK/Language/Swift.php (1)

• getKeywords (23-92)

🔇 Additional comments (1)

src/SDK/SDK.php (1)

191-193: LGTM! Good refactoring to delegate keyword escaping.

The change to delegate keyword escaping to the language-specific implementation ($language->escapeKeyword($value)) improves maintainability and follows the Single Responsibility Principle. Language-specific logic is now properly encapsulated in the Language class.