Widen dependency versions by pkruithof · Pull Request #64 · Eppo-exp/php-sdk

pkruithof · 2026-07-23T07:33:13Z

🎟️ Fixes issue
📜 Design Doc: link if applicable

Motivation and Context

We're using Eppo with this library and want to upgrade to Symfony 8. Currently the cache dependency is preventing that.

Description

I've widened the requirement for this dependency, and fixed some low-hanging fruit I encountered while doing so:

remove composer.lock as it's not recommended for libraries
expanded the test suite so it tests this library with both the lowest and highest versions of the dependencies

How has this been documented?

How has this been tested?

running make test

pkruithof · 2026-07-23T07:33:45Z

    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        deps:


Ideally you'd want to add different PHP versions to this as well. I'll leave that out of scope for this PR though.

Not sure why there were no workflows triggered for this PR though, the file syntax seems ok and I don't see any errors in the actions tab.

pkruithof · 2026-07-23T07:36:25Z

 use Psr\Http\Client\ClientExceptionInterface;
 use Psr\Http\Client\ClientInterface;
 use Psr\Http\Message\RequestFactoryInterface;
-use Teapot\StatusCode\RFC\RFC7231;


Having a dependency just for readable http status codes, and enforcing that on all who install this library seems a bit wasteful. But if you insist on keeping this I can revert this, although in that case I would suggest using symfony/http-foundation as it's a far more common dependency.

pkruithof · 2026-07-29T08:45:11Z

@aarsilv you seem to be the most active maintainer here, could you take a look at this please?

aarsilv · 2026-08-10T11:37:57Z

Hey! Apologies for the delay this GitHub notification slipped through the cracks. Change seems reasonable at a high-level, but how come delete whole lockfile (vs. update)? I know composer doesn't use it but we use it for things like security scans.

Copilot

Pull request overview

This pull request updates the library’s dependency constraints to allow newer Symfony Cache versions (targeting Symfony 8 compatibility) and adjusts related tooling/tests to support working without a committed composer.lock.

Changes:

Widen symfony/cache constraint to include ^8.0 and remove the Teapot dependency used only for HTTP status constants.
Remove composer.lock from version control and add it to .gitignore.
Update CI and local test invocation to better support dependency-resolution testing.

Reviewed changes

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

Show a summary per file

File	Description
`composer.json`	Widens `symfony/cache` constraint and removes `shrikeh/teapot` requirement.
`composer.lock`	Deleted to align with library best practices (no committed lockfile).
`.gitignore`	Ignores `composer.lock` going forward.
`.github/workflows/run-tests.yml`	Runs dependency resolution via `composer update` and adds a dependency-mode matrix.
`Makefile`	Switches PHPUnit invocation to `./vendor/bin/phpunit`.
`src/API/APIRequestWrapper.php`	Removes Teapot constants and uses numeric HTTP status ranges.
`tests/API/APIRequestWrapperTest.php`	Removes Teapot constants and inlines HTTP status codes in tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

    - name: Install dependencies
-      run: composer install --prefer-dist --no-progress
+      run: composer update --prefer-dist --no-progress ${{ matrix.deps }}


        $redirectHeaders->setHeader(new Header('Location', $redirectLocation));

-        $redirectResponse = new Response(statusCode: RFC7231::MOVED_PERMANENTLY, headers: $redirectHeaders);
+        $redirectResponse = new Response(statusCode: 308, headers: $redirectHeaders);


bertilhatt · 2026-08-10T13:35:17Z

Feedback:

The composer.lock file should be added to the .gitignore, because this is a library and not a project.

