- Issue created by @nicxvan
- π¨πSwitzerland berdir Switzerland
Setting to postponed to prevent someone from picking this up for now. I think it's too early? Should at least wait until preprocess and module implements alter is done, for the .module/admin.inc cleanup part?
My gut reaction is that yes, this is too much for a single issue. admin.inc alone is 400 loc that will need to be reviewed somehow for things like DI and what one.
Some functions like content_translation_translate_access are related to several open issue about improving and fixing translation access. That's one deep rabbit hole. I'm not sure if I just want to move it to a service just so it's there.
- πΊπΈUnited States nicxvan
No need to postpone this, as you mentioned this needs planning which this issue is for.
I don't plan on creating the follow ups yet but I want to start parsing them out, the hook cleanup ones can probably be worked on.
I'll look for related issues to functions that make sense like
content_translation_translate_access - π¨πSwitzerland berdir Switzerland
> No need to postpone this, as you mentioned this needs planning which this issue is for.
I've seen a fair share of issues being picked up that by less experienced contributers to work on it, but doesn't matter too much.
On the Hooks classes, that's not quite how I would group things. For example InfoAlterHooks do all alter things, but the things they alter are very different. Somehow you've grouped many of them by the suffix, to me the prefix/first word is typically the primary grouping factor, as suggested in the cleanup meta issue (of which this should probably be a child issue?). To me, the goal should be that I can look at a list of hook classes and be able to make an assumption about which one contains e.g. hook_entity_type_alter(). Right now, I'd have no idea.
entity_bundle_info_alter(), entity_base_field_info(), entity_extra_field_info(), entity_type_alter() to me belong in the same group, they are altering different aspects of of entity type definitions. That some are called type alter and others info alter and some aren't alters is just a bit inconsistent naming. I'd also put hook_field_info_alter() in the same group I think. entity and field API's are closely related. How to call that depends a bit on what we'd do with the rest of the entity hooks. In most modules, I'd probably put all entity related hooks in a single Hooks class and just go with EntityHooks. This module has a lot of them, so I seen argument of splitting that up into definition and runtime hooks, so I'd probably go with EntityDefinitionHooks for this one?
Despite not starting with entity, language_fallback_candidates_entity_view_alter() is also an entity system hook, so that could be in EntityOperations too if we want to go that (not 100% sure I like it, but not sure that just EntityHooks is better). I can also seen an argument that language_fallback_candidates_entity_view_alter, the language_content_settings_* and language_types_info_alter() could be in a LanguageHooks. Unsure about this one. some of those operate on an entity and some don't, but pretty much all the hooks do. If you hook at page_attachments_alter(), that is also 100% about entity operation stuff.
views_data_alter just one hook, but for most modules, we've already established that we have a ViewsHooks class, as they were migrated from views.inc hook group, so I think it makes sense to adopt that here as well.
The leftovers possibly in ContentTranslationHooks?
Thoughts?
- πΊπΈUnited States nicxvan
I like that strategy, I'll work on reorganizing in a comment at some point so it's easier to compare.
I only created the one follow up cause I'm pretty sure that direction is good no matter what is decided here.