Problem/Motivation

In the code, there is a line that reads $form[‘#cache’] = [‘max-age’ => 0]; here: https://git.drupalcode.org/project/tfa/-/blob/8.x-1.x/src/Form/TfaLoginF...
This TfaLoginForm replaces the core login form.

I'm not sure I understand why the cache max-age is set to 0. This was introduced here: https://www.drupal.org/project/tfa/issues/2923786#comment-12464361 without much further explanation.

Unless I'm mistaken, what happens in buildForm does not require preventing caching, and since submission is handled via a POST request, forms aren't cached in this case.

Is there a specific reason I missed?

Steps to reproduce

None.

Proposed resolution

None.

Remaining tasks

Discuss this topic and determine if the cache max age can be inherited from the parent core UserLoginForm.

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

arousseau created an issue.

cmlara’s picture

cmlara
he/him
commented

I'm not sure I understand why the cache max-age is set to 0. This was introduced here:

It is actually a few comments earlier in https://www.drupal.org/project/tfa/issues/2923786#comment-12411743. There are a few other places that likely do make sense for the cache to be set to 0.

Unless I'm mistaken, what happens in buildForm does not require preventing caching, and since submission is handled via a POST request, forms aren't cached in this case.

I've seen it, and had a similar thought a time or two, however partially due to of how fragile the 8.x-1.x branch is, and partially due to cores Internal Page Cache (IPC) being enabled by default (which ignores max-age) it hasn't been a significant priority to try and remove it.

The pages (as form pages) already have an immediate expire and are Cache-Control: must-revalidate, no-cache, private to clients. The only location being impacted currently is Core's Dynamic Page Cache (DPC) being X-Drupal-Dynamic-Cache: UNCACHEABLE (poor cacheability) which if the IPC always serves the cached copy to anonymous users (while logged in users shouldn't be able to visit the login page) its a non-issue.

The IPC likely means that it provides no particular benefit, however equally "can't be a security fault in TFA for caching if we tell core never cache" certainly plays a role on why investigating it hasn't been a priority especially when would need to trace it back multiple core majors to be sure there wasn't some oddity in cache handling a tone time.