Repository navigation
6.x - #4124
Draft
lukeholder wants to merge 424 commits into
Draft
6.x#4124lukeholder wants to merge 424 commits into
lukeholder wants to merge 424 commits into
Conversation
Targets the handful of rules with real custom matching logic (not the mechanical 1:1 base-rule ports), all testable without a database: - CouponCodeConditionRule: case-insensitive EQ/NE matching - TotalDiscountConditionRule: negated-value comparison (discounts are stored as negative amounts, but the rule's configured value is entered as positive) - PaymentGatewayConditionRule: legacy single-value <-> values[] B/C shim - DiscountGroupConditionRule: the custom "is in all of" operator No condition tests existed anywhere in this codebase before this.
Added/Deprecated entries for the new CraftCms\Commerce\*\Conditions classes across Catalog, Catalog Pricing, Customers, Orders, Payments, Promotions, Purchasables, Shipping, Stores, and Tax, plus behavioral notes for the consumers now wired to them (Discount, ShippingRule, BaseShippingMethod, Zone/ZoneInterface, CatalogPricingRule, legacy Gateway).
Ports craft\commerce\elements\Transfer, elements\db\TransferQuery, elements\conditions\transfers\TransferCondition, services\Transfers, and fieldlayoutelements\TransferManagementField to CraftCms\Commerce\Transfer\*, following the same base-condition/element/query conventions established for the rest of the condition-system migration. Legacy classes become class_alias stubs; Transfer moves into src/Plugin.php's $elementTypes, replacing the legacy Elements::EVENT_REGISTER_ELEMENT_TYPES bridge in src-yii2/Plugin.php. Rewires TransfersController, SettingsController, and HasServices to reference the new classes directly, and adds Pest unit tests for the element's pure-logic methods (detail totals, status transitions, location validation).
…s to Laravel in src/
Ports craft\commerce\elements\actions\{CopyLoadCartUrl,CreateDiscount,CreateSale,
DownloadOrderPdfAction,SetDefaultVariant,UpdateOrderStatus}, the remaining
fieldlayoutelements\* classes (ProductTitleField, VariantTitleField, VariantsField,
UserAddressSettings, and the 8 Purchasable*Field classes), and the fields\Products/
Variants custom field types to CraftCms\Commerce\*, following the established
element-action/field-layout-element/relation-field base classes. Legacy classes
become class_alias stubs; Product/Variant/UpdateOrderStatus/SetDefaultVariant move
into src/Plugin.php's $elementTypes/$fieldTypes arrays, replacing the legacy
Fields::EVENT_REGISTER_FIELD_TYPES bridge in src-yii2/Plugin.php.
Also deletes src-yii2/linktypes/Product.php, which was fully superseded by the
already-migrated and already-registered CraftCms\Commerce\Catalog\LinkTypes\ProductLinkType
and had no remaining references anywhere in the codebase.
Replaces the manual per-driver SQL string building in Stat::getChartQueryOptionsByInterval() (MySQL CONVERT_TZ/PostgreSQL AT TIME ZONE/SQLite passthrough for timezone conversion, then EXTRACT/strftime/DATE() for day and month grouping keys) and the selectRaw()/groupByRaw()/ orderByRaw()/DB::raw() calls scattered across the individual Stats classes (SUM, COUNT, IFNULL vs COALESCE, CASE WHEN) with typed, composable query builder expressions. Adds four small expression classes under src/Support/Expressions/ (LocalTimestamp, DateOnly, MonthKey, Round) following the same driver-detection pattern as tpetry/laravel-query-expressions, for the SQL constructs the package doesn't provide (calendar month/day truncation with timezone conversion) — used together with the package's own Sum, Count, Coalesce, CaseGroup/CaseRule, Alias, and Value expressions everywhere else. Behavior-preserving: all Stats feature tests pass except the one pre-existing, unrelated TotalOrdersTest failure that predates this change.
…ling cms-6 checkout phpstan.neon pointed scanFiles/stubFiles at ../cms-6/yii2-adapter/..., which only resolves when cms-6 is checked out as a sibling directory (true in the local ddev dev setup, where /tmp/packages/cms-6 and /tmp/packages/commerce-6 are siblings, but not in CI or any standalone checkout of this repo). Point them at vendor/craftcms/yii2-adapter/... instead, which composer already installs identically everywhere and matches how every other CI job in this repo already resolves craftcms/cms and craftcms/yii2-adapter. Also pin parallel.maximumNumberOfProcesses to 1. Discovered while testing the above: PHPStan's worker processes each independently reflect on classes reached through class_alias() chains (the legacy src-yii2/ -> src/ stubs), and depending on which worker resolves a given alias target first, whether it can trace the chain is non-deterministic between otherwise-identical runs — surfacing as argument.type/method.notFound errors, or stale @phpstan-ignore-next-line suppressions, that flip between two or three call sites in Payment/Gateway/Gateways.php and Payment/Transactions.php from run to run. Single-process analysis is slower but removes the race entirely; confirmed 3 consecutive clean runs locally after pinning it.
Completes the Gateways domain migration: Gateway.php (inlining GatewayTrait), the three concrete gateway types, and the helpers/PaymentForm.php stub, with all consumers rewired to the new namespace. Fixes the CI phpstan failures caused by craft\commerce\base\Gateway still being a real, non-aliased class with legacy type-hints that mismatched GatewayInterface, and removes ~19 now-stale @phpstan-ignore-next-line suppressions this made provable.
2.4.5's PHPStanContainerMemento reflected into a private $container property on PHPStan\Parser\RichParser that no longer exists as of PHPStan 2.2.x, crashing rector process with MissingPrivatePropertyException. 2.6.3 requires phpstan/phpstan ^2.2.6, which includes the matching container-compatibility fix (rectorphp/rector#8208).
PR #4124 (head 6.x, base 5.x) stays open for the whole migration, so every push already triggers a pull_request run. The push: branches: [6.x] trigger was firing a second, redundant full CI run for the same commit.
Stores::afterDeleteCraftSiteHandler(): on a single-store install, reassigning the primary store to "another" store after the last site is deleted found no other store (firstWhere() returned null) and then dereferenced it. Skip the reassignment when there's nothing to promote. ProductTypes::getViewableProductTypeIds()/getCreatableProductTypeIds(): both called $user->can(...) without checking that request()->craftUser() returned a user, fataling in console/queue contexts. Mirrors the console/null-user guard already used by the sibling getViewableProductTypes().
…namespace composer.json maps craft\commerce\ to src-yii2/, and every file in these directories declares (or aliases into) namespace craft\commerce\base or craft\commerce\enums (lowercase). The directories were Base/ and Enums/ (capitalized), which macOS's case-insensitive filesystem silently tolerates but a case-sensitive Linux filesystem does not. This broke autoloading for every class under craft\commerce\base\* (StoreTrait, GatewayTrait, Model, Stat, etc.) and craft\commerce\enums\* on GitHub Actions CI runners, which never surfaced locally because ddev mounts the Mac host filesystem into the Linux container. It also explains why the Tests/Feature and Tests/Arch CI jobs have effectively never run to completion until now — they were gated behind Rector, which was itself broken until the previous commit.
… live autoload root
Two arch rules, both scoped to src/ and verified to actually catch violations
(Pest's toUse()/toBeUsedIn() resolve targets as classes/namespaces via its
ObjectsRepository, so plain function names outside its small hardcoded
core-language-construct list, and Class::method static-call strings, are
silently never matched — confirmed empirically before relying on either):
- No debug functions (die/dd/dump/env) in src/.
- src/ must not reference legacy Craft core classes, excluding craft\commerce
(allowed during the migration per this repo's CLAUDE.md).
Getting any arch() rule to run at all required moving
src-yii2/test/{fixtures/elements/ProductFixture.php,mockclasses/Purchasable.php}
to tests-yii2/, matching where every other pre-Pest reference/porting fixture
already lives. Pest's arch plugin enumerates every PSR-4 namespace declared in
composer.json to build its analysis graph, not just the expect() target, so
these two forgotten files sitting inside the live craft\commerce\ (src-yii2/)
autoload root — referencing craft\base\ElementInterface, which isn't even
aliased anymore — fataled the whole test run before either rule could
evaluate. Updated their two internal namespace declarations and the three
call sites that imported them (craft\commerce\test\* -> craftcommercetests\*)
to match their new location.
…rce\Gql\...) Same class of bug as the earlier src-yii2/Base and src-yii2/Enums fix, this time in the new src/ tree: namespace CraftCms\Commerce\Gql\Handlers (etc.) declared throughout, but the directory was src/gql/handlers (lowercase). Unlike the earlier StoreTrait case, this didn't fatal — Plugin::boot() registers CraftCms\Commerce\Gql\Handlers\HasProduct as a GQL argument handler via is_a($handler, ArgumentHandlerInterface::class, true), and is_a() with $allow_string swallows a failed autoload and just returns false. That surfaced as "Argument handler [...] must implement [ArgumentHandlerInterface]" on Tests/Feature CI, which was misleading: the class autoloads fine on macOS (case-insensitive host filesystem mounted into the ddev container) and so genuinely does implement the interface — it just couldn't be found by name on GitHub's case-sensitive Linux runners. Renamed src/gql -> src/Gql, handlers -> Handlers, types -> Types, types/input -> Types/Input, types/input/criteria -> Types/Input/Criteria. Re-scanned both src/ and src-yii2/ in full for any other namespace/directory casing mismatches; none remain.
…ueries, arguments)
Completes the GraphQL migration checklist: Arguments/Elements/{Product,Variant},
Interfaces/Elements/{Product,Variant}, Types/Elements/{Product,Variant},
Types/Generators/{ProductType,VariantType}, Types/Input/{IntFalse,Product,Variant},
Types/SaleType, Resolvers/Elements/{Product,Variant}, and Queries/{Product,Variant},
all under CraftCms\Commerce\Gql\. helpers/Gql.php was already fully ported in an
earlier commit; its legacy src-yii2 counterpart is now a thin deprecated subclass
matching the established pattern.
The "wire up schema-registration" checklist item turned out to already be mostly
done — Plugin.php already registered the GqlArguments handlers and the
GqlSchemaComponentsResolving/GqlEagerLoadableFieldsResolving listeners. The only
missing piece was populating the new $gqlTypes/$gqlQueries properties the base
Plugin class's HasGql concern reads automatically, replacing the legacy
Event::on(Gql::EVENT_REGISTER_GQL_TYPES/QUERIES) wiring entirely.
Also fixes 3 stray legacy craft\gql\* imports in already-migrated files
(Catalog/Variants.php, Gql/Types/Input/Criteria/{Product,Variant}Relation.php)
that the new Arch tests would otherwise have flagged, and points those two
Criteria classes at the new Arguments classes instead of the legacy ones.
Verified beyond phpstan/check-cs: manually executed a GraphQL query through
products -> variants -> sales against a seeded schema, and prebuilt/validated
the full schema (introspection path), both via a throwaway test since this
repo has no GQL feature-test harness yet.
…on on SQLite
Two compounding bugs, both in date/timezone handling around the "today" stat:
1. LocalTimestamp's SQLite branch returned the raw (UTC) column unconverted,
on the assumption "SQLite is only used by the test suite, which always
runs in UTC" — false: tests/TestCase.php pins the app's timezone to
America/Los_Angeles for determinism. Day/month grouping (TotalOrders and
every other stat using getChartQueryOptionsByInterval) would bucket orders
under their UTC calendar date instead of the app's configured one,
splitting a single day's data across two chart entries whenever UTC and
LA disagreed on the current date (i.e. for most of each 24-hour period).
Fixed by resolving the configured timezone's current UTC offset in PHP
(DateTimeZone::getOffset(), which accounts for DST) and applying it via
SQLite's datetime(column, '+/-N minutes') modifier, since SQLite has no
named-timezone SQL functions to call directly.
2. Separately, TotalOrdersTest's "today" dataset computed its expected
start/end dates via new DateTime('now') inside the ->with() array. Pest
resolves dataset closures before beforeEach()/app boot, i.e. before
TestCase::setUp() pins the timezone — so the dataset's "now" could be
read under a different default timezone than the one Stat itself later
uses when it independently recomputes "today" inside TotalOrders's own
constructor. Moved the date computation into the test body, after the
fixture (and app boot) has already run, so both sides agree by construction.
Verified with 5 repeated runs of the previously-100%-reproducible failure,
the full Stats suite, and the full test suite (109/109 passing, first fully
clean run this session).
Ports the five remaining Yii2 console controllers to CraftCms\Commerce\Console\Commands\*,
following the Illuminate\Console\Command + CraftCommand pattern already established by
the resave commands. GatewaysController's two real actions (list, webhook-url) split into
separate command classes, matching how cms-6 splits its own multi-action controllers
(e.g. project-config:get/set/apply). Legacy `commerce/*` CLI routes are preserved as
command aliases via $aliases, so existing scripts/muscle-memory keep working.
Destructive/interactive flows were translated to their Laravel-idiomatic equivalents where
that's a strict improvement with no behavior change for the required inputs: ResetData now
uses ConfirmableTrait (a --force bypass plus a components->task()-driven, single
DB::transaction()-wrapped delete, rather than the ad-hoc yes/no string prompt + manual
begin/commit/rollBack); ExampleTemplates and TransferCustomerData use Laravel's own
ask()/confirm() and only prompt for options not already supplied on the command line.
console/Controller.php (an empty pass-through base with nothing else extending it once the
above landed) is deleted outright, not stubbed — console controllers are CLI entry points
invoked by route string, not classes anything else in the codebase instantiates or
type-hints against, so there's no back-compat surface to preserve the way there is for
services/models.
Verified against a real dev install (not just phpstan/tests), which surfaced two real,
unrelated pre-existing bugs the migrated code paths hadn't exercised before:
- Gateway::set{Billing,Shipping}AddressCondition() didn't accept null, despite handling it
in the method body (setOrderCondition already did) - throws the moment any real gateway
config has a null condition, i.e. immediately for `commerce:gateways:list`.
- CatalogPricing::setQueueProgress() called method_exists(null, ...), which throws a
TypeError under PHP 8's stricter argument types - hits every no-queue call, i.e.
immediately for `commerce:pricing-catalog:generate`.
Both fixed. commerce:reset-data was verified by code review (DB::transaction wrapping,
correct table/column names) rather than executed live, since the harness's destructive-
action guard correctly declined to run a bulk-delete command even against a dev database
confirmed to have zero rows in every affected table.
Follow-up to 0a53428: a bad pathspec in that commit's `git add` (src-yii2/console, already handled by a separate git rm) silently aborted staging every other file passed in the same invocation, so Plugin.php's $commands registration for the 5 new console commands, the Gateway.php/CatalogPricing.php bugfixes those commands surfaced, and the CHANGELOG-WIP.md entry never actually made it into that commit — only the new command classes and the src-yii2 deletions did. The commands existed but were never wired up. Re-verified with phpstan and the full test suite now that Plugin.php's registration is actually included.
…ectConfigData,Purchasable}.php
All 8 already had complete src/Helpers/ counterparts from earlier migration work,
so this is mostly the usual legacy-stub conversion + consumer rewiring — but
verifying each one caught two real, latent bugs:
- src/Helpers/{Cp,Currency,Purchasable}.php still imported the legacy core
craft\helpers\Cp instead of CraftCms\Cms\Cp\FormFields (fieldHtml/moneyInputHtml/
textHtml) or the CraftCms\Cms template()/TemplateMode helpers (renderTemplate)
or FormFields::lightswitchFromConfig()->toHtml() (lightswitchHtml, itself
deprecated in cms-6's own yii2-adapter). This is a real gap in the "src/ should
not reference legacy Craft core classes" Arch rule added earlier this session:
Pest-arch's dependency-layer resolution excludes vendor-directory namespaces
entirely, so any craft\* class living under vendor/craftcms/cms (as opposed to
this repo's own src-yii2/) is invisible to it and can slip through undetected.
That's a separate, wider-reaching finding worth its own follow-up pass.
- CraftCms\Commerce\Helpers\Localization no longer `extends \craft\helpers\
Localization` (it only needs normalizePercentage(), and normalizeNumber() is
called via CraftCms\Cms\Support\Facades\I18N internally), but
PaymentsController.php called the commerce subclass's *inherited*
normalizeNumber() directly - undefined once the extends was dropped. Pointed
it at the I18N facade instead, matching Localization's own internal usage.
Rewired the 15 already-migrated src/ consumers that still imported the legacy
craft\commerce\helpers\* classes to use CraftCms\Commerce\Helpers\* directly.
Verified beyond phpstan/tests: called the FormFields-backed methods
(Cp::taxZoneFieldHtml, Currency::moneyInputHtml, Purchasable::skuInputHtml,
Purchasable::availableForPurchaseInputHtml) against a real dev install via
tinker, since none of the existing test suite exercises CP-rendering helpers.
…idgetTrait,StoreTrait,TaxEngineInterface,TaxIdValidatorInterface,ZoneInterface} All 9 already had complete src/ counterparts from earlier migration work (most already carried @deprecated docblocks pointing at them, and the CHANGELOG-WIP.md entries for their replacements already existed - this was purely the stub-conversion + verification pass). StatTrait is deliberately left as a real, untouched legacy trait, matching the GatewayTrait precedent from the Gateway migration: its properties were merged directly onto the new Stat class rather than ported to a dedicated trait, so there's no clean class_alias target, and it costs nothing to leave as free-standing legacy code for any third-party code still using it directly. Model.php is a genuine dead end: an empty pass-through subclass of craft\base\Model with zero consumers anywhere in the codebase. Pointed its stub at CraftCms\Cms\Component\Component directly (the documented modern base for what used to extend craft\base\Model - see docs/6.x/extend/models.md and every already-migrated Commerce model) rather than a nonexistent "new commerce Model" class. Comparing old vs new caught one real fidelity gap: legacy Stat implements both StatInterface and HasStoreInterface, but the new class only implemented StatInterface even though StoreTrait already satisfies HasStoreInterface's contract - just a dropped `implements` clause. Added it back. Rewired the 2 already-migrated src/ consumers (Catalog/Elements/Product.php, Store/Models/SiteStore.php) still importing the legacy craft\commerce\base\StoreTrait.
…ias TypeError race in service delegation layer
Wires the product/variant/transfer element-edit screens to the generic
EditElementController in routes/cp.php, moves Variables::getDonation() to
Plugin::getDonation(), and deletes LegacyRoutingModule.php (dead code - no
controllers remain in craft\commerce\controllers for it to resolve).
Also fixes a TypeError that any craft\commerce\services\* delegation method
returning/accepting a craft\commerce\{models,elements}\* class alias could
throw on its first call in a process (class_alias's own autoload racing the
return-type check). All 22 affected wrappers now type-hint against the
CraftCms\Commerce\* class directly, and Transfer::canView()/canSave()/
canDelete() now check the permission that's actually registered
(commerce-manageInventoryTransfers, not commerce-manageTransfers).
…ce/settings/producttypes
ProductTypesController::productTypeIndex()/editProductType() never passed
readOnly to their templates even though index.twig/_edit.twig both reference
it, throwing a Twig RuntimeError. productTypeIndex() also wrapped
index.twig's own `{% extends "commerce/_layouts/settings" %}` in a
CpScreenResponse, double-rendering the CP chrome - switched it to
pageTemplate() like every other index-listing controller (OrderStatuses,
Pdfs, etc.), since editProductType()'s _edit.twig is content-only and
CpScreenResponse is correct there.
Add a persistent Commerce development workbench
[6.x] Render product sources in the secondary nav
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
4 participants
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.
https://linear.app/craftcms/issue/COM-613/laravel-port