Upgrade plugin from Sylius 1.x to Sylius 2.x#108
Open
loevgaard wants to merge 42 commits into
Open
Conversation
Follows the Setono v1→v2 playbook (https://github.com/orgs/Setono/discussions/1) and aligns the layout with `setono/sylius-plugin-skeleton@2.2.x`. Key changes: - composer.json: PHP `>=8.2`, Symfony `^6.4 || ^7.4`, Sylius `^2.0`, Doctrine ORM `^3.0`. Drops FOSRestBundle, JMS Serializer, behat-transliterator, Psalm, phpspec, Buzz and the carrier-bundle dev deps; adds `setono/sylius-plugin: ^2.0`, `setono/doctrine-orm-trait`, `api-platform/core ^4`, `lexik-jwt ^3.1`, `dama/doctrine-test-bundle`. All five third-party carrier bundles (Budbee, CoolRunner, DAO, GLS, PostNord) move to `suggest`. - Bundle class overrides `getPath()` and `getConfigFilesPath()` so Doctrine-mapping discovery works against the flattened layout. - File layout: `src/Resources/{config,translations,views,public}` moves to repo-root `config/`, `translations/`, `templates/`, `public/`. - DI: XML services converted to PHP DSL under `config/services/**`, service IDs renamed to FQCN (with interface aliases) where appropriate; the registry, cache alias and provider tag IDs are preserved. The extension implements `PrependExtensionInterface` and inlines the messenger bus, `sylius_twig_hooks` configuration and the former `app/config.yaml` / `app/fixtures.yaml` imports. - AJAX endpoints rewritten to plain Symfony controllers returning `JsonResponse` via `Symfony\Component\Serializer`. JMS YAML metadata replaced by `#[Groups]` / `#[SerializedName]` attributes on `PickupPoint`. - `LoadPickupPointsHandler` switches from `EntityManagerInterface` to `ManagerRegistry` + `Setono\Doctrine\ORMTrait`, takes a class-string parameter, and uses `#[AsMessageHandler]`. - Trait `@ORM\Column` PHPDoc converted to PHP 8 attributes for ORM 3 compatibility. `PickupPoint` latitude/longitude mapping switched from `decimal` to `float` to match the PHP property types under ORM 3 type checks. - `Behat\Transliterator\Transliterator` swapped for `Symfony\Component\String\Slugger\AsciiSlugger`. - Plugin's `_javascripts.html.twig` and pickup-point shipment label auto-wired through `sylius_twig_hooks` (`sylius_admin.base#javascripts`, `sylius_shop.base#javascripts`, `sylius_admin.order.show.content.sections.shipments.item`). - Tooling rebuilt around `setono/sylius-plugin`: new `phpstan.neon` (level 6), `ecs.php`, `rector.php` (UP_TO_PHP_82, applied), `composer-dependency-analyser.php`, `phpunit.xml.dist` with unit/functional suites and the `DAMADoctrineTestBundle` extension, and a `.github/workflows/build.yaml` using the `setono/sylius-plugin/*@v2` composite actions across PHP 8.2/8.3/8.4 × Symfony 6.4/7.4 × lowest/highest. - Tests: phpspec dropped (6 specs ported to PHPUnit + Prophecy under `tests/Unit/`); Behat dropped; `tests/Application/` rebuilt against the 2.2.x skeleton with the plugin's resource overrides preserved, `Sylius\TwigHooks\SyliusTwigHooksBundle` and `DAMADoctrineTestBundle` registered, and `HEADER_X_FORWARDED_ALL` replaced with the Symfony 7 combination. Verification: composer validate, ECS, PHPStan, PHPUnit (15 tests), composer-dependency-analyser, Rector (applied 16 modernizations), `debug:container` (all FQCN services resolve), `debug:router`, `doctrine:schema:validate`, `lint:container`, `lint:yaml`, `lint:twig`, and an end-to-end smoke test booting the test app to confirm the plugin's Twig hooks fire on the shop layout and the AJAX endpoint returns JSON with CSRF enforcement intact. See UPGRADE-2.0.md for a full migration guide.
- `tests/Application/webpack.config.js`: replace the v1-era Encore script with the skeleton's `SyliusAdmin.getWebpackConfig()` / `SyliusShop.getWebpackConfig()` helpers so `yarn build` resolves the Sylius asset entry-points correctly. - `tests/Application/config/bundles.php`: filter the bundle list through `class_exists()`. The `static-code-analysis` matrix removes `sylius/sylius` before booting the kernel, which drops transitively-installed bundles (e.g. `Symfony\Bundle\MonologBundle\MonologBundle`); without the filter PHPStan crashes when its Symfony plugin boots the kernel. - `phpstan.neon`: drop the `symfony.consoleApplicationLoader`. We do not need container-resolved analysis, and skipping the kernel boot avoids the same missing-bundle class-load failures in the `static-code-analysis` matrix. - `tests/Unit/Provider/LocalProviderTest.php`: build the pickup-point `FactoryInterface` via Prophecy instead of instantiating `Sylius\Component\Resource\Factory\Factory` directly. The Sylius resource-bundle legacy namespace is exposed through `class_alias()`, which PHPStan cannot follow once the kernel is no longer booted.
… run The CI's functional-tests action runs `vendor/bin/phpunit --testsuite=functional` which fails with "Test directory ... tests/Functional not found" when the directory does not exist. Add an empty placeholder mirroring the plugin-skeleton layout.
The BC check tries to install 1.x's composer.json to compare API surface, but 1.x pins `api-platform/core ^2.7.16` which composer now blocks because of security advisories (PKSA-gs8r-6kz6-pp56, PKSA-gnn4-pxdg-q76m, PKSA-dsd6-6541-26zs). For a major version bump every BC break is intentional, so make the job informational rather than blocking until 2.0 ships.
Every API change in this PR is intentional — running Roave's BC check against 1.x serves no purpose for a major version bump. (As a bonus, the tool currently cannot even install 1.x because composer blocks the 1.x `api-platform/core ^2.7` constraint on security advisories.)
Drop the opt-in PSR-cache decorator and the local-snapshot fallback decorator along with the infrastructure that backed the snapshot: the `setono-sylius-pickup-point:load-pickup-points` console command, the messenger command/handler, the plugin-owned `PickupPoint` Doctrine resource and indices listener, and the `TimeoutException`. Each provider now talks to its carrier API directly. Tighten PHPStan to `level: max` and refresh `.gitattributes`, `UPGRADE.md`, and `README.md` to match the reduced surface.
Now that the LoadPickupPointsHandler infrastructure is gone, nothing in the plugin calls `ProviderInterface::findAllPickupPoints()`. Drop the method from the interface and every shipped provider implementation. Also drop `PickupPointInterface extends ResourceInterface` and the now-meaningless `PickupPoint::$id` / `getId()` — the model is a DTO populated from a carrier API response, not a Doctrine entity, so the `sylius/resource-bundle` shadow dep flagged by `composer-dependency-analyser` disappears. Remove `psr/http-client` from `require` — no production code in src/ touches PSR-18 directly (each carrier provider talks to its own bundle's client). PHPStan: silence the cross-Symfony-version `cast.useless` report on the `(array) $view->vars` cast in `PickupPointChoiceType::buildView()`. The cast is necessary because `FormView::$vars` is typed `array` on Symfony 7+ but left untyped (and so resolved as `mixed`) by phpstan-symfony's FormView stub and by Symfony 6.4. Drop the brittle `@param array<string, mixed>` PHPDoc on `ShippingMethodExampleFactory::create()` — it triggered a contravariance violation against the looser parent signature on the lowest-deps matrix.
`Model\PickupPoint` and `Model\PickupPointInterface` are gone; the replacement is `DTO\PickupPoint` — a final class with public properties, populated by each provider from the carrier API response. The two derived accessors (`getCodeValue()`, `getFullAddress()`) stay as methods so the serialization groups (`Detailed` / `Autocomplete`) remain stable for the AJAX endpoints. Providers now assign properties directly instead of going through setters; the interface and consumers (controller, data transformer) were updated to the new type.
Drop the `Model\PickupPointCode` value object. Its three pieces (provider, id, country) now live as public properties on `DTO\PickupPoint`, alongside the existing data fields. The wire-format string `provider---id---country` that drives the AJAX endpoint and the hidden form field is now produced by `PickupPoint::getCodeValue()` directly from those properties. `ProviderInterface::findPickupPoint()` now takes the id and country as separate string arguments instead of a `PickupPointCode` instance, and `PickupPointToIdentifierTransformer` parses the wire-format string inline (the only place that needs to).
Both were pulled in by the now-removed `PickupPointCode` value object (`Countries::exists()` and `mb_strtoupper()` respectively). Nothing in `src/` references them anymore, and the dependency-analysis CI matrix flagged them as unused.
…the DTO
Per request, `DTO\PickupPoint` is now a pure data bag with public scalar
properties and nothing else — no `getFullAddress()`, no `getCodeValue()`,
no `#[Groups]` / `#[SerializedName]` attributes. The form data transformer
now builds the `provider---id---country` wire-format string inline from
the DTO properties.
Heads-up: the AJAX endpoints (`PickupPointByIdAction`,
`PickupPointsSearchByCartAddressAction`) still call `$serializer->serialize(
$pickupPoint, 'json', ['groups' => ['Detailed']])`, and the shop JS reads
`value.code` and `value.full_address` off the response. Removing the
attributes means the JSON-with-groups output is now `{}`, so those
endpoints need a follow-up — either drop the `groups` filter and let
public-property serialization emit every field, or rebuild the response
payload in the controller.
The DTO no longer carries `#[Groups]` / `#[SerializedName]` attributes, so
serializing with `['groups' => ['Detailed']]` (or `['Autocomplete']`)
produces `{}`. Drop the groups arg from both AJAX actions
(`PickupPointByIdAction`, `PickupPointsSearchByCartAddressAction`) so the
serializer emits every public property on the DTO — `provider`, `id`,
`name`, `address`, `zipCode`, `city`, `country`, `latitude`, `longitude`.
The shop JS previously read `value.code` and `value.full_address` off the
response. Compose both client-side from the new individual fields so the
existing Twig template (`{code}` / `{full_address}` placeholders) keeps
working untouched.
- DTO becomes a pure data bag: `provider`, `id`, `name`, `address`, `zipCode`, `city`, `country`, `latitude`, `longitude`. Both `latitude` and `longitude` are now `?string` (carrier APIs return them as floats or strings; the DTO is the boundary, so coerce on the way in). - DTO implements `\JsonSerializable` (round-tripping via Doctrine JSON columns) and exposes a `fromArray(array): self` factory that defensively coerces each field through a private `stringOrNull()` helper. Providers now assign properties directly instead of going through setters. - `ProviderInterface::findPickupPoints()` returns `list<PickupPoint>` instead of `iterable<PickupPoint>`. The five non-generator providers already produced arrays; `DAOProvider`'s internal Generator was materialized into a list and its `findPickupPoint()` lookup reads `[0] ?? null` rather than iterating.
`PickupPointAwareInterface` (and `PickupPointAwareTrait`) gain a new `pickup_point` JSON column / `?PickupPoint $pickupPoint` accessor. The trait's property is typed `null|PickupPoint|array` so the setter can store a DTO directly (Doctrine serializes via `JsonSerializable`) while the getter handles both the in-memory DTO and the array form returned by Doctrine on hydration — re-inflating arrays via `PickupPoint::fromArray()`. The legacy `pickup_point_id` string column and its `hasPickupPointId()` / `setPickupPointId()` / `getPickupPointId()` methods stay in place behind `@deprecated since 2.0` notes so existing consumers keep working; nothing in the plugin reads the new column yet. UPGRADE.md gains a section documenting the deprecation table and an example Doctrine migration that backfills the JSON column by splitting the legacy `provider---id---country` string with `SUBSTRING_INDEX` (MySQL/MariaDB; PostgreSQL `split_part` shown as a commented variant).
Sylius 2.x admin/shop templates only render form fields whose Twig hook
is registered. The plugin's form extensions add `pickupPointProvider`
to the admin shipping-method form and `pickupPointId` to the shop
checkout shipment form, but until now those fields were in the form
yet never rendered (causing the admin field to be invisible and the
shop checkout to hit a 422 on submit because the JS-driven pickup-point
widget never reached the DOM).
`Extension::prepend()` now:
- Registers `@SetonoSyliusPickupPointPlugin/Form/theme.html.twig` via
`twig.form_themes` so the `setono_sylius_pickup_point_choice_row`
block applies wherever `PickupPointChoiceType` is rendered.
- Adds hooks `sylius_admin.shipping_method.{create,update}.content.form.configuration#pickup_point_provider`
pointing at a new
`Admin/ShippingMethod/Form/Configuration/pickupPointProvider.html.twig`.
- Adds hook `sylius_shop.checkout.select_shipping.content.form.shipments.shipment#pickup_point`
pointing at a new `Shop/Checkout/SelectShipping/Shipment/pickupPoint.html.twig`,
which renders `form.pickupPointId` only when the field is present on
the shipment form.
- `PickupPointsSearchByCartAddressAction` no longer validates an
`_csrf_token` query param. It now reads the new `provider` query
param via `Request::query->get()` and throws a `BadRequestHttpException`
for empty input. The Generator special-case (`iterator_to_array()`)
is gone — the provider contract guarantees a list now.
- `ShippingMethodChoiceTypeExtension` stops depending on
`CartContextInterface` and `CsrfTokenManagerInterface`; the
`choice_attr` callback only emits `data-pickup-point-provider`. The
controller service def loses `security.csrf.token_manager`, the form
extension service def loses the cart-context + csrf args, and the
unit test was rewritten around the slimmer constructor.
- Shop JS drops the `{_csrf_token}` substitution and renames the
`{providerCode}` placeholder to `{provider}`. The `_javascripts.html.twig`
template updates the search-URL query string to match.
CLAUDE.md gains a "Working agreements" section codifying two rules the user already directed in chat: - Don't run `git commit` (or `git push`) without explicit instruction. - After any change that affects the rendered admin/shop UI, drive the test app in a browser via the Playwright MCP tools and verify the flow before reporting the task done.
`PickupPoint` already implements `JsonSerializable`, so `JsonResponse`'s internal `json_encode()` call produces the same flat output the Symfony serializer used to emit — no need to inject the serializer just to serialize the array of DTOs ourselves. The action now returns `new JsonResponse($provider->findPickupPoints($order))` directly. The controller service definitions also gained `->public()` and dropped the `controller.service_arguments` tag, and the search action now reads the cart from `sylius.context.cart` (not the composite) — matching the broader Sylius 2.x conventions.
Modeled after `Symfony\Component\Console\Attribute\AsCommand`, the new
class-level attribute `Setono\SyliusPickupPointPlugin\Attribute\AsProvider`
declares the two pieces of metadata every pickup-point provider needs:
- `code` — machine identifier (registry key + wire-format `provider`
segment), e.g. `"faker"`, `"gls"`.
- `name` — label shown in the admin shipping-method form, typically a
translation key, e.g. `"setono_sylius_pickup_point.provider.faker"`.
The abstract `Provider` reads the attribute via reflection (with a per-
class static cache) to implement `getCode()` and `getName()`, so concrete
providers can drop those methods entirely. `FakerProvider`, `DAOProvider`,
`GlsProvider` and `PostNordProvider` are annotated with `#[AsProvider]`
and lost their hand-written getters.
DI side:
- Each shipped provider's service file now uses bare
`->tag('setono_sylius_pickup_point.provider')` (no inline `code` /
`name` attributes).
- `RegisterProvidersPass` resolves the `(code, name)` pair from the tag
attributes if present, otherwise reflects on the service's class for
`AsProvider` — and throws when neither source supplies them.
- The extension calls `registerAttributeForAutoconfiguration(AsProvider::class, …)`
so any consumer-side provider with `autoconfigure: true` is auto-tagged
the same way.
Drop the `Budbee` and `CoolRunner` carrier providers, their service definitions, their configuration-tree nodes, the `suggest` entries for `setono/budbee-bundle` and `setono/coolrunner-bundle`, and the phpstan / composer-dependency-analyser path exclusions that targeted those files. `UPGRADE.md` gains a "Removed providers" section pointing consumers at the remaining DAO / GLS / PostNord / Faker providers (or their own `ProviderInterface` implementation) and listing the YAML config keys that need to go. The DI extension test (`SetonoSyliusPickupPointExtensionTest`) drops both keys from its minimal configuration so the extension keeps loading against the updated config tree.
The `name` field on `#[AsProvider]` is now the human-readable carrier
brand as it appears in the admin shipping-method dropdown — `"Faker"`,
`"DAO"`, `"GLS"`, `"PostNord"` — rather than a translation key.
Updated the four shipped providers' attributes accordingly and dropped
the matching docblock guidance on `AsProvider::$name`.
The previously translated `setono_sylius_pickup_point.provider.*` keys
are removed from `translations/messages.{en,da}.yml`; they were the
former `name` values and are no longer referenced.
Rename every directory under `templates/` to snake_case to match the Sylius / Symfony convention (`templates/admin/...`, `templates/shop/...`, `templates/form/...`). File basenames stay camelCase so existing Twig namespaces and includes don't shift more than they need to. Updated references: - `SetonoSyliusPickupPointExtension::prepend()` — every `@SetonoSyliusPickupPointPlugin/...` template path in the `twig.form_themes` entry and the four `sylius_twig_hooks.hooks` entries now points at the lowercase directories. - `UPGRADE.md` — the README-imported `Shop/Label/Shipment/pickupPoint.html.twig` example reference was lowercased to match.
Restructure routing to match `setono/sylius-plugin-skeleton@2.2.x`: thin
top-level entrypoints delegate to per-section files under `config/routes/`.
- `config/routes.yaml` (new) — localized entrypoint, imports
`config/routes/shop.yaml` with `prefix: /{_locale}` and the skeleton's
`_locale` requirement regex.
- `config/routes_no_locale.yaml` (new) — non-localized entrypoint for
stores with localized URLs disabled (imports the shop file with no
prefix), carrying the skeleton's explanatory header.
- `config/routes/shop.yaml` — now holds the actual route definitions; the
`/ajax/pickup-points` segment that used to live on the import prefix is
baked into each `path` so the final URLs are unchanged.
- Removed the old `config/routes/shop_non_localized.yaml` entrypoint and
the nested `config/routes/shop/ajax/pickup-point.yaml` leaf.
No admin route file is created — the plugin registers no admin routes (the
admin shipping-method field renders via a Twig hook, not a route).
Route names and resulting URLs are unchanged
(`setono_sylius_pickup_point_shop_ajax_pickup_points_search_by_cart_address`
→ `/{_locale}/ajax/pickup-points/search`,
`setono_sylius_pickup_point_shop_ajax_pickup_point_by_id`
→ `/{_locale}/ajax/pickup-points/{pickupPointId}`), so the templates that
reference those names keep working.
The test app now imports `@SetonoSyliusPickupPointPlugin/config/routes.yaml`,
and the README/UPGRADE routing sections point at the new entrypoints.
The `getExtendedTypes()` assertion used a hardcoded class-name string, which Rector's StringClassNameToClassConstantRector flagged — failing the `coding-standards` CI job (it runs `rector process --dry-run` without continue-on-error). Import the class and assert against `ShippingMethodChoiceType::class` instead.
Add a Commands-section callout: the repo's vendor/ is locked to PHP >= 8.4 while the shell default is often 8.1 (failing Composer's platform check). Run the versioned Homebrew binary directly — e.g. "$(brew --prefix php@8.4)/bin/php" vendor/bin/... — rather than the 8.1/8.2/8.3/8.4 switcher aliases, which brew-link the system-wide default and can break other in-progress work on the machine.
loevgaard
commented
May 29, 2026
loevgaard
commented
May 29, 2026
loevgaard
commented
May 29, 2026
loevgaard
commented
May 29, 2026
loevgaard
commented
May 29, 2026
loevgaard
commented
May 29, 2026
loevgaard
commented
May 29, 2026
| return static function (ContainerConfigurator $containerConfigurator): void { | ||
| $services = $containerConfigurator->services(); | ||
|
|
||
| $services->set('setono_sylius_pickup_point.provider.dao', DAOProvider::class) |
Member
Author
There was a problem hiding this comment.
Use FQCN as service id?
Member
Author
There was a problem hiding this comment.
I'd keep the code-based ids (setono_sylius_pickup_point.provider.<code>) rather than switch to FQCN, precisely because of the multi-carrier design. A FQCN service id is unique per class, so ->set(SomeProvider::class, …) can only appear once — that blocks registering the same provider class as multiple providers with different codes, which is exactly what the registry + RegisterProvidersPass are built for (tag each service with its own code/name). The service id is purely an internal label here: the pass derives [code, name] from the tag attributes or #[AsProvider] and keys the registry by code — nothing resolves the code from the service id. Switching only the four single-instance providers to FQCN would mean mixing two id schemes and would force a rename the day a class gets a second instance. I'd rather keep one consistent, future-proof scheme — but happy to revisit if you feel strongly.
Member
Author
There was a problem hiding this comment.
Just use FQCN because we know now that the providers we have are not multi-carrier, so if we add the CoolRunner (for example) again, we would use code-based ids
loevgaard
commented
May 29, 2026
| return static function (ContainerConfigurator $containerConfigurator): void { | ||
| $services = $containerConfigurator->services(); | ||
|
|
||
| $services->set('setono_sylius_pickup_point.provider.faker', FakerProvider::class) |
Member
Author
There was a problem hiding this comment.
Use FQCN as service id?
Member
Author
There was a problem hiding this comment.
Keeping the code-based id here for the multi-instance reason — see my reply on the DAO provider.
loevgaard
commented
May 29, 2026
| return static function (ContainerConfigurator $containerConfigurator): void { | ||
| $services = $containerConfigurator->services(); | ||
|
|
||
| $services->set('setono_sylius_pickup_point.provider.gls', GlsProvider::class) |
Member
Author
There was a problem hiding this comment.
Use FQCN as service id?
Member
Author
There was a problem hiding this comment.
Keeping the code-based id here for the multi-instance reason — see my reply on the DAO provider.
loevgaard
commented
May 29, 2026
| return static function (ContainerConfigurator $containerConfigurator): void { | ||
| $services = $containerConfigurator->services(); | ||
|
|
||
| $services->set('setono_sylius_pickup_point.provider.post_nord', PostNordProvider::class) |
Member
Author
There was a problem hiding this comment.
Use FQCN as service id?
Member
Author
There was a problem hiding this comment.
Keeping the code-based id here for the multi-instance reason — see my reply on the DAO provider.
The provider registry was the generic Sylius `ServiceRegistry`, whose `get()` returns `object` — forcing a `/** @var ProviderInterface */` cast at every call site — and `Provider::getCode()`/`getName()` resolved the `#[AsProvider]` attribute via reflection on every call. Both are now gone. Registry: - New `Registry\ProviderRegistryInterface` (extends `IteratorAggregate`, generically typed `<string, ProviderInterface>`) + final `ProviderRegistry` with a typed `get(string): ProviderInterface`, `has`, `all`, `names` and an `add($provider, $code, $name)` builder. `get()` throws the new `UnknownProviderException` (listing available codes) on a miss. - The 3 consumers type-hint the interface and drop their casts; the choice extension drops the redundant `get()`/`getCode()` and uses the code it already has. Compile-time metadata (no runtime reflection): - `ProviderInterface` drops `getCode()`/`getName()`/`__toString()` and gains `setCode(string)`. The abstract `Provider` stores the code (nullable, read through a protected `getCode()` that throws if never set) and stamps it onto returned pickup points. - `RegisterProvidersPass` resolves each provider's `[code, name]` once (tag attributes, else `#[AsProvider]`), wires `setCode($code)` per provider definition — so one class can back several providers via distinct service tags — and `add()`s each into the registry, throwing `NonUniqueProviderCodeException` on a duplicate code. The `setCode` call lives on the provider definition (not in `add()`) so a provider is configured independently of which registry implementation holds it. Cleanup: - Drop the now-unused `sylius/registry` from `require`. - Remove the dead registry argument from the `HasPickupPointSelectedValidator` service definition (the validator has no constructor).
It still carried the skeleton placeholder `Acme\SyliusExamplePlugin\Tests\Application\Kernel`; point it at the real `Setono\SyliusPickupPointPlugin\Tests\Application\Kernel`.
loevgaard
commented
May 29, 2026
loevgaard
commented
May 29, 2026
loevgaard
commented
May 29, 2026
PickupPoint::fromArray() is the boundary for arbitrary Doctrine JSON-column data, where JSON numbers decode as int/float. The is_string()-only helper silently turned a numeric id or coordinate into null. Add scalarStringOrNull() that also accepts int/float and stringifies them (bool intentionally excluded), and use it for id/latitude/longitude. Text fields stay on the strict helper.
The theme defines a single block (setono_sylius_pickup_point_choice_row)
consumed by exactly one form_row(form.pickupPointId), so registering it via
twig.form_themes polluted every form in the host app. Drop the global prepend
and apply it locally with {% form_theme form.pickupPointId ... %} in the shop
checkout shipment template; inherited Sylius themes stay active.
- search: /ajax/pickup-points/search -> /ajax/pickup-points/from-cart
- by-id: /ajax/pickup-points/{pickupPointId} -> /ajax/pickup-points/from-id
The by-id pickupPointId is the composite wire code "provider---id---country"
and the carrier id is opaque third-party data, so a path segment was fragile;
move it to the query string. Route names stay the same and the controller
already reads $request->get('pickupPointId') (resolves from the query), so
path('...by_id', {pickupPointId: ...}) auto-appends ?pickupPointId=... with no
controller or twig change.
Explain the constructor param is a code => human-readable-name map from the setono_sylius_pickup_point.providers container parameter (built by RegisterProvidersPass), flipped to name => code for the choice field.
On Symfony 6.4 (lowest supported) FormView::$vars is untyped, so PHPStan sees `mixed` and rejects direct offset writes; the (array) cast normalizes it so the writes type-check on both 6.4 and 7.x. Add an inline comment documenting this (the redundant-cast report on 7.x stays silenced by the scoped phpstan.neon ignore).
loevgaard
commented
May 29, 2026
loevgaard
commented
May 29, 2026
ProviderInterface::findPickupPoints() now takes a new immutable Setono\SyliusPickupPointPlugin\DTO\Address value object instead of an OrderInterface, so providers can be used outside the checkout flow. Address has nullable street/postcode/city/countryCode and a fromOrder(OrderInterface) named constructor (returns an all-null instance when there is no shipping address). This also centralizes the getShippingAddress() extraction that was duplicated across all four providers. findPickupPoint(string $id, string $country) becomes findPickupPoint(string $id, array $metadata = []) (@param array<string,mixed>), an open SPI hook since the data needed to resolve a pickup point by id is carrier-specific. The transformer passes ['country' => $country]; PostNord reads $metadata['country'] (guarded by is_string); DAO/GLS/Faker ignore it. The controller now calls findPickupPoints(Address::fromOrder($order)).
Replace the two helpers (stringOrNull + scalarStringOrNull) with a single scalarOrNull used for every field. The int/float -> string coercion now also applies to text fields, which is a net positive: a numeric JSON zipCode (e.g. 9000) rehydrates as "9000" instead of being dropped to null. bool/array/null still map to null.
Symfony's AddConstraintValidatorsPass always registers a constraint validator in the factory locator under its class name, and the Foo/FooValidator naming convention means the default Constraint::validatedBy() (static::class.'Validator') resolves HasPickupPointSelectedValidator with no config. So drop both the `alias` tag attribute and the validatedBy() override. Update the constraint unit test to assert the FQCN-by-convention resolution.
`postalCode` is the term already used by the carrier code (PostNord's query param and the PickupPoint/visitingAddress field), so the value object now matches that instead of introducing a third spelling. Address::fromOrder() still maps Sylius's getPostcode() into it via the positional constructor arg.
All three form type extensions implement getExtendedTypes(), which Symfony's FormPass uses when the tag has no `extended_type` attribute. The attribute was a redundant second source of truth that actually overrides getExtendedTypes(), so it could silently drift; dropping it makes getExtendedTypes() authoritative. Also removes the now-unused Sylius form-type imports. Verified via debug:form that each extension still attaches to the same type.
Replace the inline 'provider---id---country' wire format with an opaque, URL/form-safe base64url-JSON token owned by a single PickupPointIdentifierEncoder service. PickupPointIdentifier is a JsonSerializable value object with a matching fromArray(), so the VO owns the wire shape and the encoder owns only transport. Keep country as a first-class PickupPoint property (set by providers) while adding an open metadata map for consumer extensibility; fromPickupPoint() folds country into the identifier's metadata, which is what ProviderInterface::findPickupPoint() consumes to re-resolve a point. Serialize the search results through the Serializer and add a PickupPointNormalizer (NormalizerAware, delegates the base shape and augments it with the encoded 'identifier'); the shop JS and form theme read that 'identifier' directly instead of re-deriving the format client-side.
Rename PickupPointByIdAction to PickupPointByIdentifierAction (route .../from-id -> .../from-identifier, query param -> 'identifier') since it resolves a point from the opaque identifier token, not a raw id. The action now decodes the token via PickupPointIdentifierEncoder and resolves through the ProviderRegistry directly, instead of borrowing the form DataTransformer. That leaves PickupPointToIdentifierTransformer unused (it was attached to no form), so it is removed.
loevgaard
commented
May 29, 2026
| @@ -0,0 +1,13 @@ | |||
| setono_sylius_pickup_point_shop_ajax_pickup_points_search_by_cart_address: | |||
Member
Author
There was a problem hiding this comment.
Should the route names in this file also be renamed to match the path?
loevgaard
commented
May 29, 2026
| return static function (ContainerConfigurator $containerConfigurator): void { | ||
| $services = $containerConfigurator->services(); | ||
|
|
||
| $services->set(PickupPointsSearchByCartAddressAction::class) |
Member
Author
There was a problem hiding this comment.
Should the controller class names be renamed to match the path?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Brings the plugin onto Sylius 2.x following the Setono v1→v2 playbook (https://github.com/orgs/Setono/discussions/1) and aligns the layout with
setono/sylius-plugin-skeleton@2.2.x.>=8.2, Symfony^6.4 || ^7.4, Sylius^2.0, Doctrine ORM^3.0. Drops FOSRestBundle, JMS Serializer, behat-transliterator, Psalm, phpspec, Buzz, and the carrier-bundle dev deps; addssetono/sylius-plugin: ^2.0,setono/doctrine-orm-trait,api-platform/core ^4,lexik-jwt ^3.1,dama/doctrine-test-bundle. All five carrier bundles (Budbee, CoolRunner, DAO, GLS, PostNord) move tosuggest.src/Resources/{config,translations,views,public}flattened to repo-rootconfig/,translations/,templates/,public/. The bundle class overridesgetPath()andgetConfigFilesPath().config/services/**; service IDs renamed to FQCN where appropriate. The extension implementsPrependExtensionInterfaceand inlines the messenger bus,sylius_twig_hooksconfiguration, and the formerapp/config.yaml/app/fixtures.yamlimports.JsonResponseviaSymfony\Component\Serializer. JMS YAML replaced by#[Groups]/#[SerializedName]attributes onPickupPoint.LoadPickupPointsHandlernow usesManagerRegistry+Setono\Doctrine\ORMTrait+ a class-string parameter and is marked#[AsMessageHandler]. Trait@ORM\ColumnPHPDoc converted to PHP 8 attributes;PickupPointlat/long mapping switched fromdecimaltofloatto match the PHP property types._javascripts.html.twigand the pickup-point shipment label are auto-wired throughsylius_twig_hooks(sylius_admin.base#javascripts,sylius_shop.base#javascripts,sylius_admin.order.show.content.sections.shipments.item).setono/sylius-plugin— newphpstan.neon(level 6),ecs.php,rector.php(UP_TO_PHP_82, applied),composer-dependency-analyser.php,phpunit.xml.distwith unit + functional suites and theDAMADoctrineTestBundleextension, and a.github/workflows/build.yamlusing thesetono/sylius-plugin/*@v2composite actions across PHP 8.2/8.3/8.4 × Symfony 6.4/7.4 × lowest/highest.tests/Unit/); Behat dropped;tests/Application/rebuilt against the 2.2.x skeleton with the plugin's resource overrides preserved,Sylius\TwigHooks\SyliusTwigHooksBundleandDAMADoctrineTestBundleregistered, andHEADER_X_FORWARDED_ALLreplaced with the Symfony 7 combination.Migration guide: see
UPGRADE-2.0.md.Test plan
composer validate --strict+composer normalize --dry-runcomposer check-style(ECS, Sylius Labs ruleset)composer analyse(PHPStan level 6)composer phpunit— 15 unit tests passvendor/bin/composer-dependency-analyservendor/bin/rector process(applied 16 PHP 8.2 modernizations)bin/console cache:clear/lint:container/lint:yaml/lint:twig/doctrine:schema:validate/debug:router | grep pickup_javascripts.html.twighook fires onsylius_shop.base#javascripts; the/ajax/pickup-points/searchendpoint returns JSON and rejects an invalid CSRF token with 403; fixtures load and thefakerpickup-point shipping method materializes.fakershipping method → pickup-point dropdown renders → place order → admin order-show pickup-point label) — pending a fully built asset pipeline in the test app.