Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
- node_load(), file_load(), user_load(), etc.pp. functions are completely unnecessary helper/wrapper functions for entity_*() functions.
- Unnecessary code duplication and maintenance of wrapper functions.
Goal
- Consistently use entity system functions only.
Comments
Comment #1
fagohm, I'm not sure about this one. While entity-generic modules certainly need entity-generic hooks, it's much nicer to be able to implement e.g. hook_node_view() instead of hook_entity_view(). Also, I guess a hook_node_view() is faster than hook_entity_view() and a if, in particular when it comes to more of this hook implementations. We might want to benchmark this though.
Comment #2
pounard@#1 I think this issue is not about hooks, which remains totally valid, but about menu helpers/loaders and such.
Comment #3
BerdirThis isn't about the hooks, it's about the API functions. node_load(), node_revision_load() and so on.
But have the same feeling there, I'm not sure if it's worth it. It's als mucher easier to use %node in hook_menu instead of %entity and then having to deal with load arguments.
Comment #4
tim.plunkettAs I understood it, it wasn't about the hooks, but the wrapper functions.
I agree with not removing the hooks. It's like having hook_form_FORM_ID_alter and hook_form_BASE_FORM_ID_alter, both are good.
Comment #5
pounardIndeed, to get back to the original issue, I'm definitely more than OK with removing all those duplicated helpers, they make no sense! If we find a way to convert %node in menu trail automatically to entity_get_controller('node')->load(some id); it would be a huge win. But what about non-entity loaders?
Comment #6
fago>As I understood it, it wasn't about the hooks, but the wrapper functions.
Ah yeah, I see - sry.
I agree that all those entity-type specific helper functions are just a burden. Modules partly define them, partly not - so as a dev you cannot rely on them to be there anyway if you don't know the module. Plus, I've seen developers putting entity-type specific modifications in there - so by not having the functions devs are forced to do it right.
> Consistently use entity system functions only.
Sounds good to me.
Comment #7
bojanz CreditAttribution: bojanz commentedBig +1.
Comment #8
BerdirI'm fine with doing it as well, wondering if we'll run into some issues when doing this. One thing that I don't really like, as mentioned above, is that hook_menu() definitions get quite a bit more complex (%entity + load arguments instead of %node, does that even work for combined stuff like node/%node/revision/%node_revision?). But the new router system might change that anyway.
And it will be a huge patch that will break tons of things, so maybe we should postpone doing this after feature freeze?
Comment #9
larowlanWhile we're talking about major changes - is it worth considering removing all of the hook_menu entries of the form
%entity_type/%entity
eg node/%node
user/%user
And replacing them with a single entry in the entity system
There's #1743590: Isolated Block Rendering which will probably add block/%block and #1606794: Implement new routing system where we're grappling with removing the number of routes (see comment 26 onwards)
And there's #1026616: Implement an entity render controller and the similar rationalization of entity forms.
All of these take us closer to a more restful approach.
Thoughts?
Comment #10
fubhy CreditAttribution: fubhy commentedComment #11
fubhy CreditAttribution: fubhy commentedSo... Considering that node_load(), etc. are currently required for the menu loaders to work (we can't change that to %entity yet because the arguments are in reverse ($entity_type, $id vs. $id, $entity_type). What we can do, however, is to replace node_load(), comment_load(), file_load(), etc. wherever they are used in code currently. Does that make sense to do that right now? Or do we want to fully post-pone this issue. We could actually split it up in 2 separate issue:
#1 Change invocations of node_load(), etc. to entity_load() in code
#2 Fix %node, %comment, etc. once we figured out #1801570: DX: Replace hook_route_info() with YAML files and RouteBuildEvent
Comment #12
fubhy CreditAttribution: fubhy commentedSo.. I've started with removing the entity type specific _load, _load_multiple, _delete, _delete_multiple, _load, _load_multiple and _save functions. Also, I added an entity_delete() function - I think it's reasonable to add that in the scope of this issue.
Let's see what the testbot says. I am pretty sure that I missed some - I did most of this through regex... But it should have covered at least most of it already.
Comment #13
fubhy CreditAttribution: fubhy commentedComment #15
fubhy CreditAttribution: fubhy commentedNext try.
Comment #17
fubhy CreditAttribution: fubhy commented#15: 1757586-14-remove-entity-type-specific-functions.patch queued for re-testing.
Comment #19
fubhy CreditAttribution: fubhy commentedAccidently removed some of the required loaders for menu paths. Bad regex.
Comment #21
fubhy CreditAttribution: fubhy commentedComment #22
tim.plunkettThe @todo's in the old MODULE_load() functions should be in the docblock, not inline. maybe @deprecated?
Comment #23
fubhy CreditAttribution: fubhy commentedYes. Good catch. Let's directly mark them deprecated and only have the menu system still rely on them.
Comment #24
fubhy CreditAttribution: fubhy commentedWoot! We are green. One more follow-up patch to fix #22 and kill some empty line deletions that got added when I debugged :)
Comment #25
sunAs with #1807042: Reorganizie entity storage/list/form/render controller annotation keys, I wonder whether we should postpone this until after Dec 1st? Looks like typical clean-up material to me. Except for the addition of entity_delete() perhaps, but we could do that in a separate issue to make sure that feature gets in?
Comment #26
tim.plunkettAnother current usage for the MODULE_load() functions is for the 'exists' callback for '#type' => 'machine_name'. See #1588422-147: Convert contact categories to configuration system
Comment #27
podaroknice
#24 removes a lot of code
looks like it is very close to RTBC
Comment #28
ParisLiakos CreditAttribution: ParisLiakos commented#24: 1757586-24-remove-entity-type-specific-functions.patch queued for re-testing.
Comment #30
fubhy CreditAttribution: fubhy commentedRe-testing that patch was a brave attempt :P
Comment #31
ParisLiakos CreditAttribution: ParisLiakos commentedwas more of a bump:P
Comment #32
andypostLet's make the issue as meta and split the task to set of issues like:
Comment #33
sunComment #34
ianthomas_ukThere's debate about whether we want to do this, since entity_load can't type hint what type of entity it will return. I haven't got an exact issue to postpone against, but please check #2010138-12: Remove taxonomy_term_load_multiple() in favour of \Drupal\taxonomy\Entity\Term::loadMultiple(). and #2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities before spending time on this.
Comment #35
BerdirYeah, Closing as duplicate of #2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities and the corresponding create() method.
Comment #36
BerdirHmpf.