Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
The auto-generated form_id is [bundle]_[entity_type]_form, but since [bundle] can be anything, there are a ton of potential namespace clashes.
We need to change that into [entity_type]_[bundle]_form, which still assumes that [entity_type] is within the namespace of the owning module, but at least prevents a lot of namespace conflicts already.
Proposed resolution
Decide on a class naming pattern for entity forms and implement it
Remaining tasks
Write the patch
Beta phase evaluation
Issue category | Bug |
---|---|
Prioritized changes | The main goal of this issue is reduce fragility in bundle entity form IDs by removing potential for namespace collisions. |
Disruption | Potentially disruptive for contributed and custom modules/themes because it changes the pattern of form IDs for bundle entity forms. This will impact markup and any form alter hooks targeting specific bundle entity forms. |
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff-1919930-13-16.txt | 1.24 KB | nlisgo |
#16 | bundle_entity_form_ids-1919930-16.patch | 5.75 KB | nlisgo |
#13 | bundle_entity_form_ids-1919930-13.patch | 4.38 KB | nlisgo |
#11 | entity-form-id-1919930-11.patch | 4.52 KB | tim.plunkett |
Comments
Comment #1
sunThis is about the entity form ID, not the CSS class name. Form API auto-generates a class name based on the form ID.
The auto-generated form_id is
[bundle]_[entity_type]_form
, but since [bundle] can be anything, there are a ton of potential namespace clashes.We need to change that into
[entity_type]_[bundle]_form
, which still assumes that [entity_type] is within the namespace of the owning module, but at least prevents a lot of namespace conflicts already.Cross-referencing existing issue: #798062: Rename TYPE_node_form to node_form_TYPE
Comment #2
tim.plunkettThis might break one or two things :)
Comment #4
tim.plunkettWait, seriously? That's it? :\
Oh well, we'll just add more tests in #1913618: Convert EntityFormControllerInterface to extend FormInterface.
Comment #5
sunI'm confused. We don't have any
hook_form_BUNDLE_ENTITY_form_alter()
hook implementations in core anymore...?Comment #6
tim.plunkettApparently not. There is still stuff like forum_form_node_form_alter(), but that's hook_form_ENTITY_form_alter(), which isn't affected here.
Comment #7
sunComment #8
xjm#4: entity-form-id-1919930-4.patch queued for re-testing.
Comment #10
tim.plunkettThis is fixed for node CSS with this from NodeFormController:
That was added in #1269734: Add form name class to forms for styling purposes, a long while ago.
Comment #11
tim.plunkettI don't think this is major. Rerolled.
Comment #12
jhedstromComment #13
nlisgo CreditAttribution: nlisgo commentedComment #14
nlisgo CreditAttribution: nlisgo commentedComment #16
nlisgo CreditAttribution: nlisgo commentedAdjusted existing tests to expect the new format of entity form id.
Comment #17
jhedstromLooks like the unit tests in
\Drupal\Tests\Core\Entity\EntityFormTest
also need updating to take this change into account.Comment #18
nlisgo CreditAttribution: nlisgo commented@jhedstrom I have updated those tests as part of the #16 patch. #13 was a simple re-roll and interdiff in #16 draws attention to the update to the \Drupal\Tests\Core\Entity\EntityFormTest unit tests.
Comment #19
jhedstromThis looks good to go.
I've added a beta-phase evaluation.
Comment #20
jhedstromComment #21
jhedstromI updated the issue summary with Sun's comment from #1 as that more accurately describes this fix.
Comment #22
alexpottThis is a breaking fix which I agree with but I think we need at least CR and an admission of the disruption of changing form IDs in the beta evaluation. Any form alters (should be rare but still) will need to be changed.
Comment #23
jhedstromI've updated the beta phase evaluation and added a CR: https://www.drupal.org/node/2450673
Comment #24
jhedstromComment #27
nlisgo CreditAttribution: nlisgo commentedComment #28
jhedstromBack to RTBC.
Comment #29
alexpottThe beta evaluation is not correct changing a form ID is not only changing markup. However as stated in #22 I still think that we should do this change since it's current disruption will be minimal but the potential of unresolvable namespace conflicts is large.
Comment #30
jhedstromUpdated the beta phase evaluation.
Comment #32
jhedstromRemoving unfrozen section, as this isn't unfrozen, but prioritized bug fix.
Comment #33
alexpottCommitted 86977a1 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.