\Drupal\webform\WebformEntityViewBuilder::view() force-disables the render cache by setting max-age to 0.

This class was there since the very first commit of the 8.x-5.x branch.

I don't think it's necessary, if there were caching problems then someone would have reported them when using the form directly and I can't see a reason why those two cases should be kept separate.

I guess we can start with removing it and seeing if any tests break :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Status: Active » Needs review
FileSize
655 bytes

Among other things, this actually disables the render cache of any entity this is embedded into as it bubbles up all the way up to the node and then dynamic page cache. (It's worth pointing out that forms in core actually do the same right now for authenticated users, so this will only make a difference for anonymous users).

Berdir’s picture

It's also interesting that \Drupal\webform\Plugin\Field\FieldFormatter\WebformEntityReferenceEntityFormatter::viewElements() calls $entity->getSubmissionForm() directly, so skips the view builder, which means this is *only* used for standard entity reference fields (Which we actually use in a paragraph type as the additional settings the webform field exposes isn't something we want to have there).

jrockowitz’s picture

@Berdir Caching in D8 is tricky. I absolutely appreciate you help.

I can confirm that the max-age is set to 0 for authenticated users via \Drupal\Core\Form\FormBuilder::prepareForm

As you noted, my only concern would be the caching of a webform for an anonymous user when the webform is displayed in a node and block.

I did some manual testing and webform nodes and blocks are working as expected for anonymous users.

Berdir’s picture

What field did you use for testing, a normal entity reference field or a webform field? As mentioned, the webform field formatter actually doesn't even use this and therefore doesn't have caching disabled.

Considering that, it seems pretty pretty safe to change?

For consistency, we should then also remove \Drupal\webform\Plugin\Block\WebformBlock::getCacheMaxAge().

jrockowitz’s picture

We should also remove the max-age=0 from \Drupal\webform\Element\Webform

I will write out a full (manual) test script to help to check that caching is working correctly for webform nodes and blocks.

jrockowitz’s picture

Attached is a patch with more caching disabled.

Things to check

  • Webform page
  • Webform node
  • Webform block
  • Webform scheduled to open and close
  • Webform prepopulate using tokens
  • Webform prepopulate using querystring parameters

Notes

  • Page cache should be enabled
  • Anonymous and authenticated users need to be tested
  • Might be able to move WebformEntityReferenceFormatterBase::setCacheContext to \Drupal\webform\WebformSubmissionForm
Berdir’s picture

> Page cache should be enabled

max-age does not affect the anonymous page cache: #2352009: [pp-3] Bubbling of elements' max-age to the page's headers and the page cache. So you will not be able to see a difference with or without that actually.

The only thing you can see to confirm that it is working would be that you can see cache entries for the affected block/node in the cache_render table for anonymous users (but not for authenticated ones, because the form itself is still not cacheable until #2578855: Form tokens are now rendered lazily, allow forms to opt in to be cacheable is fixed).

One possible option for that would be to make the form a lazy-builder component, which should actually already happen for blocks. I did that for the poll module because that is highly-personalized based on user/cookie. The result would be that the rest of the rendered node would still be cached and just the form would be then inserted later. With big pipe, you should be able to visually see that the form is inserted after the initial page load then. See \Drupal\poll\PollViewBuilder::view().

But I think this can be done in a separate issue.

jrockowitz’s picture

I am not sure having Big Pipe lazy load a webform is the best UX because generally the webform is the main piece of content on a page. I do agree any experimentation with lazy loading can be handled in a new ticket.

I need to do another review and round of manual testing and then I will commit this patch.

jrockowitz’s picture

Re-rolling the patch.

jrockowitz’s picture

Manual Testing Steps…

Notes

  • The random number will be cached for anonymous URLs.
  • The (anonymous) page cache module caches webforms.

Webform Block

  • Place the attached webform as webform block on every page (/admin/structure/block/list/bartik)
  • Confirm that the URL and random number changes as you navigate to different pages.
  • Confirm that random number only changes if the Query changes

Webform Node

  • Create a Webform Node for the attached webform.
  • Confirm that random number only changes if the URL / Query changes

  • jrockowitz committed 6270cd3 on 8.x-5.x
    Issue #2975902 by jrockowitz, Berdir: Allow caching of webforms...
jrockowitz’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.