From #2448503: Convert the "Field edit" form to an actual entity form :
(yched) - field_ui's job of "decorate the 4 core entity types (field, field_storage, view_mode, form_mode)" seems a bit randomly split between field_ui_entity_type_build() and field_ui_entity_type_alter() 200 lines lower.
The usual rule of thumb is "hook_*_build() is to add stuff that's not there, hook_*_alter() is to modify what's already there", right ? So it should all go in field_ui_entity_type_build() ?
(amateescu) - Very good point! Although, the hook_*_build() in this case is meant for "adding something new on the top level" afaik, (e.g. a new entity type definition that's not in code, for example what ECK will use), so what we're currently doing in field_ui_entity_type_build() should move to field_ui_entity_type_alter() because we're just altering the definition of already existing entity types
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 2459949-9.patch | 4.34 KB | amateescu |
| #6 | 2459949-6.patch | 2.64 KB | amateescu |
Comments
Comment #1
yched commentedbetter title
Comment #2
yched commentedComment #3
jibranHow about that!
Comment #4
amateescu commentedLet's do it.
Comment #6
amateescu commentedLet's do it again after pulling latest HEAD :/
Comment #7
tstoecklerI think it should be the other way around. Specifically, I disagree with
Everything that can go into build, should. That would be my version of a rule of thumb. With the current patch you cannot e.g. alter the list builder class for field storages (which is totally valid for a
better_field_ui.module) without going through the crazy module-weight-dance which the build-then-alter construct circumvents in the first place.On the other hand, we don't really lose any flexibility by putting all that information into the build phase. Semantically, I think it is also more correct (because we are not altering existing information) but I realize that is arguable.
Comment #8
jibranWe need beta evaluation here.
Comment #9
amateescu commentedTBH, I don't really care about what goes where for these two hooks :)
https://www.drupal.org/node/2196275 seems to agree with you, so let's do that.
Comment #10
tstoecklerLooks good to me, thanks!
Not RTBCing, to see whether I'm the only one of that opinion but codewise this is good to go.
Comment #11
jibranComment #12
yched commentedYes, agreed with #7, I think we can't be wrong with _build() :-)
Comment #13
alexpottZero disruption and this allows contrib to alter sanely. Committed 76524ee and pushed to 8.0.x. Thanks!