Keep composer.lock and prove symfony/cache ^6.4|^7.0|^8.0 works. PR #64 widens that constraint for Symfony 8 support. It also deletes composer.lock. This branch keeps both the widening and the lockfile. #64 https://datadoghq.atlassian.net/browse/FFESUPPORT-934 Deleting the lock was a prerequisite for `composer update` in CI, not for the widening. `composer install` becomes one leg of a matrix instead of the only install mode. The update legs rewrite composer.lock inside the runner, and nothing is committed back. Why the lockfile stays: - GitHub builds the PHP dependency graph from composer.lock. That graph feeds our Dependabot alerts. FFESUPPORT-534, -734 and -887 all came from it. - `composer validate --strict` checks lock sync only when a lock exists. - config.platform.php guards the committed lock. It has no other purpose. Matrix legs and what each resolves: - 8.1, 8.3 locked -> symfony/cache 6.4.40 - 8.1 lowest -> psr/log 2.0.0, psr/cache 2.0.0, google/cloud-storage 1.30.0 - 8.3 highest -> symfony/cache 7.4.16 - 8.4 highest -> symfony/cache 8.1.4 The highest legs unset config.platform.php. The 8.1.0 pin caps the solver at symfony/cache 6.4, so both legs in PR #64 resolved 6.4.x. The ^7.0 and ^8.0 branches went untested. Also: - Add .github/dependabot.yml with grouped updates. Grouping cuts the lockfile churn that makes a lock expensive to keep. - Gate `composer audit` on --no-dev. All 6 current advisories are dev-only. - ramsey/composer-install keys its cache per leg. hashFiles('**/composer.lock') collapsed to a constant once the lock was gone. - Restore 301 in the redirect test. 308 takes a different path in the decorator. composer.lock changes by the Teapot removal only: -105 lines, no version churn. Verified: 98 tests, 664 assertions pass. CI run 31860944824 passed all five legs and produced the resolutions above. codex reports no blocking issues. Out of scope: the 6 dev-only advisories, and test-package.yml running on every branch creation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

aarsilv · 2026-08-17T21:49:07Z

Hello @pkruithof! I took a stab folding this into #67

Love that you're widening symfony/cache to ^6.4|^7.0|^8.0 and dropping shrikeh/teapot.

Dropping composer.lock is the standard advice for libraries, but GitHub builds our PHP dependency graph from composer.lock and that graph feeds our security alerts. For my linked PR composer install just becomes one leg of the matrix instead of the only install mode. The update legs rewrite the lock inside the runner and nothing is committed back.

Regarding the CI runs, PRs from outside contributors need a maintainer to approve workflows, this is why you couldn't start them.

Anyways, give #67 a look and let us know if it will work for you!

…assert Two findings from CodeRabbit on #67. Set persist-credentials: false on actions/checkout@v5. The action stores the workflow token in local git config by default. Composer and PHPUnit run PR-controlled code, which can read it. No step needs git auth here: make test clones sdk-test-data, which is a separate public repo. Assert an unrecoverable 400 instead of 401. handleHttpError throws InvalidApiKeyException for 401 before it builds HttpRequestException, and that catch branch in assertStatusRecoverable ignores $recoverable. The 401 case therefore asserted nothing, and duplicated testUnauthorizedClient. Proof: assertStatusRecoverable(true, 401) passes, while assertStatusRecoverable(true, 400) fails on the isRecoverable assertion. Both predate this branch. PR #64 touched the lines, so review surfaced them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

pkruithof · 2026-08-24T12:14:03Z

Thanks for checking, looks good to me!

Regarding the composer.lock, it's fine to keep it in: Composer docs do not state that you shouldn't commit it for libraries, just that it's not needed. So in your case I'd keep it in, like you said.

