Contrib.social

Deprecate not giving cacheability metadata to forms

Open on Drupal.org →
Created on 19 October 2023, about 1 year ago
Updated 31 October 2023, about 1 year ago

Problem/Motivation

Most forms use CSRF tokens for authenticated users. Because of this, rendered authenticated forms could not be cached.
But since #2463567: Push CSRF tokens for forms to placeholders + #lazy_builder → , form tokens are now rendered lazily. So that many forms could be cached if they provided the right cacheability metadata. At least the CSRF token is not a blocker anymore.

Some forms already provide cacheability metadata, some don't. For the ones who do, we aren't sure they provide the right metadata as actually at the end of the day they were still not cached. Form cacheability metadata was not used so it's not tested.

Because of this uncertainty, for backward compatibility, we still default to uncacheable forms but we allow forms to opt-in individually for cacheability by providing their own non-zero $form['#cache']['max-age] ( 📌 Form tokens are now rendered lazily, allow forms to opt in to be cacheable Needs review ).

The aim of this issue is to make developers give correct cacheability metadata to their forms through a deprecation process. So that, at next major core version, no more forms have missing/wrong cacheability, and we can finally default to caching forms.

Proposed resolution

The idea that has emerged and been discussed in comments #110, #122 and #126 of 📌 Form tokens are now rendered lazily, allow forms to opt in to be cacheable Needs review is the following: make FormInterface implement CacheableDependencyInterface. Thus, all forms will have to declare their cacheable metadata.

Remaining tasks

  • Make FormInterface implement CacheableDependencyInterface
  • Where FormInterface::buildForm() is "consumed", inject the cacheability metadata from CacheableDependencyInterface methods into the render array (I guess we can have a look at how it's done for blocks).
  • For backward compatibility, make a default implementation of CacheableDependencyInterface methods in FormBase, which still sets the max-age to zero. According to #2578855#126, it's enough to do this to ensure BC: we can assume all classes implementing FormInterface extend FormBase.
  • Remove the previous BC system from 📌 Form tokens are now rendered lazily, allow forms to opt in to be cacheable Needs review which sets the max-age to zero (we now do this in FormBase).
  • Find the right way to tell that it's deprecated to rely on FormBase's CacheableDependencyInterface implementation. The deprecation must be as discoverable as possible to ease upgrades.
  • Make a change record

User interface changes

None.

API changes

FormInterface will implement CacheableDependencyInterface.

Data model changes

None.

Release notes snippet

To be done.

📌 Task
Status

Active

Version

11.0 🔥

Component
Form  →

Last updated 2 days ago

Created by

🇫🇷France GaëlG Lille, France

Live updates comments and jobs are added and updated live.
  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

  • Needs change record

    A change record needs to be drafted before an issue is committed. Note: Change records used to be called change notifications.

Sign in to follow issues

Comments & Activities

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024