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

Reference: https://www.drupal.org/core/beta-changes
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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Title: Decide on appropriate pattern for class names on entity forms. » Bundle entity form IDs violate module namespaces (both on server-side + front-end CSS)
Category: task » bug
Status: Postponed » Active
Issue tags: -D8 upgrade path

This 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

tim.plunkett’s picture

Status: Active » Needs review
FileSize
504 bytes

This might break one or two things :)

Status: Needs review » Needs work

The last submitted patch, entity-form-id-1919930-2.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.31 KB

Wait, seriously? That's it? :\
Oh well, we'll just add more tests in #1913618: Convert EntityFormControllerInterface to extend FormInterface.

sun’s picture

Issue tags: -Entity system, -Blocks-Layouts

I'm confused. We don't have any hook_form_BUNDLE_ENTITY_form_alter() hook implementations in core anymore...?

tim.plunkett’s picture

Apparently not. There is still stuff like forum_form_node_form_alter(), but that's hook_form_ENTITY_form_alter(), which isn't affected here.

sun’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Issue tags: -Block plugins

#4: entity-form-id-1919930-4.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Block plugins

The last submitted patch, entity-form-id-1919930-4.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.55 KB

This is fixed for node CSS with this from NodeFormController:

    // Override the default CSS class name, since the user-defined node type
    // name in 'TYPE-node-form' potentially clashes with third-party class
    // names.
    $form['#attributes']['class'][0] = drupal_html_class('node-' . $node->type . '-form');

That was added in #1269734: Add form name class to forms for styling purposes, a long while ago.

tim.plunkett’s picture

Priority: Major » Normal
Issue summary: View changes
FileSize
4.52 KB

I don't think this is major. Rerolled.

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
nlisgo’s picture

nlisgo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: bundle_entity_form_ids-1919930-13.patch, failed testing.

nlisgo’s picture

Status: Needs work » Needs review
FileSize
5.75 KB
1.24 KB

Adjusted existing tests to expect the new format of entity form id.

jhedstrom’s picture

Status: Needs review » Needs work

Looks like the unit tests in \Drupal\Tests\Core\Entity\EntityFormTest also need updating to take this change into account.

nlisgo’s picture

Status: Needs work » Needs review

@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.

jhedstrom’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

This looks good to go.

I've added a beta-phase evaluation.

jhedstrom’s picture

Issue summary: View changes
jhedstrom’s picture

Issue summary: View changes

I updated the issue summary with Sun's comment from #1 as that more accurately describes this fix.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This 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.

jhedstrom’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

I've updated the beta phase evaluation and added a CR: https://www.drupal.org/node/2450673

jhedstrom’s picture

Issue summary: View changes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: bundle_entity_form_ids-1919930-16.patch, failed testing.

nlisgo’s picture

Status: Needs work » Needs review
jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Block plugins +Needs issue summary update

The 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.

jhedstrom’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Updated the beta phase evaluation.

jhedstrom’s picture

Issue summary: View changes

Removing unfrozen section, as this isn't unfrozen, but prioritized bug fix.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs issue summary update

Committed 86977a1 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 86977a1 on 8.0.x
    Issue #1919930 by tim.plunkett, nlisgo: Bundle entity form IDs violate...

Status: Fixed » Closed (fixed)

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