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

Created on 7 October 2024, 6 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
  • Pipeline finished with Canceled
    4 months ago
    Total: 78s
    #347126
  • Pipeline finished with Canceled
    4 months ago
    Total: 83s
    #347129
  • Pipeline finished with Failed
    4 months ago
    Total: 122s
    #347132
  • Pipeline finished with Failed
    4 months ago
    Total: 557s
    #347137
  • Pipeline finished with Failed
    4 months ago
    Total: 615s
    #347319
  • Pipeline finished with Success
    4 months 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.

  • Status changed to Needs work 3 months ago
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    I added some more comments to the MR, would like to get this in so we can start adding other attribute classes. It's unfortunate enough that they aren't in 11.1, because it means contrib can't really depend on them, as the BC system doesn't work for them (we have legacyhooks, but if you use FormAlter instead of Hook then your module is broken in 11.1, which is awkward).

    I will move the entity discussion to a separate issue.

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

    because it means contrib can't really depend on them, as the BC system doesn't work for them (we have legacyhooks, but if you use FormAlter

    Do you mean for 11.1.0?

  • Pipeline finished with Failed
    3 months ago
    Total: 704s
    #379298
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Addressed your feedback!

  • Pipeline finished with Success
    3 months ago
    Total: 7129s
    #379302
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    @nicxvan:

    > Yes, we haven't even addressed the main question around form alter though. Should we validate that first?

    I'm not sure I know what the main question is? The general structure with form id and base form id? IMHO we agreed on that in #11-14.

    > Do you mean for 11.1.0?
    Yes, that's what I Mean. I don't see how we could add this in a BC compatible way, so modules will only be able to use specific hook classes once the require the core version and until, need to stick to using Hook classes.

    adding needs change record tag to add one that explains the prefix/suffix concept and how to add hook classes and the specific ones that were added here. Maybe we can do an aggregated change record that similar to plugin conversions that lists which minor version added which hook attribute class?

    Do we also create a follow-up to update all form alters to use the new ones, or will we update as we go? Not sure it's worth going through them all just because.

    I also created πŸ“Œ Implement Entity hook attribute Active

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

    For the CR

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    The CR says it is from 11.2+, but I think the problem explained in #24 deserves an explicit mention that this attribute class can only be used if a module requires 11.2 since LegacyHook doesn't work for this.

    This doesn't only introduce the specific Formalter attribute but also the concept of hook attribute classes for dynamic hooks? I'd suggest we add a second CR that focuses on how to define your own Hook attribute classes, specifically when it contains dynamic parts such as form ids or entity types.

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

    Oh sorry about that!

    Yes, that's what I Mean. I don't see how we could add this in a BC compatible way, so modules will only be able to use specific hook classes once the require the core version and until, need to stick to using Hook classes.

    Unless they get backported, but yeah eventually that will become a problem.

    adding needs change record tag to add one that explains the prefix/suffix concept and how to add hook classes and the specific ones that were added here. Maybe we can do an aggregated change record that similar to plugin conversions that lists which minor version added which hook attribute class?

    Missed that one, we can also just attach more issues as we go.

    Do we also create a follow-up to update all form alters to use the new ones, or will we update as we go? Not sure it's worth going through them all just because.

    Either I think.

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

    Applied your suggestions and added that CR.

  • Pipeline finished with Success
    3 months ago
    Total: 503s
    #396028
  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    Seems the Change Record is now lin place.

  • Pipeline finished with Success
    2 months ago
    Total: 902s
    #402952
  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    Rebased.

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

    nicxvan β†’ changed the visibility of the branch 11.x to hidden.

  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    Added a few minor edits to the CR. See the revision log for details.

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

    I am going to revert those CR changes, that CR is explicitly about the PREFIX and SUFFIX constants, a themer might consume the attribute, but only a module would create a or implement one.

    Also I'm not sure the note about it still being a hook makes sense here since it's a constant on the Hook Attribute.

    I'll see if I can clarify what this CR is for.

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

    Had 1 question in slack around #[Alter but @nicxvan cleared it up and added a new CR.

    Looking at the MR seems all threads have been addressed, suppose after this lands some follow ups could be opened to convert others. May be good first time contribution items for DrupalCon coming up!

    Believe this one is good.

  • πŸ‡¨πŸ‡¦Canada Charlie ChX Negyesi 🍁Canada

    Please do not force api module to chase an entire chain of ancestors, always define both constants.

  • Pipeline finished with Success
    about 2 months ago
    Total: 410s
    #424273
  • πŸ‡¨πŸ‡¦Canada Charlie ChX Negyesi 🍁Canada

    Thanks that's much better.

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

    > Please do not force api module to chase an entire chain of ancestors, always define both constants.

    I don't know how complex that would be, didn't look at the code. But either we enforce that or we'll need to support it (or live with hook documentation/discovery in api.module not working). People will extend hook classes and expect that to work because it works for actual discovery.

    If that really is a considerable complexity and we want to enforce it, we need to make those classes final. Which I very well know how much you dislike that. And then Alter maybe shouldn't exist in the first place, my suggestion for adding that (here in this issue specifically) was exactly so it can be a base class.

    It's still not clear to me if the expectation is that eventually all hooks should have a custom Hook attribute class (and we use it to document said hook) or just common/frequently used ones.

  • πŸ‡³πŸ‡ΏNew Zealand quietone
  • πŸ‡ΊπŸ‡ΈUnited States dww

    Quick driveby review. A few pedantic nits for now. Haven’t finished, but this is all I could do from my phone while eating lunch with the other hand. πŸ˜‚

  • Pipeline finished with Failed
    about 1 month ago
    Total: 472s
    #432238
  • Status changed to Needs review about 1 month ago
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Is that answer acceptable?

  • The Needs Review Queue Bot β†’ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

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

    Rebased

  • Pipeline finished with Success
    27 days ago
    Total: 1117s
    #441887
  • πŸ‡ΊπŸ‡ΈUnited States dww

    Thanks! Overall, this looks really good and clean. But (as usual) I've got some concerns and nits on the MR. πŸ˜…

    Initial pass at saving credits.

  • Pipeline finished with Canceled
    20 days ago
    Total: 1457s
    #448036
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Ok, huge WTF, I injected comment.manager and getFieldUIPageTitle does not exist, did a quick search, it was removed 10 years ago.
    #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form β†’

    Can we safely just delete these? These alters can't have done anything in years.

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

    I created an issue to track that and removed the change from this issue: πŸ› Determine what to do about comment module form alters calling getFieldUIPageTitle Active
    I converted contact form alters instead so we can validate.

  • Pipeline finished with Success
    19 days ago
    Total: 1187s
    #448391
  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

  • Pipeline finished with Success
    14 days ago
    Total: 1093s
    #452270
  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    Fix bot line length warnings

  • NR bot hopefully appeased.

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

    Humans are smarter than bots. πŸ˜‚ I still have questions and concerns. But we're really close now.

    Thanks!
    -Derek

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

    Looks like we need a few follow-ups here:

    • Something to document how to put form submit callbacks into Hook classes
    • Fixing Hook to use NULL for both of its optional parameters, instead of '' for $method vs NULL for $module

    Otherwise, I think this is RTBC.

  • Pipeline finished with Success
    14 days ago
    Total: 3274s
    #452513
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Created the follow ups, I think it's safe to RTBC based on that.

  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

  • Is it weird that the bot is picking up PHPCS issues that the build is not?

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

    > Is it weird that the bot is picking up PHPCS issues that the build is not?

    No, this is just a very new rule, was not enforced before.

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

    Thanks for opening those followups. Resolved my open threads. I'll let @nicxvan apply the phpcs suggestion, then it's RTBC to me.

    Thanks!
    -Derek

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

    Basically, the bot runs checks using the new rules, this needs to be rebased to see the new rules.

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

    It was a clean rebase, just new 80 character limit for FormAlter which was missed in the earlier rebase.

  • Pipeline finished with Failed
    14 days ago
    Total: 235s
    #452749
  • πŸ‡ΊπŸ‡ΈUnited States dww

    cspell failed on the latest. Added another MR suggestion to fix it. So very close! πŸ˜…

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

    Whoops, I must have had an old pipeline run open cause it was green.

    Just gonna set to needs review this time.

  • Pipeline finished with Success
    14 days ago
    Total: 1146s
    #452828
  • πŸ‡ΊπŸ‡ΈUnited States dww

    Thanks! Pipeline is green. I’ve read the diff many times now. Nothing left to improve to my eyes, other than the follow-ups. πŸ˜‰ let’s get this in.

    Thanks for your patience and persistence!

  • No, this is just a very new rule, was not enforced before.

    Oh, interesting. I could've sworn the 80-character limit for comments was an old rule, but good to know.

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Sorry folks, but we need to get the change records right on this.

    I agree with @berdir in #24 that the change records for this should follow the established pattern used for the plugin conversion from annotation to attributes. Since this issue is implementing the new feature then it should not refer to https://www.drupal.org/node/3499788 β†’ . And if more implementations are planned then we should have a CR for that work, similar to https://www.drupal.org/node/3229001 β†’

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

    I can update the crs to be more explicit.

    https://www.drupal.org/node/3499788 β†’ refers to both issues because neither have landed, whichever lands first the other reference will be removed.

    I'll update the crs shortly.

Production build 0.71.5 2024