[PP-1] Determine how to implement Form Alters with attributes.

Created on 7 October 2024, 3 months ago

Problem/Motivation

There are currently three form alter hooks:
hook_form_alter
hook_form_FORM_ID_alter
hook_form_BASE_FORM_ID_alter

We need to consider dev experience for implementing these with the new hook attribute system.

Two main suggestions:
Move all to FormAlter, handle complexity of determining which to implement and how to handle the developer choosing.

Use three Attributes one for one.
Not final names:
FormAlter
FormAlterById
BaseFormAlter

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

base system

Created by

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Remember we can use named arguments now so to me a single #[FormAlter] attribute is easier.

    Alter all forms:

    #[FormAlter]
    

    Alter one form:

    #[FormAlter('node_form')]
    

    Alter all forms, specify method:

    #[FormAlter(method: 'alterForm')]
    

    I don't see the method argument being used widely anyway when we can just apply the attribute to the methods themselves?

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I forgot about named arguments, that makes it much cleaner, you can even use it to help with targeting base form vs form id so we can help identify it for documentation!

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    I'm not even sure we need to tell the difference between base form IDs and form IDs, we don't do that at the moment with hooks except in the docblock - I'm not sure what it would gain us.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    The order they are called from comes from the name. #Hook has to pass the hook name still so that is preserved. We probably want to preserve that for any child Attribute too.

    It's also useful for documentation to know which hook is being called.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡¨πŸ‡¦Canada Charlie ChX Negyesi 🍁Canada
  • πŸ‡¨πŸ‡¦Canada Charlie ChX Negyesi 🍁Canada
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    A bit confused about the order discussion. hook_form_BASE_FORM_ID_alter() and hook_form_FORM_ID_alter() has the same structure, there is no explicit order in hook definition. It's the same pattern, it's purely documentation thing. The order is defined by the hook invocation order: the form system will first call the hook for the base form id and then form_id, but your hook function might match both if there are conflicting form ids and base ids:

    There is no "base" in this alter definition:

      /**
       * Implements hook_form_BASE_FORM_ID_alter().
       */
      #[Hook('form_node_form_alter')]
      public function formNodeFormAlter(&$form, FormStateInterface $form_state, $form_id) : void {
    

    So, having both FormAlterById and BaseFormAlter would purely be a documentation thing about what your intention is, but the resulting hook string would be the same. looking at the example above, given that we have the intention to remove the requirement to have a Implements hook_..() docblock that might not be a bad thing to have that annotated somehow, but fairly minor difference.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    I was convinced I was missing something in #5/#6, so thanks @berdir because I was thinking exactly along those lines.

    your hook function might match both if there are conflicting form ids and base ids

    This was a problem before, and it's still a problem now, but I don't think that is for this issue to solve.

    For me I think we should just implement

    #[FormAlter] === hook_form_alter
    #[FormAlter('node_delete_confirm')] === hook_FORM_ID_alter or hook_BASE_FORM_ID_alter

    Maybe we could have #[BaseFormAlter(...)] to identify base forms, and perhaps that will help us solve name collisions in the future, but at least to start with I think they should be treated identically - at least until we remove procedural hooks, because there is no way of identifying (except via an optional comment) which is a base form and which is not.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    > This was a problem before, and it's still a problem now, but I don't think that is for this issue to solve.

    Certainly, I just mentioned that to point out how similar those two hooks really are.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I think that simplifies this.

    We add implements still.
    Use FormAlter with an optional form id if that is provided we specify hffia.

    Some future date we either add type, add a base form attribute, or deprecate base form and just leave it ambiguous.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    When I started working on this I realized we can't have separate IMPLEMENTS if we have only one child attribute.

    @ghost helpfully reminded me that this is also about api finding the form alters.

    He suggested we add a PREFIX and SUFFIX const we can set to form and alter then pass the id in conditionally.

    I'll push that up then update the IS.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Merge request !10295Resolve #3479141 "Determine how to" β†’ (Open) created by nicxvan
  • Pipeline finished with Canceled
    30 days ago
    Total: 78s
    #347126
  • Pipeline finished with Canceled
    30 days ago
    Total: 83s
    #347129
  • Pipeline finished with Failed
    30 days ago
    Total: 122s
    #347132
  • Pipeline finished with Failed
    30 days ago
    Total: 557s
    #347137
  • Pipeline finished with Failed
    30 days ago
    Total: 615s
    #347319
  • Pipeline finished with Success
    29 days ago
    Total: 1079s
    #347335
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    This is ready for review, I think this covers all bases and will likely work for api to pick up implementations.

    I think @ghost will review the api side.

Production build 0.71.5 2024