Awaiting the merge/release 🙂

) * Widen dependency versions * ci(FFESUPPORT-934): test the widened dependency ranges via a matrix Keep composer.lock and prove symfony/cache ^6.4|^7.0|^8.0 works. PR #64 widens that constraint for Symfony 8 support. It also deletes composer.lock. This branch keeps both the widening and the lockfile. #64 https://datadoghq.atlassian.net/browse/FFESUPPORT-934 Deleting the lock was a prerequisite for `composer update` in CI, not for the widening. `composer install` becomes one leg of a matrix instead of the only install mode. The update legs rewrite composer.lock inside the runner, and nothing is committed back. Why the lockfile stays: - GitHub builds the PHP dependency graph from composer.lock. That graph feeds our Dependabot alerts. FFESUPPORT-534, -734 and -887 all came from it. - `composer validate --strict` checks lock sync only when a lock exists. - config.platform.php guards the committed lock. It has no other purpose. Matrix legs and what each resolves: - 8.1, 8.3 locked -> symfony/cache 6.4.40 - 8.1 lowest -> psr/log 2.0.0, psr/cache 2.0.0, google/cloud-storage 1.30.0 - 8.3 highest -> symfony/cache 7.4.16 - 8.4 highest -> symfony/cache 8.1.4 The highest legs unset config.platform.php. The 8.1.0 pin caps the solver at symfony/cache 6.4, so both legs in PR #64 resolved 6.4.x. The ^7.0 and ^8.0 branches went untested. Also: - Add .github/dependabot.yml with grouped updates. Grouping cuts the lockfile churn that makes a lock expensive to keep. - Gate `composer audit` on --no-dev. All 6 current advisories are dev-only. - ramsey/composer-install keys its cache per leg. hashFiles('**/composer.lock') collapsed to a constant once the lock was gone. - Restore 301 in the redirect test. 308 takes a different path in the decorator. composer.lock changes by the Teapot removal only: -105 lines, no version churn. Verified: 98 tests, 664 assertions pass. CI run 31860944824 passed all five legs and produced the resolutions above. codex reports no blocking issues. Out of scope: the 6 dev-only advisories, and test-package.yml running on every branch creation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: drop the dead argument to getRespondingHttpClientMock The helper takes one parameter. The call passed two. PHP discards the extra argument on a userland method, so nothing failed and the test suite stayed green. The argument was still dead. It predates this branch. Found by Copilot on #67. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(FFESUPPORT-934): harden checkout and make the unrecoverable test assert Two findings from CodeRabbit on #67. Set persist-credentials: false on actions/checkout@v5. The action stores the workflow token in local git config by default. Composer and PHPUnit run PR-controlled code, which can read it. No step needs git auth here: make test clones sdk-test-data, which is a separate public repo. Assert an unrecoverable 400 instead of 401. handleHttpError throws InvalidApiKeyException for 401 before it builds HttpRequestException, and that catch branch in assertStatusRecoverable ignores $recoverable. The 401 case therefore asserted nothing, and duplicated testUnauthorizedClient. Proof: assertStatusRecoverable(true, 401) passes, while assertStatusRecoverable(true, 400) fails on the isRecoverable assertion. Both predate this branch. PR #64 touched the lines, so review surfaced them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(FFESUPPORT-934): name the HTTP status codes and cool down dependabot Replace the HTTP status literals in APIRequestWrapper with private constants. Add a dependabot cooldown so a release ages before we take it. The constants are private, because no other class needs them. The names follow RFC 7231 and RFC 7235, which keeps parity with the constants that shrikeh/teapot supplied. The tests keep literal status codes, so a wrong constant value cannot hide behind a test that reads the same constant. Cooldown covers version updates only, so a security advisory still opens a pull request at once. The GitHub Actions ecosystem supports default-days only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(FFESUPPORT-934): stop the recoverability helper from swallowing failures Remove the InvalidApiKeyException arm in assertStatusRecoverable. The arm hid a regression of the recoverability logic. Proof: change handleHttpError to set isUnauthorized for every status at or above 400. With the arm, testRecoverableHttpError and testUnrecoverableHttpError report OK. Without the arm, both error. No caller sends 401, so the arm was unreachable and only masked the wrong exception type. Also: add a highest leg on PHP 8.1, so the minimum supported PHP resolves the current release of every dependency. Drop custom-cache-suffix, because cache_key.sh already puts the detected PHP version in the key. Add RFC 7232 to the status code comment, which defines 304. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci(FFESUPPORT-934): keep one stable check and stop the spurious package runs Add a build-and-test job that aggregates the matrix, so branch protection has one check name that survives a matrix change. Fix the trigger that started a package test on every branch. The matrix job is now test-matrix. The aggregate job keeps the name build-and-test, which is the name that ran before the matrix existed. test-package.yml declared `on: create` with a tags filter. GitHub ignores filters on create, so the workflow ran for every new branch. A push trigger honours the filter. Also set versioning-strategy to widen. This is a library, so Dependabot must keep the versions that consumers still resolve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci(FFESUPPORT-934): describe the matrix legs correctly and drop the hard-coded repo The comment did not cover the locked legs, which install what composer.lock pins. The checkout fallback now reads github.repository, so a rename or a mirror cannot break a scheduled run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(FFESUPPORT-934): check out this repo when another repo calls the workflow A called workflow sees the caller's github context, so github.repository names the caller. sdk-test-data calls run-tests.yml, so the fallback resolved to sdk-test-data and checked out the wrong tree. Name the repo instead, and read the pull request head only when this repo owns the event. This also corrects the caller-pull-request path, which took the caller's head repo before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(FFESUPPORT-934): stop relying on type coercion and name nullable types handleHttpError declares a string parameter, and the caller passed a stream. Non-strict code coerced it through Stream::__toString. Cast at the call site. The client exception was passed as the message. Exception::__construct coerced it through Throwable::__toString, which discarded the chain. Pass the message and set the previous exception. PHP 8.4 deprecates an implicitly nullable parameter. Six of them exist across the exception classes, so the new 8.4 leg emits six notices. Name the types. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(FFESUPPORT-934): silence the remaining PHP 8.4 and 8.2 deprecations Name every nullable parameter type. Declare the test properties the suite assigns. Brace the interpolated array access. * fix(FFESUPPORT-934): score a null categorical attribute as missing array_key_exists() with a null key is deprecated since PHP 8.1. A null value now takes the missing-value coefficient, which matches how a numeric attribute is scored. * test(FFESUPPORT-934): name the test status codes from the RFC package Add teapot/status-code to require-dev. The tests name each status from the RFC, and the source names its own constants, so a typo in either one fails a test. require is unchanged. --------- Co-authored-by: pkruithof <pkruithof@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

