Code review findings: leftover resolver.xml, CachedVatContext reset, and other issues #5

A code review of the plugin (master, 209a7c2) surfaced the findings below. Overall the plugin is in good shape — clean decorator/composite architecture, solid test coverage, psalm level 1, mutation testing — but there is one genuine leftover bug, one runtime hazard, and a handful of lower-severity issues.

Bugs

• resolver.xml registers services for classes that no longer exist. src/Resources/config/services/resolver.xml defines Setono\SyliusToggleVatPlugin\Resolver\PriceResolver and PriceResolverInterface, but there is no src/Resolver/ directory — the classes were deleted in 198aa20 ("Decorate calculator instead of PriceHelper") while the XML file and its import in services.xml were left behind. It only works today because both services are private and unused, so the compiler drops them before lint:container runs. Anyone injecting PriceResolverInterface (it looks like a public API) gets a confusing "class not found" at runtime. Fix: delete the file and its import.

• CachedVatContext caches request state for the service's lifetime with no reset. src/Context/CachedVatContext.php memoizes the first answer forever. Under PHP-FPM that effectively means "per request", but under worker-mode runtimes (FrankenPHP, RoadRunner, Swoole) the first visitor's VAT choice sticks to the worker and is served to every subsequent visitor — and the toggle appears broken. Fix: implement Symfony\Contracts\Service\ResetInterface and tag the service with kernel.reset.

• Loose cookie parsing. CookieBasedVatContext does (bool) $cookie, so any non-empty value other than "0" — including "false" — means "with VAT". Fix: accept only '1'/'0' and throw NoVatContextException for anything else, which cleanly falls through to the configured default.

• Defensive rounding in the calculator. ProductVariantPricesCalculator casts the tax calculator's float to int, which truncates. Sylius's DefaultCalculator happens to round() before returning, so stock installs are fine — but the CalculatorInterface contract just says float, and a custom/decorated tax calculator returning e.g. 219.8 would silently shave a cent. Fix: (int) round(...).

Security (low severity)

• State change via GET. The toggle route is a plain <a href> in the topbar on every page, so crawlers and link prefetchers will follow it, and it is trivially cross-site triggerable (<img src=".../toggle-vat">). Impact is only a flipped display preference. Fix: at minimum add rel="nofollow" to the link; a small POST form would be the proper fix.

• Referer-based redirect without validation. ToggleVatAction redirects to the Referer header verbatim. Practically unexploitable, but security scanners will flag it as an open redirect. Fix: only redirect when the referer host matches the request host.

• Document the caching caveat. Prices now vary by cookie, so a full-page cache/CDN in front of the shop will serve the wrong VAT variant between users unless it varies on this cookie. Worth a README warning.

Architecture

• sylius/core: ^1.0 promises far more than is true. The plugin depends on the sylius.calculator.product_variant_price and sylius.twig.extension.price service ids, the concrete Sylius\Bundle\CoreBundle\Twig\PriceExtension shape, and ProductVariantPricesCalculatorInterface::calculateOriginal — none of which existed across the whole 1.x line, and CI only tests ~1.12.19. Fix: tighten to ^1.12 so incompatible installs fail at composer require instead of at runtime.

• Custom VAT contexts silently defeat the toggle. Autoconfigured contexts get tag priority 0, which outranks CookieBasedVatContext (-90) — so a custom context that always returns makes the toggle button a no-op (it sets a cookie that is never consulted). If intended, document the -90/-100 priorities in the README's "VAT context" section so implementors can slot around them.

• Twig\PriceExtension decoration is quietly fragile. The wrapping new TwigFilter($name, $closure) drops any options the original filter carries (currently none, so no bug today), and __call never fires for the extension interface methods (getFunctions() etc.) because AbstractExtension defines them — if Sylius's base extension ever adds functions or filter options, the decorator silently swallows them. A comment or a hard-coded wrap of the two known filter names would make the intent and risk explicit.

Nitpicks / leftovers

• SetonoSyliusToggleVatExtension — the @var docblock still lists decorate_price_helper: bool, an option that no longer exists.
• Configuration — the builder chain is missing its trailing ->end() calls after cookie_name. It works (nodes are appended on creation), but breaks the moment someone appends another node to the chain.
• Twig\Extension — sstv_display_with_vat is registered with needs_environment => true, but Runtime::displayWithVat() takes no Environment; it only works because PHP ignores extra args. is_safe => ['html'] on a bool-returning function is also meaningless. Both options should be dropped (they are correct on sstv_vat_toggler).
• ProductVariantPricesCalculator — stray unused @psalm-type BacktraceClosure.
• behat/behat is in require-dev but there are no Behat features/suites.