Contrib.social

ScheduledTransitionForm doesn't include the scheduled_transitions_for cache tag when empty

Open on Drupal.org →
Created on 13 February 2025, 3 months ago

Problem/Motivation

In Drupal 11, as of 📌 Form tokens are now rendered lazily, allow forms to opt in to be cacheable Needs review forms are now render cacheable. This applies to ScheduledTransitionForm, because we aren't calling parent::form, which would make it uncacheable as an entity form (see https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...)

Most of the time this bug would not be encountered because the ScheduledTransitionsLocalTask adds this tag. However, on our project the local tasks were uncacheable themselves, so this tag didn't bubble to the page.

This bug also isn't present if some ScheduledTransitions already exist, because (I believe) the tag is added via the entity operations or some other cacheability being added when an ST is rendered.

Steps to reproduce

This should be reproducible in a test by visiting the ST form without the local tasks block present.

Proposed resolution

Always add the cache tag to the form, it should not depend on STs being rendered or the local task block.

Remaining tasks

Failing test
Fix
Review
Commit

🐛 Bug report
Status

Active

Version

2.8

Component

Code

Created by

🇦🇺Australia acbramley

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @acbramley
  • Comment 3 months ago →
    🇦🇺Australia acbramley

    I can't reproduce this in a test :( it looks like something funky is happening on our project where DynamicPageCache is caching the empty table on initial cache clear. I need to dig further to see why because the DenyAdminRoutes response policy should be blocking this (which it is in tests).

  • Comment 3 months ago →
    🇦🇺Australia acbramley

    Tracked this down to some custom code setting the _admin_route option on a cloned object in a breadcrumb builder, this affected downstream code.

    I still wonder if we should add the cache tag anyway for completeness?

  • Comment 3 months ago →
    🇦🇺Australia dpi Perth, Australia

    Sounds like you're on it. Yeah lets add tags.

    Is form caching on by default now, easily testable nowadays?

  • Merge request !55Issue #3506589: Add cache tag → (Open) created by acbramley
  • Pipeline finished with Failed
    3 months ago
    Total: 271s
    #423712
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024