Feature/pre 3469#305
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
Spike / proof-of-concept to validate that unified-plugin-core’s key contracts can be implemented cleanly on top of Sylius (Workflow state machine, PSR-6 cache, Sylius gateway config, Doctrine persistence), backed by unit + kernel-level integration coverage.
Changes:
- Add
src/Spike/skeleton implementations for 4 UPC contracts plus a normalizedPayplugOperationDoctrine entity. - Add PHPUnit unit tests for each spike class and a kernel-booted
SpikeIntegrationTest. - Adjust test/kernel wiring (
phpunit.xml.dist) and add local Composer wiring to pull unified-plugin-core.
1. What's Good
- All new PHP files are
strict_types=1and the spike classes are clearly labeled as non-production. - Good defensive handling around PSR-6’s
mixedreturn type (SyliusTokenCache::get()). - Workflow transition application is correctly guarded with
can()and includes a flush (and the unit tests explicitly assert flush behavior).
2. Summary table
| Dimension | Rating |
|---|---|
| Security | Fine |
| Correctness | Medium (integration test + schema setup will fail by default) |
| Performance | Fine |
| Maintainability | Medium (spike wiring bleeds into non-test env; Composer path repo) |
| Operational implications | High (composer install / CI and test suite stability) |
3. Closing one-liner
Before merge, the spike needs to be made CI-safe (Composer dependency strategy + integration test/schema setup) and the Doctrine mapping prepend must not affect non-test environments.
4. Individual findings (one section per issue)
Heading: Operational implications — High
Subtitle (bold): Composer path repository makes installs non-reproducible (composer.json:40-68)
Code block:
+ "payplug/unified-plugin-core": "@dev",
...
+ "repositories": [
+ {
+ "type": "path",
+ "url": "../../unified-plugin-core",
+ "options": { "symlink": true }
+ }
+ ],Explanation paragraph: A path repository outside the repo will fail on CI and for contributors who don’t have that sibling directory, so composer install becomes non-deterministic and the plugin is effectively non-installable by default.
Fix: Use a CI-friendly dependency source (tagged release or VCS URL) or keep the spike dependency local-only (documented), rather than committing a relative path repo outside this repository.
Heading: Operational implications — Medium
Subtitle (bold): Spike Doctrine mapping is registered in all environments (src/DependencyInjection/PayPlugSyliusPayPlugExtension.php:31-37)
Code block:
public function prepend(ContainerBuilder $container): void
{
$this->prependTwigExtension($container);
$this->prependDoctrineMigrations($container);
$this->prependMonologExtension($container);
+ $this->prependSpikeDoctrineMapping($container);
}Explanation paragraph: This unconditionally registers the spike entity mapping in production/dev too, which can affect Doctrine metadata and schema expectations outside tests, despite the spike being explicitly “not shipped code.”
Fix: Only prepend the spike mapping in the test environment (or behind an explicit opt-in flag).
Heading: Correctness — Medium
Subtitle (bold): Integration test runs by default but requires manual DB + table setup (tests/PHPUnit/Spike/SpikeIntegrationTest.php:34-38)
Code block:
protected function setUp(): void
{
self::bootKernel();
$this->entityManager = self::getContainer()->get(EntityManagerInterface::class);
}Explanation paragraph: phpunit.xml.dist includes tests/PHPUnit, so this kernel test will run in the default suite; however it depends on fixtures + a manually created payplug_operation table (no migration included). That will break fresh CI runs and local runs unless the user has performed extra manual steps.
Fix: Make the integration test opt-in (env var) and/or create/update the PayplugOperation schema in setUp() (e.g., Doctrine SchemaTool) so the test suite is self-contained.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Spike/SyliusOrderStateMutator.php |
Spike adapter mapping UPC outcomes to Sylius payment workflow transitions (with flush). |
src/Spike/SyliusTokenCache.php |
Spike PSR-6 implementation of UPC token cache contract. |
src/Spike/SyliusConfigurationRepository.php |
Spike adapter reading/writing Sylius gateway config with live/test scoping and non-leaky error messages. |
src/Spike/SyliusPaymentRepository.php |
Spike Doctrine-backed persistence for operation data + idempotency flag. |
src/Spike/Entity/PayplugOperation.php |
New normalized entity representing operation data, separate from Payment::details. |
tests/PHPUnit/Spike/*.php |
Unit coverage for spike adapters and a kernel integration test. |
src/DependencyInjection/PayPlugSyliusPayPlugExtension.php |
Adds Doctrine mapping prepend for spike entity directory (currently unconditional). |
phpunit.xml.dist |
Fixes kernel boot config by using KERNEL_CLASS. |
composer.json |
Adds unified-plugin-core dev dependency plus a local path repository to resolve it. |
2ef0fdc to
db58f13
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
composer.json:40
- Depending on a moving dev branch ("dev-develop") makes installs non-reproducible and can break CI unexpectedly when that branch changes. If this must stay, consider pinning to a specific commit hash or tagged release (or using a local path repository during spike work).
"payplug/unified-plugin-core": "dev-develop",
db58f13 to
690d9f5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
composer.json:45
payplug/unified-plugin-coreis required asdev-develop, which is a moving target and can break this spike (and its test expectations) unpredictably as the branch evolves. To keep the spike reproducible for reviewers/CI reruns, pin this to a specific commit hash (or a tagged/pre-release version) rather than a live branch name.
"payplug/unified-plugin-core": "dev-develop",
"php-parallel-lint/php-parallel-lint": "1.4.0",
"phpmd/phpmd": "^2.15.0",
"phpro/grumphp": "^2.12",
"phpstan/extension-installer": "1.4.3",
"phpstan/phpstan": "2.0.4",
NotifyPaymentRequestHandler (task 5) will reference UPC contract types directly; leaving the package require-dev-only would fatal-error the container the moment a merchant's production composer install --no-dev tries to autowire the new dependency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The spec doc was staged from an earlier, unrelated session step and got swept into the unified-plugin-core dependency commit by mistake. Pulling it back out of git tracking (still present on disk, uncommitted) — it should only be committed when explicitly requested.
Moves the IOrderStateMutator skeleton out of src/Spike/ into a real production namespace, ahead of wiring it into NotifyPaymentRequestHandler.
Moves the ITokenCache skeleton out of src/Spike/ into a real production namespace. Stays validated by a real-infrastructure integration test only (see SpikeIntegrationTest) — no production call site, since its one real analog (PayPlugApiClientFactory's OAuth token cache) gates authentication for every gateway and isn't worth the regression risk of replacing here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…onRepository Moves the IConfigurationRepository skeleton out of src/Spike/ into a real production namespace. getPublicKeyId()/getPublicKeyValue() now default to an empty string instead of throwing, since no production code writes public_key_id/public_key_value yet (Hosted Fields isn't built) — documented as friction rather than blocked on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Additive real call site: runs after the existing PaymentTransitionApplier transition, guarded by the mutator's own can() check (safe no-op once the transition's already applied) and wrapped in try/catch so any failure is logged, never allowed to break webhook notification handling.
…call Cannot cast mixed to string at (string) $order->getId() — Sylius's ResourceInterface::getId() returns mixed. Matches the existing @phpstan-ignore-line convention already used elsewhere in this same file for getDetails() (mixed return), rather than adding a new baseline entry.
Documents the require-dev blocking friction found and fixed, and the production wiring decisions for each promoted contract. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Symfony does not auto-alias a singly-implemented interface for plain autowiring — confirmed by directly booting the container, which failed to compile with "Cannot autowire service NotifyPaymentRequestHandler: ... references interface IOrderStateMutator but no such service exists." Since NotifyPaymentRequestHandler is an #[AsMessageHandler] service instantiated on every webhook, this broke all PayPlug notification handling. Adds the explicit alias, matching the convention already used for other interfaces in this file.
…rders PayplugOrderStateMutator::apply() resolves the order's *last* payment internally (its contract is keyed by order ID, not payment ID). On an order with more than one payment (e.g. a failed attempt followed by a retry), that could be a different payment than the one this specific webhook is about — skip the additive call rather than risk transitioning the wrong payment.
Description
Spike/proof-of-concept validating that unified-plugin-core's 6 core contracts (Contracts/) are actually implementable against Sylius before they get frozen. Produces throwaway skeleton implementations of the 4 highest-risk contracts under src/Spike/, backed by real PHPUnit coverage at two levels — not production code, and not meant to be merged as-is into the plugin's real service wiring.
What was built (src/Spike/):
Verdict: no blocking friction. All 4 contracts are satisfiable as-is. Full findings, including a couple of adaptation notes that don't require interface changes, are in src/Spike/VALIDATION.md.
Testing, two levels:
The integration pass caught a real bug the unit tests couldn't see: SyliusOrderStateMutator never flushed the EntityManager after the state machine transition (Symfony Workflow only mutates the state property in memory), so the transition silently never persisted. Fixed by injecting EntityManagerInterface and flushing after every applied transition — same pattern the plugin's existing PaymentStateResolver already uses.
Two unrelated pre-existing issues were also found and fixed along the way since they blocked running this work at all:
Motivation: UHF (Unified Hosted Fields) targets Sylius as its first CMS inte6 UPC contracts we need proof they're implementable without friction,particularly the ones flagged as risky (Symfony Workflow constraints, PSR-6 dential storage).
Precision: The src/Spike/ files are deliberately kept outside the usual directories (src/Entity, etc.) and documented as proof-of-concept skeletons, not production code — worth re-stating this in the reviewer notes to avoid any confusion during review.
Related issue(s): Closes PRE-3469
Type of Change
Checklist
Code Quality
Testing
Security & Ops