Cleanup omnibus: dead config keys, missing token dependency, per-role user saves, page_attachments overhead, drush alias, field length, translatability, misc

Adding items discovered during the Phase 1–3 work, so they have a home:

• Entity annotations → attributes: subscription and subscription_plan still use @ContentEntityType annotations, deprecated for removal in Drupal 13 — the source of the two standing deprecations in every test run. The new subscription_charge entity (#3618739: Add a charge-based connector mode with a local billing cycle engine) already uses the attribute and can serve as the template.
• Test metadata → attributes: PostPurchaseTokenServiceTest declares @covers/@coversDefaultClass in doc comments, which PHPUnit 11 deprecates and PHPUnit 12 drops (currently ~56 "PHPUnit Deprecations" per suite run, inflated by process isolation). Convert to #[CoversClass]/#[CoversMethod].
• The plan show field fails validation: calling validate() on any subscription_plan entity surfaces show.0.value: This value should be of the correct primitive type. — a pre-existing definition quirk (nothing called validate() on plans before the scheduling-mode constraint in #3618739: Add a charge-based connector mode with a local billing cycle engine, whose test scopes around it). Likely the boolean field's default; worth fixing so plan validation is clean.
• DrupalPractice warnings the drupal.org CI does not surface (visible in a full local phpcs run with warnings enabled): unused variables in subscription_entity.page.inc / subscription_plan.page.inc; t() calls in SubscriptionManagerAdminForm and SubscriptionEntityDeleteForm; User::load() calls in SubscriptionManagerController (five sites); Role::loadMultiple() in the drush commands; unused $entity variables in both entity forms; blank-line-after-inline-comment nits in both entity interfaces.

One more for the annotations-to-attributes bucket: the two Views field plugins, SubscriptionRemoteStatus and SubscriptionWillRenew, still use @ViewsField annotations (deprecated, removed in Drupal 13 — same change record as the entity annotations). Surfaced by the new DefaultViewsTest in #3618739: Add a charge-based connector mode with a local billing cycle engine, which is the first test to load them.

Test-coverage gaps in pre-existing code, found while auditing coverage after #3618739: Add a charge-based connector mode with a local billing cycle engine (charge-cycle gaps are noted on #3619074: Succeeded charge records are terminal, so payer-initiated reversals (SEPA refunds, Bacs indemnity claims, chargebacks) have nowhere to land and #3618740: Add a dunning framework: past_due state, scheduled retries, grace period, and lifecycle notifications, which rework those surfaces):

• The drush commands (SubscriptionManagerCommands) have no tests at all.
• The two Views field plugins (SubscriptionRemoteStatus, SubscriptionWillRenew) are never rendered by any test — DefaultViewsTest executes the shipped views, which don't use them. A natural fix: add the fields to the default subscriptions view, which covers both at once.
• No update-path tests for updates 10001–10018. Standard contrib omission (they need heavy functional fixtures), noting it here so the blind spot is a recorded decision rather than an oversight.

Another item, found while designing #3616934: Deleting a user deletes local subscription entities but never cancels the remote contract, so billing continues: the membership-portal half of SubscriptionManagerConnectorInterface has no caller.

Four methods — supportsInlineActions(), getSubscriptionDisplayData(), getAvailableActions() and executeAction() — describe a portal page that renders the connector's display data and, when the connector allows it, inline pause/resume/cancel buttons. That page is not in the module: nothing in src/, the templates, or the routes calls any of the four. (refreshSubscription(), the fifth method from the same era, is used, by the refresh throttle and the Drush refresh command, and stays.) The only reference anywhere was the find-duplicates --cancel Drush helper falling back to executeAction('cancel'), and #3616934: Deleting a user deletes local subscription entities but never cancels the remote contract, so billing continues moves that onto the new RemoteCancellingConnectorInterface. Every shipped connector answers supportsInlineActions() = FALSE; Stripe still implements executeAction('cancel') regardless, the others return a "manage this in the portal" refusal.

Two ways to resolve it, to decide here:

1. Remove the four methods from the interface. Pre-1.0, so no BC layer, but it is a breaking change for every connector (all four implement them) — a change record, and a bullet in the connector catch-up issues the roadmap files after beta7. Connectors keep any cancel code they have behind RemoteCancellingConnectorInterface instead.
2. Build the portal page the methods were written for. If we ever do, it should be driven by capability interfaces (a Cancel button appears when the connector implements RemoteCancellingConnectorInterface, an instrument-replacement button when it implements InstrumentReplacingConnectorInterface), not by a stringly-typed action switch — which is to say the four methods would not survive that either.

The four methods are dead weight that every new connector author has to implement and every reviewer has to wonder about, and the shape of any future portal is better served by the interfaces the module has adopted since. The Views-based listings and the billing-history page (#3621282: Show subscribers their invoice history in the membership portal) cover the display side today.

However, it would be nice to have the portal-page functionality provided by the interfaces, so maybe we should create a follow-up issue for that? it would need to be clear about the functionality that it would add, i.e. what we're missing now.

#3616936: SubscribeMenuLink declares the user.roles cache context but varies per user, leaking Join/Upgrade labels across users raises core_version_requirement to ^11.3 (Drupal 10 reaches end of life alongside Drupal 12, and nothing ever tested this module on it). That leaves Drupal 10 remnants to remove here, all already marked @todo Remove once Drupal 10 support is dropped or made redundant by core 11.3's plugin autowiring:

1. The #[LegacyHook] shims in subscription_manager.module — twelve procedural functions that only exist to call the OOP hook classes on cores without hook attributes: help, form_user_login_form_alter, form_user_register_form_alter, page_attachments, user_presave, user_predelete, user_login, user_insert, subscription_insert, subscription_update, subscription_delete, cron. With them gone the .module file holds only the granted-roles backfill helper, which can move into a service or the install file.
2. Hand-written create() factories, now that PluginBase::create() autowires constructors (as SubscribeMenuLink does since #3616936: SubscribeMenuLink declares the user.roles cache context but varies per user, leaking Join/Upgrade labels across users): the three queue workers (CycleWorker, DunningRetryWorker, RemoteSyncWorker) can drop create() outright; SubscriptionManagerController, SubscriptionManagerAdminForm and SupportedSchedulingModeConstraintValidator can use core's AutowireTrait. In every case the subscription_manager and logger.channel.subscription_manager services need a #[Autowire(service: '…')] attribute, since neither has an interface alias — or we add class-name aliases to services.yml and let the existing @todo on the hooks service definition (restore autowire: true) be resolved the same way.
3. composer.json has an empty require: add "drupal/core": "^11.3" so Composer enforces what info.yml declares.
4. Test connectors in tests/modules/ implement ContainerFactoryPluginInterface::create() by hand too; same treatment, so the test modules model the recommended connector shape.

Not for this issue: adding Drupal 12 to the requirement and OPT_IN_TEST_NEXT_MAJOR to the CI is the roadmap's own Drupal 12 step, since it needs the 12.0.x deprecation report first.

Scoping pass before implementation, with every item from the summary and comments #3–#7 checked against current 1.0.x. Grouped below by what I'd make each commit; a few items are already done or need a decision.

Already done, nothing to do

• page_attachments overhead (item 4): the hook early-returns for anonymous users, does one getUserLocalSubscriptionToAnyPlan() call, and no longer User::load()s — and PageAttachmentsQueryBudgetTest pins all three. Done as part of the Phase 1 work.

Still to do — the state of each

• Dead config keys (item 1): site_id and api_key are read nowhere — in this module or in any of the four connectors — and neither is log_webhooks (not in the summary; each connector has its own log_webhooks in its own settings). One update hook clears the three keys; the schema loses them. manage_route is live. custom_subscribe_url is live too, just not here: this module only stores it, but Stripe Subscription's and Shopify Subscription's subscribe controllers read it from subscription_manager.settings and redirect there (token-replaced with the user) instead of starting their own checkout — it has been that way since the 2024 refactor. So it stays; what needs fixing is its description ("Override the default subscribe for the default connector" explains nothing, and there is no default-connector setting any more) — say that it is honoured by connectors that support it, and what tokens do. Whether it should instead be honoured centrally, or per connector in the chooser entries, is a design question for a follow-up, not a cleanup.
• Undeclared token dependency (item 2): the settings form still renders #theme token_tree_link with no guard and no dependency. Guard it with moduleExists('token'); core's own token replacement works without the contrib module, so no dependency is needed.
• Per-role user saves (item 3): SubscriptionEntity::addRoles() and removeRoles() both call $owner->save() inside the role loop. Collect, then save once.
• Drush alias (item 5): @aliases foo is still on sync-plans; the other three commands have proper sm-* aliases.
• Name length (item 6): max_length is 50 on both the subscription and the plan name fields. I'd widen both to 255 in one update (plan names come from the remote service and are the likelier to overflow).
• Translatability (item 7): the remaining t()-in-class warnings are eight in the admin form and one in the delete form; the two Views field plugins return untranslated "Yes"/"No"/"Unknown" with inline style="color: …" markup, which should become t() plus CSS classes.
• Misc (item 8): .gitigore (contents: .idea) is still there; composer.json still points at http://cgit.drupalcode.org/…; two route paths lack the leading slash (user/{user}/manage-subscription, user/{user}/billing-history). I could not re-find the docblock typo the summary mentions; if nobody remembers it, it's dropped.
• Entity annotations → attributes (#3): SubscriptionEntity and SubscriptionPlanEntity still use @ContentEntityType (every test run logs the deprecation); SubscriptionChargeEntity is already an attribute and is the model.
• PHPUnit metadata (#3): all 182 runner deprecations are one kind — "Metadata found in doc-comment" — from @group on 26 test classes and @covers/@dataProvider on methods. Mechanical conversion to #[Group], #[CoversClass], #[DataProvider].
• The show field quirk (#3): the boolean base field has setDefaultValue(''), so a freshly created plan fails the primitive-type constraint until the value is set explicitly (CycleEngineTest works around it). Default to FALSE; no storage change, so no update hook.
• DrupalPractice warnings (#3): 21 in total — five User::load() calls in the controller (inject the entity type manager; ControllerBase already offers it), one Role::loadMultiple() in the Drush class, unused $entity variables in the two entity forms and in the two legacy *.page.inc preprocess files, and two comment-spacing nits in the entity interfaces. The *.page.inc preprocess functions are live (both entity templates ship in templates/), so they stay; only the unused variables go.
• @ViewsField annotations (#4): both field plugins; #[ViewsField] is available on every supported core.
• Coverage gaps (#5): DefaultViewsTest executes the three views but renders nothing, so the two field plugins' render() is still unexecuted — extend it to render a row. Drush is the larger question, below.
• Drupal 10 remnants (#7): as described there. From #3616936: SubscribeMenuLink declares the user.roles cache context but varies per user, leaking Join/Upgrade labels across users: no readonly on autowired plugin properties (DependencySerializationTrait).
• Portal API (#6): not here. Rather than delete the four unused methods, the portal they were written for gets built properly, on capability interfaces, in its own issue — charge-based connectors have no hosted portal to redirect to, so the module needs a native one anyway. The methods stay untouched until that issue replaces them.

Drush coverage

Drupal's GitLab templates do install Drush in the composer job (the .require-drush step, allowed to fail), so the command class is loadable in CI. But the commands are Drush container services, not Drupal ones, and find-duplicates alone is ~500 lines of logic that lives inside the command class with io()/logger() calls throughout. Two ways in: (a) instantiate the command class by hand in a Kernel test and drive its public methods — brittle, and still coupled to Drush's runtime; or (b) extract the logic into Drupal services (a duplicate-subscription resolver, a reconciler, a plan syncer the service already mostly has) that the commands merely call and format, and test the services. Going with (b): it is what made RemoteCancellation testable in #3616934: Deleting a user deletes local subscription entities but never cancels the remote contract, so billing continues, and find-duplicates --cancel is a money-moving path that deserves real tests. It is the largest single item here.

Proposed commits

1. Dead config keys (three), update hook, schema; custom_subscribe_url description and the token-help guard.
2. Drupal 10 removal: #[LegacyHook] shims, composer.json core requirement, autowire plugins/controller/form/validator/test connectors.
3. Annotations → attributes: both entities, both Views field plugins.
4. PHPUnit metadata → attributes.
5. Entity fixes with update hooks/tests: single user save per role change, name length 255, show default.
6. Translatability and DrupalPractice: $this->t(), Views plugin strings and classes, controller DI, unused variables, interface comment nits.
7. Misc: .gitignore, composer URLs, route slashes, Drush alias.
8. Coverage: render the Views field plugins; Drush logic extraction and tests (per the decision above).

Change record: one, covering the removed config keys, the widened name fields, and the new services behind the Drush commands.

Decisions (ratified): (1) the portal methods are left alone here and handled by a follow-up issue that builds the portal on capability interfaces (#3627571: Build the membership portal page on capability interfaces, replacing the unused inline-actions API); (2) remove the three dead keys, keep custom_subscribe_url with a truthful description and a guarded token help; (3) Drush: extract the logic into Drupal services and test those; (4) widen both name fields; (5) one MR, reviewed commit by commit.

Fixed in the MR as scoped in #8, in eleven commits meant to be reviewed one at a time (each is one theme; the test commits follow the code they cover). Everything below ran against the DDEV copy of a real site, including both update hooks.

1. Dead config keys. site_id, api_key and log_webhooks are gone from the install config and schema; update 10026 clears them. custom_subscribe_url stays with a description that says what it actually does (Stripe Subscription and Shopify Subscription read it from here), and the token-browser link is only rendered when the Token module is installed. New SettingsFormTest.
2. Drupal 10 removal and autowiring. The twelve #[LegacyHook] stubs are gone, so .module holds only the two form submit callbacks the hook class registers by name and the two reconcilers the update hooks run. composer.json requires drupal/core ^11.3. Every hand-written create() is replaced by core autowiring: PluginBase::create() for the queue workers and the test connectors, AutowireTrait for the controller, the settings form and the constraint validator, autowire: true for the hooks service. services.yml gains class-name aliases for this module's services so they resolve by type; the logger channel is named with #[Autowire(service: 'logger.channel.subscription_manager')]. (Connector authors: the same pattern works for connector plugins; no readonly on autowired plugin properties, because of DependencySerializationTrait.)
3. Entity and Views attributes. @ContentEntityType on both remaining entities and @ViewsField on both field plugins are attributes; the entities also gain label_collection. The annotation test module keeps the connector annotation on purpose.
4. PHPUnit attributes. #[Group] and #[CoversClass] replace the doc-comment metadata: 182 runner deprecations to zero.
5. Entity fixes (+ tests). addRoles()/removeRoles() save the owner once per call instead of once per role. Both name fields are 255 characters; update 10027 alters the populated columns directly and records the new definition, because the storage schema refuses column changes on a field with data (and, found the hard way, it must store the code definition — a stored definition carries a cached schema, so widening its setting leaves a pending entity update). The plan's show field defaults to FALSE, so a fresh plan validates; CycleEngineTest no longer needs to scope around it.
6. Translatability and DrupalPractice (+ Views render test). Remaining bare t() calls are $this->t(); the Views field plugins return translated labels styled by a small subscription_manager/status CSS library instead of inline colours and untranslated text, with the date formatter and time service injected; the controller and the Drush class load through the entity type manager rather than User::load()/Role::loadMultiple(); unused variables, a useless buildForm() override and two scaffold comments are gone. phpcs (Drupal + DrupalPractice) is clean across the module.
7. Misc. .gitigore → .gitignore; composer source URL and GPL-2.0-or-later; leading slashes on the two user routes; sm-sync-plans replaces the foo alias. The docblock typo from the summary could not be re-found and is dropped.
8. Drush coverage (+ tests). The logic behind cleanup-duplicates, find-duplicates and reconcile moves into two services, DuplicateSubscriptionResolver and SubscriptionReconciler; the command class validates options, formats rows and prints summaries, nothing else. MaintenanceServicesTest (8 cases) covers keeper selection, dry runs, cross- and same-connector detection, priority resolution with remote cancellation (through RemoteCancellation from #3616934: Deleting a user deletes local subscription entities but never cancels the remote contract, so billing continues) including the failure path, and reconciliation including status filters, skipped rows, failures and exceptions. Legacy duplicate data — the thing these commands exist for — is seeded straight into storage, since the single-active rule (#3616773: shouldRedirectCurrentUserOnLogin() only consults the default connector) no longer lets the entity API create it.

Suite: 179 kernel tests, 2492 assertions; the only remaining deprecation is the intentional one from the annotation test module. Not here, as agreed: the portal API (#3627571: Build the membership portal page on capability interfaces, replacing the unused inline-actions API) and the custom_subscribe_url redesign (its own issue).

Change record drafted.