feat: add PHP SDK#3235
Open
countradooku wants to merge 8 commits into
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3235 +/- ##
============================================
- Coverage 73.82% 73.78% -0.04%
Complexity 943 943
============================================
Files 1190 1190
Lines 107833 107833
Branches 84851 84867 +16
============================================
- Hits 79606 79563 -43
- Misses 25473 25484 +11
- Partials 2754 2786 +32
🚀 New features to boost your workflow:
|
The PHP extension client needs to build independently from the root Rust workspace while still testing against the in-tree Rust SDK. This adds the PHP binding package, Dockerized integration test path, and CI detection so PHP-only changes exercise the right checks. Constraint: Foreign SDK crates are kept outside the root workspace like the existing Python and C++ SDKs Constraint: PHP extension builds require php-config, cargo-php, and libclang in containerized CI Rejected: Leave PHP tests manual only | PHP-only PRs would not run SDK-specific validation Confidence: medium Scope-risk: moderate Directive: Keep foreign/php excluded from the root Cargo workspace unless the extension build is intentionally made workspace-compatible Tested: cargo fmt --manifest-path foreign/php/Cargo.toml --check; composer validate --working-dir foreign/php --strict; PHP syntax lint for tests; cargo metadata --manifest-path foreign/php/Cargo.toml; docker compose config; docker compose build php-tests; PHP integration smoke against apache/iggy:latest with 23 passed, 3 skipped, 0 failed Not-tested: Full docker compose test with source-built Iggy server after php-tests image build
The PHP and Python SDK integration suites depend on a server container that can be checked for readiness and reached from sibling containers. The previous healthcheck hit an authenticated HTTP endpoint with curl, while the runtime image did not install curl and the default server binds loopback-only addresses. The compose files now start the server with default root credentials, bind service ports to all interfaces, and use the bundled CLI ping for readiness. Constraint: Runtime image should not need extra healthcheck packages just for foreign SDK tests Constraint: SDK test containers connect through the Docker service name, not the server container loopback interface Rejected: Install curl and keep /stats healthcheck | /stats requires authentication and does not prove TCP SDK readiness Confidence: high Scope-risk: narrow Directive: Keep foreign SDK compose server bind addresses aligned with client container hostnames Tested: docker compose config for PHP and Python test files; git diff --check Not-tested: Full Docker build/test run locally because Docker daemon was unavailable
hubcio
requested changes
May 12, 2026
Contributor
|
I would recommend removing the cargo.lock file from version control. The python and cpp bindings also do not commit the lock files. Maybe also consider splitting this into two PRs - one for the blocking client and one for the async client. That'll reduce the LoC and make it easier for people to review. |
The PHP SDK addition was too large and carried an async bridge plus Docker-heavy CI path in the first review. This narrows the PR to the blocking client, removes async-only dependencies and lockfiles, and moves PHP checks into a language-specific pre-merge action that reports into the existing CI status gate. Constraint: Reviewer requested the async client be split out to reduce LoC and review risk Constraint: PHP CI must avoid the previous 22 minute Docker Compose path Rejected: Keep async client in this PR | too much surface area and an extra php-tokio dependency for initial review Rejected: Keep inline PHP workflow steps | inconsistent with other SDK CI actions and missed status aggregation Confidence: high Scope-risk: moderate Directive: Reintroduce IggyAsyncClient in a separate PR after the blocking client and CI path settle Tested: Ruby YAML parse for workflows/actions; PHP syntax checks; composer.json JSON parse; cargo fmt --manifest-path foreign/php/Cargo.toml -- --check; Docker PHP test image build Not-tested: Full GitHub Actions runtime duration
The Python test compose adjustment was unrelated to the PHP SDK review and made the PR harder to reason about. Restore it to master so this branch only carries the PHP SDK and its CI wiring. Constraint: Review feedback is focused on PHP SDK size and CI validity Rejected: Keep Python compose reachability tweak | unrelated to this PR and should be handled separately if still needed Confidence: high Scope-risk: narrow Directive: Do not mix foreign SDK compose changes into the PHP SDK PR unless they are required by PHP tests Tested: git diff confirmed foreign/python/docker-compose.test.yml matches master Not-tested: Python Docker Compose runtime
Author
The PHP test action wrote IGGY_PHP_EXTENSION through GITHUB_ENV before starting the Iggy server. Server config validation treats every IGGY_* variable as typed configuration and panicked on the unknown extension path variable, so the PHP test job never reached the test runner. Constraint: Iggy server rejects unknown IGGY_* environment variables Rejected: Add IGGY_PHP_EXTENSION to server ignored vars | the variable is test-runner specific and should not enter server config Confidence: high Scope-risk: narrow Directive: Keep PHP test harness variables outside the IGGY_* namespace unless they are real server configuration Tested: bash -n foreign/php/scripts/test.sh; Ruby YAML parse for .github/actions/php/pre-merge/action.yml Not-tested: Full GitHub Actions rerun
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an experimental PHP SDK under
foreign/php, implemented as a Rust-backed PHP extension over the in-tree Iggy Rust SDK.The PR includes:
IggyClientbindingsforeign/php, matching the existing foreign SDK patternValidation
Ran the following locally:
cargo fmt --manifest-path foreign/php/Cargo.toml --checkcomposer validate --working-dir foreign/php --strictforeign/php/tests/*.phpcargo metadata --manifest-path foreign/php/Cargo.toml --no-deps --format-version 1docker compose -f foreign/php/docker-compose.test.yml configdocker compose -f foreign/php/docker-compose.test.yml build php-testsextension_loaded("iggy-php") === trueapache/iggy:latest:23 passed, 3 skipped, 0 failedNotes
The full compose test path is present, but I only completed the
php-testsimage build plus an integration smoke against an already-running healthy Iggy container. TLS tests are opt-in and skipped unlessIGGY_TLS_CONNECTION_STRING/IGGY_TLS_PLAINTEXT_ADDRESSare set.