We have a convention, don't we, that when modules generate identifiers, they should be prefixed with the module name. This helps avoid name collisions. Seems like node.module should play by the same rules. For example, there's code in node_type_form_validate() that prevents 'theme' from being used as a node type name, because the resulting form id would conflict with theme_node_form(). But then, don't we also need to prevent 'template_preprocess' from being a node type name? Also, we call hook_form_FORM_ID_alter(). What if we wanted to add another hook, like hook_form_foo_alter() that also acted on FORM_ID, so we'd have hook_form_foo_FORM_ID_alter(). That would create conflicts for any node type that starts with 'foo_'. Fixing node.module to follow our own convention fixes these problems.
This would be an API change for D7, so assigning to D8.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | drupal8.node-type-form.6.patch | 6.49 KB | sun |
| drupal.rename_node_forms.patch | 8.35 KB | effulgentsia |
Comments
Comment #1
effulgentsia commentedNote. I think the reason we've had TYPE_node_form for so long is that before CCK, it was assumed that TYPE itself was prefixed by the module name implementing the node type. So, the blog module implemented a blog type. With that assumption, TYPE_node_form is an ok naming convention. But, CCK changed all that, and we should have fixed the naming once D6 allowed administrator defined content types. Especially with most of CCK now in core, D7 would be a great time to fix this. Leaving this assigned to D8, however, until there's a stronger case made for breaking BC in D7.
Comment #2
sunOriginating issue: #757154: Base form_id via hook_forms() not taken into account for #validate, #submit, hook_form_FORMID_alter()
I don't think that node_$type_form is right though. Just like we do sub-theme functions, node_form_TYPE - i.e., as suffix - would be consistent with other sub-type-logic we have.
We could even go one step ahead and do node_form__TYPE. That would allow us to take over that form_id directly as value for #theme and we'd automatically get the sub-theme functions/pattern behavior. Since a form's default #theme value is affected by aforementioned issue, I think that we should change it in that patch.
Although this is an API change, I'd say it's a very small one, which only affects modules exposing custom node types and perhaps hook_form_alter() implementations in custom modules of existing D7 sites, which should be rare.
Comment #3
webchickI don't care how minor of an API change it is, it's a "nice to have" that doesn't address a critical bug (these Form IDs have persisted since 4.6 or earlier), so D8.
Comment #4
quicksketchThis doesn't seem major by any stretch of the imagination. There is an incredibly minimal difference here. Major issues are now preventing new features per http://drupal.org/node/1201874. This has no business preventing new features.
Comment #5
sunNote sure about #4 - this is an important discussion we need to have from an API perspective, since we risk to transpose/project the wrong namespace pattern from node bundles to other entity bundles.
Reading #2 again,
node_form__TYPElooks very appealing.Comment #6
sunRe-rolled against latest branch HEAD.
Comment #8
sunThis topic came up for custom blocks again: #1919930: Bundle entity form IDs violate module namespaces (both on server-side + front-end CSS)
Since we have a lot more bundle entity forms in D8 now, we definitely have to do something about this.
Comment #9
nancydruI would not consider this "minor" - it might not be a big deal for core, but it's going to break lots of contrib code.
Comment #10
andypostNow
EntityFormController::getFormId()cares about it.