This summary could still use an explanation of the problem/motivation, but for now, here's just a list of what the patch does:

  • Adds a cache.form service/bin and pass it to FormBuilder.
  • Changes $form_state['cache'] into $form_state['persist'] and $form_state['cache_form']. This is documented in FormBuilderInterface, which explains that the form is only cached if $form_state is persisted. In other words, you can persist $form_state without caching $form, but you can't do the reverse. Not sure yet if having a $form_state['cache_form'] variable that is only listened to when $form_state['persist'] is also TRUE, is a WTF or not.
  • Changes $form_state['no_cache'] into $form_state['no_persist']. That's the more important thing to allow this high priority suppression of, since $form_state persistence is tied to functionality. $form_state['cache_form'] is for optimization only (not for functionality), so I don't think needs a dedicated 'no_*' setting. A later running handler can just set 'cache_form' to FALSE if it want to turn off the optimization.
  • Renames FormBuilder::getCache() and FormBuilder::setCache() to loadState() and saveState(), and makes them primarily about $form_state. They also implement $form caching, but whether that succeeds or not is irrelevant to $form_state. As part of making $form_state primary, this patch removes $form['#cache_token'] and adds $form_state['csrf_token'] instead.
  • Removes form_get_cache() and form_set_cache() wrappers, because the rename of above makes them confusing to keep.
  • Changes FormAjaxController to only require $form_state persistence, not $form caching. This enables AJAX forms to work even if the form cache gets cleared or sent to a NULL backend. Therefore, this patch also changes ajax_process_form() to set $form_state['persist'] only, and not $form_state['cache_form'], so that whether or not to cache a given AJAX form is a per-form decision, not something dictated by the AJAX system. For FormAjaxController to work this way, this patch adds $process and $form_build_id optional parameters to FormBuilder::buildForm(). There might be a cleaner way to implement this than tacking on those parameters, but I don't yet have ideas on how.
  • Changes FormBuilder::rebuildForm() to return the built but not yet processed form (doBuildForm() is a confusing name, since it's as much about processing as about building, but renaming that function isn't part of this issue's scope). This resolves a @todo in HEAD about centralizing where setCache() (now, saveState()) is called.
  • Removes the drupal_rebuild_form() wrapper, because per above, the API of the return value of what it wraps is changed. Alternatively, we could make the wrapper invoke doBuildForm() to preserve BC, but this is a very rarely called function, so why leave the BC wrapper?
  • Adds a test to Drupal\system\Tests\Form\StorageTest to ensure multistep forms can work even when the form cache gets cleared between steps (i.e., that only $form_state persistence is required).
  • Changes test forms that were setting $form_state['cache'] to setting both $form_state['persist'] and $form_state['cache_form']. We may want additional tests that set only one but not the other.
  • Changes ViewsForm from setting $form_state['cache'] to setting both $form_state['persist'] and $form_state['cache_form']. There's a code comment about why the form caching is necessary, but I'm concerned about that; it should be possible to put what's required to not change into $form_state, and then allow the form to be rebuilt if it's not in cache.
CommentFileSizeAuthor
#15 form-cache-2183275-15.patch41.16 KBeffulgentsia
#12 form-cache-2183275-12.patch40.31 KBeffulgentsia
#10 form-cache-2183275-10.patch40.15 KBeffulgentsia
#5 form-cache-2183275-5.patch17.51 KBeffulgentsia
#3 form-cache-2183275-3.patch11.35 KBeffulgentsia
#2 form-cache-2183275-2.patch9.54 KBeffulgentsia
form-cache.patch5.71 KBeffulgentsia

Comments

Status: Needs review » Needs work

The last submitted patch, form-cache.patch, failed testing.

effulgentsia’s picture

effulgentsia commented
Status: Needs work » Needs review
StatusFileSize
new form-cache-2183275-2.patch9.54 KB
effulgentsia’s picture

effulgentsia commented
StatusFileSize
new form-cache-2183275-3.patch11.35 KB

#2 will likely pass, but only due to missing test coverage. This adds a test to ensure the $form_state survives even when $form cache is cleared. This test is expected to fail, because FormBuilder still couples the two. Next step is to decouple that.

Status: Needs review » Needs work

The last submitted patch, 3: form-cache-2183275-3.patch, failed testing.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new form-cache-2183275-5.patch17.51 KB
effulgentsia’s picture

effulgentsia commented
Status: Needs work » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I don't really see how this could work? Try this:

1. Apply patch, rebuild.php
2. Visit node/add/article
3. Empty cache_form
4. Try to upload an image.

=> An unrecoverable error occurred. Use of this form has expired. Try reloading the page and submitting again.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Like @Berdir, I'm not able to follow this proposal either.

The form + form_state cache items are very tightly tied to each other.

We purposively moved cache_form outside of the cache system: #512026: Move $form_state storage from cache to new state/key-value system

And some closely related, long-standing bug reports:
#1694574: drupal_process_form() deletes cached form + form_state despite still needed for later POSTs with enabled page caching
#343415: Form cache is not cleared on submit when page cache is activated

effulgentsia’s picture

effulgentsia commented

We purposively moved cache_form outside of the cache system: #512026: Move $form_state storage from cache to new state/key-value system

Nope. That issue purposely moved $form_state to kv. $form went with it, not by design, but because of:

The form + form_state cache items are very tightly tied to each other.

That's what I'm working on untying. CNRs on this issue so far are for bot. When it's ready for human review, I'll unassign myself from this issue, and create a real issue summary. Feedback in the meantime is welcome, just realize it's a work in progress.

Re #7, yep, aware of it, and working on that.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new form-cache-2183275-10.patch40.15 KB

This addresses #7, but is still raw.

Status: Needs review » Needs work

The last submitted patch, 10: form-cache-2183275-10.patch, failed testing.

effulgentsia’s picture

effulgentsia commented
Status: Needs work » Needs review
StatusFileSize
new form-cache-2183275-12.patch40.31 KB

Status: Needs review » Needs work

The last submitted patch, 12: form-cache-2183275-12.patch, failed testing.

effulgentsia’s picture

effulgentsia commented
Issue summary: View changes
effulgentsia’s picture

effulgentsia commented
Assigned: effulgentsia » Unassigned
Status: Needs work » Needs review
StatusFileSize
new form-cache-2183275-15.patch41.16 KB

This fixes the easy fails. FileField, Views, and Batch need more investigation.

I also updated the summary to explain what the patch does, but not yet why. Though maybe for some of the followers of this issue, it's obvious?

I'll post interdiffs from here on out, so if anyone wants to start a architecture or code review, despite a few test failures still, go ahead. I don't think I'll have much time to work on this for the next 2 weeks, so unassigning myself for now, but I'll post new patches with interdiffs if/when I can get to it.

Status: Needs review » Needs work

The last submitted patch, 15: form-cache-2183275-15.patch, failed testing.

effulgentsia’s picture

effulgentsia commented

New thought based on #2208115-15: Add DependencySerializationTrait. What if we stop caching/persisting $form entirely? Once the remaining failures here are resolved, it becomes completely unnecessary. The cache hit ratio on $form is at best 50%, and it's not that expensive to rebuild from $form_state: if computationally/io expensive information is needed (e.g., from a web service call to another site), then that can be stored in $form_state.

Meanwhile, completely removing $form from being serialized gets us out of the dependency serialization craziness we currently do.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

That sounds very sane. I'd really like that!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

That sounds interesting, but frankly, it's a radical change proposal that at least I have to swallow and sleep over first.

I'm not really sure whether the premise of "fast to compute" is really true for the forms that actually need form caching — most forms just inherit it due to #ajax, and speaking of, #ajax might be one of the exact points where the idea breaks down, because #ajax changes the cached $form structure.

effulgentsia’s picture

effulgentsia commented

That sounds interesting, but frankly, it's a radical change proposal that at least I have to swallow and sleep over first.

Of course. I'd like you to keep an open mind at least until the patch is green, so concerns about "it can't possibly work" are removed. But feedback before then is welcome too.

because #ajax changes the cached $form structure

It does, but it should only do so based on a corresponding change to $form_state, so that the new $form could be rebuilt from the new $form_state if needed. There may be some ajax forms that don't do this (including possibly the ones causing the remaining failures), but I consider that to be a bug. If you have experience from Mollom or other contrib projects that provide a solid use case for changing $form in a way not possible to rebuild from $form_state, please share those.

catch’s picture

catch
he/him
Primary language English
commented

Cross-linking #2263569: Bypass form caching by default for forms using #ajax., I haven't caught up with this issue yet but it'd definitely impact that one way or the other.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: +Needs issue summary update

Now that we have Drupal\Core\Form\FormCache, and $form_state is no longer an array, this could use an issue summary update

chx’s picture

chx commented
Status: Needs work » Closed (fixed)

FormCache is using $this->keyValueExpirableFactory->get('form') which is great because the form cache never was a cache to boot. This I conclude done.

effulgentsia’s picture

effulgentsia commented
Priority: Normal » Major
Status: Closed (fixed) » Needs work

because the form cache never was a cache

I opened this issue in order to make it into a true and proper cache, and only putting state information into $form_state and not into $form. I haven't had time to continue this further, but I'd still like to, whether for 8.0 or 8.1. I'm hesitant to postpone it to 8.1 prematurely though, because there are known performance problems from $form storage growing out of control, even with relatively short expiration. Being able to apply a proper cache backend for it (including a NULL one) while still having ajax and multistep forms function properly would be a big win for that. Raising to Major for that.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@effulgentsia has this happened, it doesn't look like 'cache_form' is a bin in D8, maybe it got renamed or something? Has this become moot is there anything salvageable from this?

effulgentsia’s picture

effulgentsia commented
Title: Use cache for $form, kv for $form_state » [PP-1] Use cache for $form, kv for $form_state

Postponing this on #2502785: Remove support for $form_state->setCached() for GET requests. After that, I'll see if there's anything left here worth doing. I still think it would be good in theory to only persist $form_state (and not $form) for multistep forms and state changes that result from validation errors, but I don't know if doing so would bring big enough benefits to bother. The main benefit would be to reduce the storage needs of the kv, especially for large $form's, but with #2502785: Remove support for $form_state->setCached() for GET requests reducing persistence to only POST requests, that benefit isn't as large as when I first opened this issue.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@effulgentsia one concern I'm facing in D7 commerce is form state on cart pages is huge when form state persists the form args and the second arg in a views form is the entire view object. The results tends to load the entire node on a results field called _field_data.

Also it is always multi step.

There is a patch to help form but not form_state with this.

And this drags down cache insert with a large blob as well as to a lesser extent unserialize/serialize.

There is an interesting comment around why form cache is needed in views, I'll see if I can dig it up but to to the extent that the "rows may change and thus the form between requests"

Any ideas where we can reduce/remove that need? Glad to open a follow up if there is a way forward on that.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented

Reading through the issue I am not sure that not caching the form is necessary a good idea.

When using Ajax / multi step forms the main use of the cached form is when the user input is submitted and the form is validated and in this first step the form will be retrieved from the cache instead of rebuilding it.
The second step almost always flags in the Ajax submit the form for a rebuild. As a summary : having form cache means that on Ajax the form is retrieved once from cache and then rebuild and without form caching the form will be rebuild twice on Ajax. Having really big nested entity forms rebuilding the form might be expensive and it would be better doing this only once instead twice. I've measured the performance of FormBuilder::doBuildForm and it currently takes up to 2 seconds for our biggest forms, which means without form caching this will be 2x2 seconds.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented

And also if the validation fails having rebuild the form would've been unnecessary as the previous cached form is being returned to the page but with errors set on the form elements which couldn't pass the validation.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Related issues: +#2769955: KeyValueExpirableFactory incorrectly chooses its storage

FWIW, this issue is related to a bug in the KeyValueExpirable storage code and/or documentation: #2769955: KeyValueExpirableFactory incorrectly chooses its storage.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented

Imho better approach:

In #2263569-61: Bypass form caching by default for forms using #ajax., an imho better approach was sketched:

For POST / AJAX [forms], we can store the form cache in a hidden field encrypted. We can perhaps use JSON instead of PHP serialize, compress it and then encrypt it. The only thing we would store server side is the encryption key, I would say once per session.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: [PP-1] Use cache for $form, kv for $form_state » Use cache for $form, kv for $form_state

Blocking issue was committed (2 weeks after it was made blocking, in July 2015)

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 8.9.x-dev » 9.1.x-dev

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.