Contrib.social

Make it possible to opt out of form_alter with form_id when running alter for a form

Open on Drupal.org →
Created on 15 April 2024, 2 months ago

Problem/Motivation

When a form has a lot of variations, storing hook implementations for it might really fill up the cache.

For example, on this site this was a problem for. We use Drupal Commerce. Drupal commerce has a separate form id per variation of the add to cart form. Which is kind of useful in a way, but it ends up creating its own alter hooks for commerce_order_item_add_to_cart_form_commerce_product_1 and commerce_order_item_add_to_cart_form_commerce_product_2 and so on. When this site has more than 20M products, that's a lot of potential alter hooks. And Drupal will cache all of these alter hooks in the cache, along with the implementors of it. Even if it's empty. So if we have 20M forms with different variations of commerce_order_item_add_to_cart_form_commerce_product_xx it all adds up even if its a compressed, serialized empty array.

The impact of not storing empty arrays would probably be rather large, but one less disruptive, and totally backwards compatible change would be to add a flag to indicate if we want to skip altering the form for the form id specifically. It's still possible to alter these forms by base_form_id or simply with form_alter too. And as it would be opt_in, it should be totally safe and backwards compatible.

Steps to reproduce

Let's create a form that has a lot of variations with its form id. With time. So let's create this (snippet of form):

/**
 * Provides a mymodule form.
 */
final class ExampleForm extends ExampleBase {

  /**
   * {@inheritdoc}
   */
  public function getFormId(): string {
    return 'mymodule_example_form' . time();
  }
}

I then refresh the page where the form is a couple of times, and then I inspect the cache bootstrap cache bucket with the module_implements key:

$items = \Drupal::service('cache.bootstrap')->get('module_implements')->data;
$filtered = array_filter($items, fn($k) => str_starts_with($k, 'form_mymodule'), ARRAY_FILTER_USE_KEY);
var_dump($filtered);

It produces this:

array(5) {
  'form_mymodule_example_alter' =>
  array(0) {
  }
  'form_mymodule_example_base_alter' =>
  array(0) {
  }
  'form_mymodule_example_form1713173540_alter' =>
  array(0) {
  }
  'form_mymodule_example_form1713173541_alter' =>
  array(0) {
  }
  'form_mymodule_example_form1713173542_alter' =>
  array(0) {
  }
}

As we can understand. This cache key will now continue to grow indefinitely. Which I wish there was a clean way of ensuring it would not do. Let's assume it's important to me to actually have all of these variations for form_id. Then it would be no way for me to (at least cleanly) tell Drupal to not try to alter with that hook, and not cache the list of implementers of that hook.

Proposed resolution

Make it possible to set a property in the form to opt out of altering based on form_id.

Remaining tasks

agree on approach
create MR

User interface changes

none

API changes

Are form keys considered API?

Data model changes

none

Release notes snippet

✨ Feature request
Status

Needs work

Version

11.0 🔥

Component
Form  →

Last updated about 6 hours ago

Created by

🇳🇴Norway eiriksm Norway

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

  • Needs subsystem maintainer review

    It is used to alert the maintainer(s) of a particular core subsystem that an issue significantly impacts their subsystem, and their signoff is needed (see the governance policy draft for more information). Also, if you use this tag, make sure the issue component is set to the correct subsystem. If an issue significantly impacts more than one subsystem, use needs framework manager review instead.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @eiriksm
  • Comment 2 months ago →
    🇳🇴Norway eiriksm Norway
  • Comment 2 months ago →
    🇳🇴Norway eiriksm Norway
  • Comment 2 months ago →
    🇳🇴Norway eiriksm Norway

    eiriksm → changed the visibility of the branch 3441006-make-it-possible to hidden.

  • Status changed to Needs review 2 months ago
  • Comment 2 months ago →
    🇳🇴Norway eiriksm Norway

    Pushed a very naïve implementation of it with no tests. Since there is no "API" to sort of manipulate from a form class to communicate to the form builder, I went for using the build info. Which you can control inside of a buildForm method like so, for example:

        $build_info = $form_state->getBuildInfo();
    $build_info[FormBuilder::SKIP_ALTER_FORM_ID] = TRUE;
    $form_state->setBuildInfo($build_info);

    Would obviously need some tests, but setting to NR to get some feedback

  • Merge request !7500First take → (Open) created by eiriksm
  • Comment 2 months ago →
    🇳🇴Norway eiriksm Norway

    I actually changed my mind, and added it to the form array instead, since I realized this would actually make it possible to alter base forms from custom code, that you would not want to alter the form_ids for, which can be convenient :)

  • Comment 2 months ago →
    🇳🇴Norway eiriksm Norway

    If someone lands here wanting a solution, one way is to decorate the module handler with your own one, extending core.

    Then, either via hardcoding it like this, or by somehow dynamically (hook_hook_info for example) indicate which implementations should be simply "discarded":

    protected function getImplementationInfo($hook) {
    // Harcoded example.
    $form_id_prefix = 'form_commerce_order_item_add_to_cart_form_commerce_product_';
    if (strpos($hook, $form_id_prefix) !== FALSE) {
      return [];
    }
    return parent::getImplementationInfo($hook);
  }
  • Pipeline finished with Canceled
    2 months ago
    Total: 3459s
    #146971
  • Status changed to Needs work 2 months ago
  • Comment 2 months ago →
    🇺🇸United States smustgrave

    Can we add test coverage for this please.

  • Pipeline finished with Success
    2 months ago
    Total: 1344s
    #146985
  • Comment 2 months ago →
    🇳🇴Norway eiriksm Norway

    Of course.

    At this point mostly interested if this makes sense to other people than me, but if I should read something into your comment I would say it currently does not seems like the opposite at least

  • Comment 2 months ago →
    🇺🇸United States smustgrave

    Maybe sub-maintainer can speak on it. Not sure I can help make the call.

contrib.social Blog FAQ Discussions
Production build 0.69.0 2024