Problem/Motivation
After the Drupal 12 hook conversion (#3603421: Automated Drupal 12 compatibility fixes for subscription_manager 1.0.x-dev), all hook implementations live in the SubscriptionManagerHooks class, but two legacy procedural patterns remain in the code path they share:
_subscription_manager_sync_remote_subscriptions()is still a procedural helper in the.modulefile. It is domain logic (reconcile a user's remote billing contracts into local subscription entities), not glue: keeping it procedural leaves it dependent on\Drupal::statics, hard to unit test, and stranded in a file that should now contain only#[LegacyHook]shims and form-submit callbacks. Both of its callers (userInsert()and the cron queue drain) are already methods on the hooks class.cron()drains thesubscription_manager_remote_syncqueue with a hand-rolledclaimItem()/deleteItem()/releaseItem()loop. Core's QueueWorker plugin system exists precisely for this: it drains declared queues during cron on its own, with standard retry semantics, and would shrinkcron()to just the subscription-expiry sweep.
A hooks class should stay a thin dispatch layer, so the helper's destination is the service layer rather than SubscriptionManagerHooks.
Proposed resolution
- Add
SubscriptionManagerService::syncRemoteSubscriptions(UserInterface $account): voidcontaining the helper's logic, with its dependencies injected rather than reached via\Drupal::statics. Inject the service intoSubscriptionManagerHooks(autowiring is already enabled for that class) and updateuserInsert()to call it. - Delete
_subscription_manager_sync_remote_subscriptions()from the.modulefile. The underscore prefix marks it module-internal and it has no callers outside the module, so no deprecation cycle is needed. It is not a hook, so no#[LegacyHook]shim applies. - Add a QueueWorker plugin for
subscription_manager_remote_sync(cron time declared in the plugin definition) that loads the user and calls the new service method, throwing to trigger core's standard release-and-retry handling. Remove the manual drain loop fromcron(), leaving only the expiry sweep. - Port the existing kernel coverage (
LoginRemoteSyncDeferralTest) to exercise the QueueWorker path, and add a unit or kernel test for the new service method directly.
If review prefers, item 3 can split into its own issue; it is included here because it touches the same call path and the two changes review most naturally together.
Remaining tasks
Sequence against related work: land after the #3603421: Automated Drupal 12 compatibility fixes for subscription_manager 1.0.x-dev hook conversion (this builds on the hooks class), and coordinate with #3616770: Self-healing remote subscription sync only queries the default connector, which rewrites this same routine to iterate all installed connectors instead of only the default. Doing this refactor first gives that issue a clean, injectable, testable home for its logic; doing them together in one branch is also reasonable.
User interface changes
None.
API changes
New public method SubscriptionManagerService::syncRemoteSubscriptions(). Removal of the module-internal procedural helper (underscore-prefixed, no external callers, no BC concern). New QueueWorker plugin; the queue name is unchanged, so already-enqueued items drain normally after deployment.
Data model changes
None.
Issue fork subscription_manager-3616880
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
colanAlso, Cron's queue drain has a poison-pill problem. On exception it releases the item and breaks, so one persistently failing item permanently blocks the whole queue every run. This is strong reinforcement for the QueueWorker conversion, which gets core's per-item handling.
Comment #3
colanComment #5
colanMR opened, implementing all four points plus the poison-item fix from #3:
SubscriptionManagerService::syncRemoteSubscriptions(UserInterface $account): voidholds the moved logic with injected dependencies; the procedural helper is deleted (module-internal, no callers left).userInsert()keeps the old fail-open behavior with its own catch (a billing-API failure must never break user creation), and the queue path gets core's standard handling.RemoteSyncWorker(attribute-based plugin,cron: time 30) drains the unchanged queue name, so already-enqueued items keep draining across deployment. Deleted-user and malformed items are dropped silently; everything else bubbles so core logs, skips to the next item, and retries after lease expiry — which fixes the poison-item stall from #3, where the hand-rolled loop's release-and-break let one persistently failing item block the whole queue every run.cron()is now only the expiry sweep.LoginRemoteSyncDeferralTestnow drains via a realcronservice run, exercising the QueueWorker path end to end. NewRemoteSyncQueueTestcovers the service method directly (sync + active-local short-circuit), worker item handling, and a poison-item regression: a failing item enqueued in front of a healthy one no longer blocks it (RED under the old loop), with the failing item retained for retry. The spy connector gained a per-uid simulated remote failure to drive it.One summary note is stale: the hooks service no longer autowires (explicit arguments since the #3603421 phpstan work), but the subscription manager service was already injected there, so
userInsert()needed no wiring changes. Sequencing is as proposed: this gives #3616770: Self-healing remote subscription sync only queries the default connector a clean, injectable home — its all-connectors rewrite now targets one service method.Comment #7
colan