Problem/Motivation

SubscribeMenuLink::getTitle() returns "Upgrade" when the current user has a subscription and "Join" otherwise, but getCacheContexts() declares only user.roles. Subscription ownership is per-user, not per-role-combination: two authenticated users with identical roles (common before any subscription-granted role, i.e. exactly the population deciding whether to join) share a render-cached menu entry, so whichever renders first fixes the label for the other. A subscriber can see "Join", or a non-subscriber "Upgrade".

Steps to reproduce

  1. Two users with the same roles, one subscribed (whose plan grants no role, or before role grant), on a site with render-cached menus.
  2. Load pages as each; both see the label produced for whoever primed the cache.

Proposed resolution

Declare the user cache context, and add the subscription_list cache tag via getCacheTags() so the label updates when subscriptions change (the module already invalidates that tag on subscription insert/update/delete). While in the file, wrap the two titles in $this->t() and have SubscriptionManagerController::subscribeTitle() reuse the same strings, fixing their missing translatability in one place.

Remaining tasks

Patch; extend SubscribeMenuLinkTitleTest to assert contexts/tags.

User interface changes

Correct per-user Join/Upgrade labels; strings become translatable.

API changes

New SubscriptionManagerService::subscribeLabel(AccountInterface): TranslatableMarkup. SubscribeMenuLink's constructor takes the current user and the subscription manager (autowired). core_version_requirement raised from ^10 || ^11 to ^11.3.

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

colan’s picture

Issue summary: View changes
Status: Active » Needs review

Fixed in the MR, in two commits, with one scope addition and one policy change worth flagging.

The menu link. SubscribeMenuLink now declares the user cache context (not user.roles) and the subscription_list cache tag, so a user's own label also flips when they subscribe. The cache_contexts key in links.menu.yml is removed: MenuLinkBase never read it, so it only documented an intention. The label strings move to a new SubscriptionManagerService::subscribeLabel(AccountInterface $account): TranslatableMarkup, which both getTitle() and the subscribe page's _title_callback use, so they cannot disagree.

Same bug, one page over. The subscribe page itself had the same class of problem: subscribe() redirects subscribers to their connector but renders the connector chooser for everyone else, and the chooser's render array declared no cache metadata while the route has no no_cache. Dynamic Page Cache would therefore have served one user's chooser page (title "Join" included) to a subscriber with the same permissions who should have been redirected. The chooser build now declares the user context and the subscription_list, subscription_plan_list and config:subscription_manager.settings tags — who sees it depends on their own subscription, what it lists on the visible plans and the chooser settings.

Autowiring, and Drupal 10. SubscribeMenuLink was pulling its dependencies through \Drupal:: calls. Since core 11.3 PluginBase::create() autowires plugin constructors (and MenuLinkDefault no longer overrides it), so the plugin now just declares a constructor with the current user and the subscription manager (the latter via #[Autowire(service: 'subscription_manager')], as the service has no interface alias). On Drupal 10 that constructor would fatal, because MenuLinkDefault::create() there still passes exactly four arguments. Rather than keep a hand-written factory for a core version that reaches end of life in about two months, alongside Drupal 12's release, core_version_requirement becomes ^11.3. Drupal 10 was nominal support anyway — nothing in this roadmap ever ran the module on it, and the CI tests only the current major. The #[LegacyHook] shims that existed for 10 can go in the cleanup omnibus (#3616937: Cleanup omnibus: dead config keys, missing token dependency, per-role user saves, page_attachments overhead, drush alias, field length, translatability, misc), along with autowiring the remaining plugins. Drupal 12 support is a separate roadmap step; 1.0.0 is realistically a Drupal 12-era release.

Tests. SubscribeMenuLinkTitleTest now builds the plugin through create() as the menu system does, and gains two cases: two users with identical roles get different labels from one link instance, which declares user (and not user.roles) and subscription_list; and the labels are TranslatableMarkup, with the page title callback agreeing with the menu link in both states. ConnectorChooserTest asserts the chooser build's contexts and tags. Suite at 164 kernel tests.

Change record drafted (the core requirement change is the user-visible part).

  • colan committed 27cbd9c6 on 1.0.x
    Merge branch '3616936-subscribe-menu-link-cache' into '1.0.x'
    
    Resolve #...

  • colan committed 20b2bc00 on 1.0.x
    Issue #3616936: Count the query budget by statement shape, not table...

  • colan committed a3df1c0c on 1.0.x
    Issue #3616936: Drop readonly from the autowired menu-link properties...

  • colan committed 2e80aa5b on 1.0.x
    Issue #3616936: Cover the per-user cache metadata and shared labels
    
    Two...

  • colan committed 1c17d7a5 on 1.0.x
    Issue #3616936: Vary the Join/Upgrade label per user and require core 11...
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.