aarsilv · 2026-08-31T22:35:58Z

Closing as this been done now as part of #67. Thanks for getting the ball rolling here! 🙌

aarsilv · 2026-08-31T22:42:40Z

@pkruithof published as v4.3.0

Widen dependency versions

95adc3a

pkruithof commented Jul 23, 2026

View reviewed changes

aarsilv requested a lite review from Copilot August 10, 2026 11:35

Copilot started reviewing on behalf of aarsilv August 10, 2026 11:35 View session

Copilot AI reviewed Aug 10, 2026

View reviewed changes

aarsilv mentioned this pull request Aug 17, 2026

ci(FFESUPPORT-934): test the widened dependency ranges via a matrix #67

Merged

aarsilv closed this Aug 31, 2026

pkruithof deleted the widen-dependencies branch September 1, 2026 06:58

Navigation Menu

Uh oh!

Widen dependency versions - #64

Widen dependency versions#64
pkruithof wants to merge 1 commit into
Eppo-exp:mainfrom
pkruithof:widen-dependencies

pkruithof commented Jul 23, 2026

Uh oh!

pkruithof Jul 23, 2026

Uh oh!

pkruithof Jul 23, 2026

Uh oh!

pkruithof Jul 23, 2026

Uh oh!

pkruithof commented Jul 29, 2026

Uh oh!

aarsilv commented Aug 10, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

bertilhatt commented Aug 10, 2026

Uh oh!

aarsilv commented Aug 17, 2026

Uh oh!

pkruithof commented Aug 24, 2026

Uh oh!

aarsilv commented Aug 31, 2026

Uh oh!

aarsilv commented Aug 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

pkruithof commented Jul 23, 2026

Motivation and Context

Description

How has this been documented?

How has this been tested?

Uh oh!

pkruithof Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

pkruithof Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

pkruithof Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

pkruithof commented Jul 29, 2026

Uh oh!

aarsilv commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

bertilhatt commented Aug 10, 2026

Uh oh!

aarsilv commented Aug 17, 2026

Uh oh!

pkruithof commented Aug 24, 2026

Uh oh!

aarsilv commented Aug 31, 2026

Uh oh!

aarsilv commented Aug 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aarsilv commented Aug 10, 2026 •

edited

Loading