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:

  1. _subscription_manager_sync_remote_subscriptions() is still a procedural helper in the .module file. 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.
  2. cron() drains the subscription_manager_remote_sync queue with a hand-rolled claimItem()/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 shrink cron() 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

  1. Add SubscriptionManagerService::syncRemoteSubscriptions(UserInterface $account): void containing the helper's logic, with its dependencies injected rather than reached via \Drupal:: statics. Inject the service into SubscriptionManagerHooks (autowiring is already enabled for that class) and update userInsert() to call it.
  2. Delete _subscription_manager_sync_remote_subscriptions() from the .module file. 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.
  3. 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 from cron(), leaving only the expiry sweep.
  4. 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.

Command icon 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

colan created an issue. See original summary.

colan’s picture

Also, 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.

colan’s picture

colan’s picture

Status: Active » Needs review

MR opened, implementing all four points plus the poison-item fix from #3:

  • SubscriptionManagerService::syncRemoteSubscriptions(UserInterface $account): void holds the moved logic with injected dependencies; the procedural helper is deleted (module-internal, no callers left).
  • Failure policy moved to the callers — one deliberate behavior change beyond the plain move: the old helper caught every throwable internally, which would have defeated queue retries after the QueueWorker conversion. The service method now lets exceptions propagate; 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.
  • Tests: LoginRemoteSyncDeferralTest now drains via a real cron service run, exercising the QueueWorker path end to end. New RemoteSyncQueueTest covers 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.

  • colan committed 1a973222 on 1.0.x
    Issue #3616880: Move the remote-sync helper into the service and drain...
colan’s picture

Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.