\Drupal\Core\Form\ConfigTarget is not fully serializable

Bumping to major since this is an uncaught exception when you use the API as it's intended.

Whoops. So... we need to revert 📌 Allow all callables in ConfigTarget Fixed which is super sad. Because that's just not compatible with caching the built form. I thought we always a had a freshly built form - but we don't - I've misunderstood how \Drupal\Core\Form\FormBuilder::buildForm() works. :(

Or we make use of https://github.com/laravel/serializable-closure

I think the best thing to do here is to revert 📌 Allow all callables in ConfigTarget Fixed in this issue and add the test. And then consider the new dependency and whether or not we can use it here in another issue.

Ugh 😬😞

That is most unfortunate, because the DX improvement that 📌 Allow all callables in ConfigTarget Fixed brought is huge. 😞

One important remark here is that #config_target is only available for ConfigFormBase. And if I search all ConfigFormBase subclasses in core

• for ->setCached( (i.e. \Drupal\Core\Form\FormState::setCached()), then I find zero hits
• for ->setCache( (i.e. \Drupal\Core\Form\FormBuilder::setCache()), then I find zero hits

So … is this truly a problem? 🤔 Do the benefits (being able to use the 2 aforementioned methods) truly outweigh the downsides (worse DX).

We're talking about simple config forms only here. Form cache is IIRC designed for wizards. Simple config not being able to use that is not a big deal IMHO — and honestly, it still could, but then it just wouldn't be able to use #config_target. That seems reasonable.

Is it an option to leave as is for 10.2 - if config target works on a form great - developers can add it. If not, we can add something to the release notes that says only use this on forms that are never cached - which will be a lot of the simple config forms. And then in 10.3 we can add https://github.com/laravel/serializable-closure as a dependency in this issue and fix it for the cached form use-case?

#7 is a desperate grasp.

I think the best thing to do here is to revert 📌 Allow all callables in ConfigTarget Fixed in this issue and add the test. And then consider the new dependency and whether or not we can use it here in another issue.

is obviously the safer approach.

Just pushed the first necessary commit to revert this. It makes @laurii's new test pass. It should make a lot of other tests fail.

… I obviously would very much prefer #8 though 😇

I've just posted an MR to show that using the laravel package works nicely and would allow us a way forward, if we go for #8.

#9's test results: the kernel test passed and many others failed, as predicted: https://git.drupalcode.org/issue/drupal-3408120/-/pipelines/62682

So just pushed commits to implement #8, and I expect that to pass all tests: https://git.drupalcode.org/issue/drupal-3408120/-/pipelines/62688 — it was surprisingly simple 😄

Hah, cross-posted again! 🙃

1. Alex added explicit unit test coverage. He went with only using SerializableClosure when actually serializing the form, which perhaps is more efficient but also more complex.
2. I did not add explicit unit test coverage. I went with always using SerializableClosure, which makes the code simpler to read.

I don't think we should be always serialising the closures - that means we have to do work that's unnecessary.

Adding dependency evaluation to issue summary.

Thought experiment - What if instead of closures we supported subclasses of ConfigTarget and those classes had actual methods?

Adding a third party dependency for this feels heavy

@larowlan sub classes would make me feel sad - composition over inheritance.

However, your question did make me think about about one thing... we might be opening up a new attack vector because people could inject code if they manage to get the serialised content. We'd definitely need to use the SerializableClosure::setSecretKey('secret'); and set it to something based on the private key and hash salt. I guess we can sort this out in the ConfigFormBase constructor and we might have to do it in a wakeUp method too. Certainly adds another level of complexity on top.

Got another idea! I think we should make the form uncacheable if you use a toConfig or fromConfig. That way we know we're okay.

Created an MR based on #19 - would love feedback from someone who knows whether doing the cache disabling in an after_build callback is a good idea. And if in fact this is a good idea at all...

Okay going down the disabling cache route and adding a test that proves the issue and actually finds another problem when AJAXing on forms with #config_target - I think this is the best way to go. It's not adding a dependency and it works well.

Going to hide the other MRs and update the issue summary.

alexpott → changed the visibility of the branch 3408120-add-serializable-closure to hidden.

alexpott → changed the visibility of the branch 3408120-drupalcoreformconfigtarget-is-not to hidden.

Going to bump this to critical as it is not beyond imagination that some contrib module has added ajax to one of the forms we've already added #config_target to.

I like the general direction of the fix, but added some questions/comments.

Complete list of other forms in core calling FormState::disableCache():

• ViewPreviewForm
• ViewsFormBase
• ViewEditForm (but this one has a @todo to remove it … dating back to 2012 though 😅)

👆 This makes me less certain that this is a good idea, because Views has the most complicated forms of all, and maybe that's the only valid justification for disallowing the form to be cached? 🤔

Having stepped through this in detail, I think this is safe given this comment in FormBuilder:

    // After processing the form, the form builder or a #process callback may
    // have called $form_state->setCached() to indicate that the form and form
    // state shall be cached. But the form may only be cached if
    // $form_state->disableCache() is not called. Only cache $form as it was
    // prior to self::doBuildForm(), because self::doBuildForm() must run for
    // each request to accommodate new user input. Rebuilt forms are not cached
    // here, because self::rebuildForm() already takes care of that.
    if (!$form_state->isRebuilding() && $form_state->isCached()) {
      $this->setCache($form['#build_id'], $unprocessed_form, $form_state);
    }

… but despite having spent an hour trying to understand the difference between

  /**
   * Tests #config_target with no callbacks.
   */
  public function testTree(): void {
…
    $assert_session->pageTextNotContains('Option 3');
    $page->selectFieldOption('test1', 'Option 2');
    $assert_session->waitForText('Option 3');
    $assert_session->pageTextContains('Option 3');
    // The ajax request should result in the form being cached.
    $this->assertCount(1, $key_value_expirable->getAll());
…

versus

  /**
   * Tests #config_target with callbacks.
   */
  public function testNested(): void {
…
    $assert_session->pageTextNotContains('Option 3');
    $page->selectFieldOption('test1', 'Option 2');
    $assert_session->waitForText('Option 3');
    $assert_session->pageTextContains('Option 3');
    $this->assertCount(0, $key_value_expirable->getAll());
…

… I still don't understand neither the test nor the behavior. 😅🙈 Both test cases trigger the same AJAX callback, and assert the same resulting form. But one somehow triggers form caching and the other does not. Even though the "nested" form literally extends the "tree" form. What am I missing? 😬

I've addressed @longwave's feedback. We now disable the form cache only when closures are used. This would allow a form alter to replace a closure with another type of callback and have the form cached if they really really really wanted to.

In answer to #27 - I've added https://git.drupalcode.org/project/drupal/-/merge_requests/5787/diffs?co... - whether or the not the form gets cached is the whole point of this change. If you comment out $form_state->disableCache(); in ConfigFormBase and run the JS test you will see it fail - because you can't serialise closures.

No further feedback. This seems to be the least intrusive option to solve this problem.

Yes, https://git.drupalcode.org/project/drupal/-/merge_requests/5787/diffs?co... helps enormously — it's painfully obvious now 😄🙈

(I did do but somehow it didn't connect the dots for me. Sorry about that!)

This looks excellent now. I'd prefer it to get explicitly +1'd by a Form API maintainer (currently: @tim.plunkett and @effulgentsia), but given that it's so very narrow now (only if #config_target is using a closure), I now do feel comfortable RTBC'ing this.

tim.plunkett → credited effulgentsia → .

@effulgentsia and I discussed this just now.
We're fine with committing this, but think that this needs it's own CR pointing out that if you choose to use closures in your config targets, then you will lose the ability to have any sufficiently complex AJAX handling on that form (anything that approaches the complexity of a multi-step form).

This is mitigatable from two directions:

1. Simplify your config forms to not be multi-step
2. Rewrite your closure as a static method

I don't know of any forms that would need the advice of #1.
And taking the example of SiteInformationForm, there is no reason that fromConfig: fn($value) => $value ?: ini_get('sendmail_from'), couldn't be a static method on the form...

Consider this a +1 from the Form/AJAX system maintainers, and if you want to relive some of the digging I did on this, enjoy reading all the child issues of #635552: [meta issue] Major Form API/Field API problems →

I've added this to the existing CR https://www.drupal.org/node/3373502 → because that's where it is going to be most useful.

I still think we could open a follow-up to use the laravel dependency and completely mitigate this issue but that's possible in 10.3.x while this all we can do for 10.2.x.

This is a tricky one, but I like the absolute minimum scope of the change here and it's a clever fix.

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

Opened the followup to serialise the closure using the laravel dependency - see 📌 Allow closures in #config_target's to be serialised Needs review

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