PRE-3361: Add get transaction tool#22
Conversation
dc57c96 to
23a011f
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new “retrieve payment/transaction” capability to the MCP core (PaymentAction::retrieveAction()), backed by a new API wrapper method, and covered by unit + integration tests.
Changes:
- Add
PaymentAction::retrieveAction()to retrieve a payment by resource ID and return aPaymentOutputDTO. - Add
Api::retrievePaymentResource()wrapper aroundPayplug\Payment::retrieve(). - Add unit/integration tests for the new retrieve flow; update CaptainHook CS Fixer config reference.
1. What's Good
- The new
Api::retrievePaymentResource()mirrors the existingcreatePaymentResource()/refundPaymentResource()response shape, which keeps DTO hydration consistent. - Unit and integration coverage is added for both success and failure paths of the retrieve flow.
- CaptainHook’s php-cs-fixer config now matches the Composer scripts’
--config=.php-cs-fixer.dist.php.
2. Summary table
| Dimension | Rating |
|---|---|
| Security | ✅ Fine |
| Correctness | |
| Performance | ✅ Fine |
| Maintainability |
3. Closing one-liner
Add declare(strict_types=1); to the new stub and either wire the stub into PHPStan/tooling or remove it to avoid carrying unused files.
4. Individual findings (one section per issue)
Heading: Correctness ⚠️ Low
Subtitle (bold): Missing declare(strict_types=1) in new PHP file (stubs/PayplugPayment.stub:1)
Code block:
<?php
namespace Payplug\Resource;Explanation paragraph: This repository consistently uses declare(strict_types=1); at the top of PHP files. Omitting it is a correctness inconsistency and can cause different coercion behavior if the file is ever executed/loaded (even if it’s “just a stub”).
Fix: Add declare(strict_types=1); after the opening tag.
<?php
declare(strict_types=1);
namespace Payplug\Resource;Heading: Maintainability ⚠️ Medium
Subtitle (bold): Stub file likely unused (not referenced by PHPStan/autoload) (stubs/PayplugPayment.stub:1)
Code block:
<?php
namespace Payplug\Resource;
class Payment
{Explanation paragraph: The new stub doesn’t appear to be referenced by Composer autoload/autoload-dev, and phpstan.neon doesn’t list it under parameters.stubFiles. As a result, it likely has no effect on static analysis/typing and becomes extra maintenance surface.
Fix: Either (a) wire it into the intended tool (e.g., PHPStan stubFiles), or (b) remove the stub if it’s not needed.
# phpstan.neon
parameters:
stubFiles:
- stubs/PayplugPayment.stubReviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Units/Actions/PaymentAction/retrieveActionTest.php | Adds unit coverage for PaymentAction::retrieveAction() behavior (load failure, retrieve failure, success). |
| tests/Integration/PaymentAction/retrieveActionTest.php | Adds integration coverage around Payplug static retrieve() behavior (exception vs success). |
| stubs/PayplugPayment.stub | Introduces a Payment resource stub (currently missing strict types + appears unreferenced). |
| src/Utilities/Services/Api.php | Adds retrievePaymentResource() wrapper returning the standard response array shape. |
| src/Actions/PaymentAction.php | Adds retrieveAction() public entry point returning PaymentOutputDTO. |
| captainhook.json | Aligns php-cs-fixer hook config with .php-cs-fixer.dist.php. |
23a011f to
b11826f
Compare
b11826f to
84b1257
Compare
84b1257 to
ad37555
Compare
ad37555 to
2c81dbd
Compare
Description
PRE-3361: Add get transaction tool
Related Issue
Ticket: PRE-3361
Type of Change
[ ] 🐛 Bug fix
[x] ✨ New feature
[ ] 💥 Breaking change
[ ] ♻️ Refactor
[ ] 🔧 Configuration / CI
[ ] 🚀 Release (
release/*branch targetingmaster)[ ] 📦 Dependency update
[ ] 🔒 Security fix
[ ] 📝 Documentation update
✅ Quality Checklist
Local Environment & Hooks
(PRE|SMP)-XXXX: descriptionpattern.phpstan.neon/.php-cs-fixer.php) were generated successfully from.disttemplates.Testing & Code Quality
composer cs:fix).vendor/bin/phpstan).CI/CD Deployment Context
release/*branch, I am targeting the correct base branch to allow the automatedapply-releaseversion bumping job to run.Screenshots (if applicable)
Notes for Reviewer