Skip to content

Remove the Buzz fallback, accept any 2xx, tolerate '@' in http_client, add CLAUDE.md - #60

Merged
loevgaard merged 5 commits into
3.xfrom
minor-review-notes
Aug 31, 2026
Merged

Remove the Buzz fallback, accept any 2xx, tolerate '@' in http_client, add CLAUDE.md#60
loevgaard merged 5 commits into
3.xfrom
minor-review-notes

Conversation

@loevgaard

@loevgaard loevgaard commented Aug 31, 2026

Copy link
Copy Markdown
Member

Fixes #54.

Review notes addressed

  • Any 2xx is a successClient::sendRequest() accepted only 200; a 204 would have been reported as a failure and retried. Tests now cover a status matrix (200/201/204/299 accepted; 199/300/301/404/500 rejected).
  • http_client: '@service' no longer fails — the README documented the @-prefixed form, but the compiler pass looked the id up verbatim, so that example produced "service not found". The pass now strips a leading @; the README example is fixed too.
  • Buzz fallback removed (breaking) — the plugin no longer ships a fallback HTTP client. http_client now defaults to psr18.http_client, the PSR-18 adapter Symfony registers when symfony/http-client (already a Sylius dependency) is installed; a PSR-17 factory such as nyholm/psr7 is needed alongside it, as it already was for the plugin's own request factory. If the configured service does not exist, the compiler pass fails with a message saying what to install or configure. InterfaceNotFoundException (Buzz-only) is removed, kriswallsmith/buzz leaves require-dev, and the README explains the new default. Apps that relied on the automatic Buzz fallback must either have symfony/http-client installed (Sylius apps do) or set http_client to their PSR-18 client's service id.
  • Trusted proxies — README explains that the IP sent to Partner Ads is the order's customerIp (from Request::getClientIp()), so trusted proxies must be configured behind a load balancer.
  • Session guard — moot: NotifySubscriber was removed in Replace request-time Partner Ads notification with conversion entity + cron command #55.
  • Cookie stuffing was informational; nothing to do.

Also in this PR

  • UPGRADE.md documenting the whole 2.x → 3.x upgrade (requirements, install/config changes, migration, the now-required cron command, behaviour changes, removed classes and renamed service ids), linked from the README. 3.x has never been tagged, so one section covers everything up to and including this PR.
  • CLAUDE.md (the repo had none yet): what the plugin does, the invariants that must not be broken (nothing failing during checkout, no unique constraint on the conversion's order, the command lock, partner id validation), the code layout, the maintainer conventions, local development and database setup, CI gotchas, and the translation rules (all 16 locales must carry every key).
  • TranslationCataloguesTest makes that rule enforceable: it fails when any locale is missing keys, has extra keys, has empty values, or when the shipped locale set differs from the documented one. symfony/yaml is added to require-dev for it (^6.4 || ^7.4).

ECS, PHPStan (level max), 168 tests, and Infection at 100% on the changed source files.

…t IP and HTTP client

- Client: any 2xx response is a success; only 200 was accepted before
- RegisterHttpClientPass: a leading '@' on the configured service id is
  stripped instead of failing with a confusing "service not found"
- README: recommend symfony/http-client's psr18.http_client over the
  unmaintained Buzz, fix the http_client example, and explain that the
  customer IP relies on trusted proxies being configured
- CLAUDE.md: document how translations are maintained, enforced by a
  test that every locale carries exactly the English keys

The session-guard note in the issue concerned NotifySubscriber, which
no longer exists.

Fixes #54
Claude-Session: https://claude.ai/code/session_01Mt12J8vdGwwWg4V23uoEf9
@codecov

codecov Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.85%. Comparing base (dc045ab) to head (da444ac).

Additional details and impacted files
@@             Coverage Diff              @@
##                3.x      #60      +/-   ##
============================================
- Coverage     84.90%   84.85%   -0.05%     
+ Complexity      106      101       -5     
============================================
  Files            22       21       -1     
  Lines           550      535      -15     
============================================
- Hits            467      454      -13     
+ Misses           83       81       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The plugin no longer ships a fallback HTTP client. The http_client option
now defaults to psr18.http_client, the PSR-18 adapter Symfony registers
when symfony/http-client (a Sylius dependency) is installed, and the
compiler pass fails with an actionable message when the configured
service does not exist. InterfaceNotFoundException only existed for the
Buzz fallback and is removed with it.

Claude-Session: https://claude.ai/code/session_01Mt12J8vdGwwWg4V23uoEf9
@loevgaard loevgaard changed the title Accept any 2xx from Partner Ads, tolerate '@' in http_client, document IP and HTTP client Remove the Buzz fallback, accept any 2xx, tolerate '@' in http_client, add CLAUDE.md Aug 31, 2026
@loevgaard
loevgaard merged commit 749cbab into 3.x Aug 31, 2026
93 of 94 checks passed
@loevgaard
loevgaard deleted the minor-review-notes branch August 31, 2026 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Minor review notes: 2xx handling, session guard, trusted proxies doc, Buzz recommendation

1 participant