Contrib.social

Implement Entity hook attribute

Open on Drupal.org →
Created on 27 December 2024, 5 months ago

Problem/Motivation

📌 [PP-1] Determine how to implement Form Alters with attributes. Active introduces the basic concept of semi-dynamic attribute classes with a suffix/prefix concept, so it can be discovered and documented.

Entity hooks are some of the most frequently implemented hooks, so it makes sense to have a good DX for them, this issue is about figuring out if and which

They are also trickier and more dynamic than other hooks, because they basically have two variable components.

1. They are either for all entity types or a specific one. hook_entity_insert() vs hook_ENTITY_TYPE_insert(). That means there is no shared prefix between the two. there are also exceptions to this, for no specific reason, such as hook_entity_view_mode_alter(), which has no ENTITY_TYPE
2. Then there is the suffix, the actual thing. CRUD hooks like insert/update/create/delete. Access hooks like access, create_access, field_access, view/render hooks like view, view_alter, prepare_view_alter and many. They are invoked in many different places, contrib might add more, the only thing they have in common is that they are related to the entity system.

Given that, I think it's fine to say that 2. isn't actually a dynamic part. It's just many different hooks.

Steps to reproduce

Proposed resolution

One proposal was to deprecate hook_ENTITY_TYPE_something() in favor of hook_entity_ENTITY_TYPE_something(). This would result in all such hooks having a shared entity prefix, both generic and for a specific entity type.

The problem is mentioned in point 2 above. We don't really control invocation of all those hooks. We'd need to make sure that every hook that has the ENTITY_TYPE is prefixed with entity, but it's just a string, invoked in many different places. We can say it's only done for specific hooks, but I'm not 100% sure yet that's worth it. The alternative is we have two classes, e.g. EntityInsert and AnyEntityInsert.

As mentioned, we can say that the second part isn't actually a dynamic part of the hook, just the hook. So it also makes sense to add an attribute class for every hook we think should have one. The question is how we decide that. entity.api.php documents around 70 hooks. I'm not sure all of those really need a specific class, many aren't actually that often used. CRUD and access I think are the most common ones, there's also operations, entity type alter being fairly common. Core in total has around 370 results for "Implements hook_entity_..." (case insensitive).

To summarize, the decision is if we want to rename and deprecate non-entity prefixed hooks, likely in groups so we can have:

// all entity types:
#[EntityInsert]

// node entities only:
#[EntityInsert('node')]

Or if we define two classes and go with:

// all entity types:
#[AnyEntityInsert]

// node entities only:
#[EntityInsert('node')]

Remaining tasks

We likely want to use this as a meta issue and create specific sub-tasks.

User interface changes

N/A

Introduced terminology

N/A

API changes

TBD

Data model changes

N/A

Release notes snippet

📌 Task
Status

Active

Version

11.0 🔥

Component

entity system

Created by

🇨🇭Switzerland berdir Switzerland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @berdir
  • Comment 5 months ago →
    🇺🇸United States nicxvan

    Just coming in to say great summary, I think long term if DX is the goal one attribute is better which means we would need to deprecate hook_ENTITY_TYPE_something()

  • Comment 5 months ago →
    🇨🇭Switzerland berdir Switzerland

    One thing that I wasn't sure about, do we have a plan how we will handle documentation of hooks in the future? I can imagine that we'll deprecate the .api.php files and function hook_... stuff and just put those things on the attribute classes? Which also means that we'll want to do attribute classes for all hooks anyway, not just common ones.

    Going one step further, I wonder if we want to also start to somehow use those hook classes when invoking hooks? Something like:

    \Drupal::moduleHandler()->invokeAll(EntityInsert::hook(), $entity);
\Drupal::moduleHandler()->invokeAll(EntityInsert::hook($entity->getEntityTypeId()), $entity);

    That would make the place where this hook is invoked explicitly referenced through a class usage. That's often useful and while for example PHPStorm can do that, it requires magic string matching. Especially entity crud hooks are tricky because they are built dynamically in the storage handler.

  • Comment 23 days ago →
    🇩🇪Germany donquixote

    In slack discussions related to 📌 Rethink #[Hook] attribute inheritance Active , we argued that attribute classes are not a good place to document the parameters passed to a hook implementation.

    Instead, we decided that we want to link from the attribute class to the old function in *.api.php.

    This also means that we can introduce new attributes without changing the hook names.
    In this issue we should focus on the attributes.

    As for attribute names, maybe we should keep a 'Hook' prefix?
    This would be to distinguish from other attributes that have nothing to do with hooks.
    But I have no super strong opinion, maybe prefix 'Entity' is fine, especially if the implementations all live in 'Hook' namespace.

  • Comment 23 days ago →
    🇺🇸United States dww

    FWIW, we did not use HookFormAlter or HookPreprocess as the child attribute names, but just FormAlter and Preprocess. So yeah, if they're in the Hook namespace, -1 to also prefixing with Hook.

    That said, +1 to documenting the hook and parameters in a real function, not in these hook attribute classes.

  • Comment 23 days ago →
    🇺🇸United States dww

    Also, FWIW, e.g. we only have FormAlter($form_id) and if you pass NULL you get the generic 'all' approach, and if you pass a value, you get the specific one. So I'd say for consistency we should do the same with these entity hooks and if you pass an $entity_type value, you get something specific, and if you leave it NULL, you get all types.

    Therefore, +1 for a single class that takes an optional arg, not 2 separate attributes.

  • Comment 23 days ago →
    🇺🇸United States nicxvan

    I strongly suspect we will not be doing this after recent relevations.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024