Widen dependency versions - #64
Conversation
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| deps: |
There was a problem hiding this comment.
Ideally you'd want to add different PHP versions to this as well. I'll leave that out of scope for this PR though.
There was a problem hiding this comment.
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.
| use Psr\Http\Client\ClientExceptionInterface; | ||
| use Psr\Http\Client\ClientInterface; | ||
| use Psr\Http\Message\RequestFactoryInterface; | ||
| use Teapot\StatusCode\RFC\RFC7231; |
There was a problem hiding this comment.
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.
|
@aarsilv you seem to be the most active maintainer here, could you take a look at this please? |
|
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. |
There was a problem hiding this comment.
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/cacheconstraint to include^8.0and remove the Teapot dependency used only for HTTP status constants. - Remove
composer.lockfrom 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); |
|
Feedback:
|
🎟️ 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:
composer.lockas it's not recommended for librariesHow has this been documented?
How has this been tested?
make test