The concept of "bundles" were introduced in D7 by Field API, before there was a formalized notion of entities and an Entity API.

Modules providing fieldable entity types needed to notify Field API when bundles were created, renamed, deleted, so that Field API could perform the corresponding housekeeping. Hence field_attach_[create|rename|delete]_bundle() funcs, and associated hook_field_attach_[create|rename|delete]_bundle() hooks.

Managing bundles, however, is not the job of Field API, and should be moved over to the Entity system. The current code in the API functions (besides invoking the hooks) should be kept as Field API's implementations of hook_entity_[create|rename|delete]_bundle().

Related issues

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Issue tags: +Entity system

proper tag

yched’s picture

Issue summary: View changes

fixed typo, rephrased a bit

andypost’s picture

Big +1

Examples module got a lot of troubles to make a example of entity module. There's a lot of confusion could be found in comments in #893842: Entity example module

Mostly about hooks invocation because each entity module should implement same hooks to notify both systems

fago’s picture

I'm not so sure about that.

What's the purpose of these bundle-attachers? It's about informing the automatically handled storage system to care about a new storage-bundle. Then, considering bundles should be a general entity concept, they have to be able to influence entity storage, thus the storage controller needs to be notified about bundle changes.
But, what's the point in doing so if the automatic storage is handled by the field api anyway? I don't see a reason the entity storage controller needs to be aware of bundle changes, except for caring about automated storage. But if it's really just a field api thing to know about new bundles, I think the current field api attacher approach is fine.

Once we merge field API storage controllers and the entity storage controllers things look different then, though.

If this is issue is more about introducing a unified developer interface for bundle objects (read "CRUD hooks"), I think we should make use of our existing API for that: the entity system. So you get the same developer interface for the same thing to do: read/write/load/delete data objects.

yched’s picture

It's simply not field API that you should be notifying when some of your bundle changes.
If you're a module implementing an entity type, all you need to care about is Entity API, not Field API.

Bundles are defined in hook_entity_info() : entity system.
Thus, if your bundles change, you should be notifying something in the entity system, not in the field system.

IIRC, RDF module also stores some settings by bundle, and currently implements hook_field_attach_rename_bundle(), because that's the only way it can get notified. That's nonsense. It shouldn't be listening to field module for that.

fago’s picture

You are right, modules might want to take bundles into account regardless of storage. Your RDF example proofs that very well. However, then I'm unsure whether bundle is the right notion for that - maybe subtype or something like that would fit better?

Next, we should streamline the hooks to match well to the rest of the entity terminology.

hook_entity_[create|rename|delete]_bundle().
->
hook_entity_"bundle"_[insert|rename|delete]() ?

Thus, the $op should be the last word in the hook name. Then create is already in use for creating the object instance (see entity_create()), thus we should not use it differently here.

fago’s picture

catch’s picture

Yeah I agree with yched on this.

More or less, anywhere there is a field_attach_*() call, we should instead have a generic entity hook that field API implements - because mostly the entire field_attach_*() concept came out of trying to enforce a consistent entity API (that contrib modules would individually implement) where no such thing existed as a concept at the time. So where modules still have to call field_attach_*() themselves, it means something is missing.

indytechcook’s picture

+1 for hook_entity_bundle_* hooks.

Let's add in hook_entity_bundle_settings or hook_entity_bundle_config to allow for a custom form API based form per bundle.

For use cases see http://drupal.org/node/430886#comment-5442598

sun’s picture

Issue tags: +Bundle system
sun’s picture

Issue summary: View changes

typo

yched’s picture

Priority: Normal » Major

So #1950326: Entity displays are not renamed/deleted when a bundle is renamed/deleted. shows the utter absurdity of the current situation : entity module has to implement hooks defined by Field API to be aware that an entity bundle was created/renamed/deleted.

Thus, bumping priority.

Not enough bandwidth to work on that myself right now, though :-/. Any takers ?

swentel’s picture

Assigned: Unassigned » swentel

I will kill this over the weekend.

swentel’s picture

Status: Active » Needs review
FileSize
25.34 KB

Let's see

amateescu’s picture

So.. I guess you didn't like my idea of having entity bundles managed by a config entity? :/

swentel’s picture

It would make sense I guess, but I'd rather wait for field api and content types to configuration getting in first to see if want to refactor bundle management or not. The first patch was an eye opener regarding the tight coupling of entity info and bundles, so I'm more or less glad at the moment we can get that patch forward first (and hopefully maybe RTBC this weekend).

Also, I didn't saw this patch as the trigger to refactor all this stuff, rather just moving things at the right place :)

amateescu’s picture

Status: Needs review » Needs work

Ok then, let's review this one.

+++ b/core/includes/entity.api.php
@@ -137,6 +137,68 @@ function hook_entity_bundle_info_alter(&$bundles) {
+function hook_entity_rename_bundle($entity_type, $bundle_old, $bundle_new) {
+  // Update the extra weights variable with new information.
+  if ($bundle_old !== $bundle_new) {
+    $extra_weights = variable_get('field_extra_weights', array());
+    if (isset($info[$entity_type][$bundle_old])) {
+      $extra_weights[$entity_type][$bundle_new] = $extra_weights[$entity_type][$bundle_old];
+      unset($extra_weights[$entity_type][$bundle_old]);
+      variable_set('field_extra_weights', $extra_weights);
+    }
+  }

We might want to update this example so something that doesn't use variable_get(). We could just have the same example as in hook_entity_create_bundle().

+++ b/core/includes/entity.api.php
@@ -137,6 +137,68 @@ function hook_entity_bundle_info_alter(&$bundles) {
+function hook_entity_delete_bundle($entity_type, $bundle, $instances) {
+  // Remove the extra weights variable information for this bundle.
+  $extra_weights = variable_get('field_extra_weights', array());
+  if (isset($extra_weights[$entity_type][$bundle])) {
+    unset($extra_weights[$entity_type][$bundle]);
+    variable_set('field_extra_weights', $extra_weights);
+  }

Same here.

+++ b/core/includes/entity.inc
@@ -86,6 +86,61 @@ function entity_get_bundles($entity_type = NULL) {
+function entity_create_bundle($entity_type, $bundle) {
...
+function entity_rename_bundle($entity_type, $bundle_old, $bundle_new) {
...
+function entity_delete_bundle($entity_type, $bundle) {

Maybe we should renamed these to entity_bundle_*()?

+++ b/core/includes/entity.inc
@@ -86,6 +86,61 @@ function entity_get_bundles($entity_type = NULL) {
+  // Get the instances on the bundle. field_read_instances() must be used
+  // here since field_info_instances() does not return instances for disabled
+  // entity types or bundles.
+  $instances = field_read_instances(array('entity_type' => $entity_type, 'bundle' => $bundle), array('include_inactive' => 1));

This means that entity.module would still have a hard dependency on field.module. Aren't we trying to get away from that? :)

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldAttachStorageTest.php
@@ -434,9 +434,9 @@ function testFieldAttachDelete() {
-  function testFieldAttachCreateRenameBundle() {
+  function testEntityCreateRenameBundle() {

All these tests should be moved to the entity module at some point, maybe in #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest.

Also, all the new doxygen blocks need to be updated to include type hints.

yched’s picture

Status: Needs work » Needs review

Thanks for kicking this @swentel !

This is looking good. Just one point :

+++ b/core/includes/entity.api.phpundefined
@@ -137,6 +137,68 @@ function hook_entity_bundle_info_alter(&$bundles) {
+ * @param $instances
+ *   An array of all instances that existed for the bundle before it was
+ *   deleted.
+ */
+function hook_entity_delete_bundle($entity_type, $bundle, $instances) {

It shouldn't be the job of entity API to collect $instances and pass them as a param. IMO we can ditch that param completely.
Modules can react to the instance deletion hooks if they need to.

swentel’s picture

This means that entity.module would still have a hard dependency on field.module. Aren't we trying to get away from that? :)

Yeah, I was thinking about that as well. That would be mean we'd have to refactor field_test_field_attach_delete_bundle() to use something different, rather act on deleting the instance, which would make sense I guess.

Woops crosspost with yched and also typo in blockquote tag

swentel’s picture

FileSize
22.54 KB
25.86 KB

Fixed type hinting, renamed to entity_bundle_*, removed $instances from entity_bundle_delete() and refactored field_test_entity_delete_bundle() to field_test_field_delete_instance(). Function bodies in entity.api.php changed like in #1942346: Convert Field API variables to CMI - we'll see who gets in first :)

Moving to entity module could indeed happen in that other issue.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me now :)

Berdir’s picture

Looks good to me as well.

sun’s picture

Awesome.

Berdir’s picture

Issue tags: +Entity Field API

Tagging.

xjm’s picture

Issue tags: -Entity Field API

Oh, thank you thank you thank you thank you. This has driven me CRAZY for a very long time.

xjm’s picture

Issue tags: +Entity Field API

Crossposts, crossposts....

fago’s picture

Status: Reviewed & tested by the community » Needs work

Great work! Here a review:

+++ b/core/includes/entity.api.php
@@ -137,6 +137,65 @@ function hook_entity_bundle_info_alter(&$bundles) {
+ * This hook is invoked after the entity module has performed the operation.

hm, entity module has performed which operation? Renaming? It's not the entity module doing that, maybe the entity system. Howsoever let's just say that this is invoked after performing the operation.

+++ b/core/includes/entity.inc
@@ -86,6 +86,56 @@ function entity_get_bundles($entity_type = NULL) {
+function entity_bundle_create($entity_type, $bundle) {
+  // Clear the cache.
+  entity_info_cache_clear();

I think the function naming is misleading - it suggests me that this actually creates the bundle - but it isn't.

Maybe, a name like "entity_invoke_hook_bundle_create()" would be clearer? Or entity_notify_bundle_create() ? Thoughts?

+++ b/core/includes/entity.inc
@@ -86,6 +86,56 @@ function entity_get_bundles($entity_type = NULL) {
+function entity_bundle_rename($entity_type, $bundle_old, $bundle_new) {

Same here.

+++ b/core/includes/entity.inc
@@ -86,6 +86,56 @@ function entity_get_bundles($entity_type = NULL) {
+function entity_bundle_delete($entity_type, $bundle) {

Same here.

swentel’s picture

I guess I like entity_invoke_hook_bundle_{op} because that's the clearest one.
Thought: merge those 3 functions into entity_invoke_hook_bundle($op) as they basically all do the same: clear cache, invoke hook.

amateescu’s picture

Status: Needs work » Needs review
FileSize
13 KB
25.41 KB

I like entity_invoke_hook_bundle() as well (or maybe entity_invoke_bundle_hook() ?) and unifying those three functions makes sense.

amateescu’s picture

FileSize
9.57 KB
25.41 KB

@Berdir confirmed in IRC that he prefers entity_invoke_bundle_hook() too, so here's anther one.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/includes/entity.incundefined
@@ -86,6 +86,27 @@ function entity_get_bundles($entity_type = NULL) {
+  module_invoke_all('entity_bundle_' . $hook, $entity_type, $bundle, $bundle_new);

I believe module_invoke_all() should not be invoked through the module handler, but we will have to rename it again anyway once Drupal::moduleHandler() is commited so I don't think this is a blocker.

I think @fago's review has been addressed and I think this good to go.

Berdir’s picture

#28: 1374116-28.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Entity system, +Bundle system, +Entity Field API

The last submitted patch, 1374116-28.patch, failed testing.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
26.5 KB

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1374116-32.patch, failed testing.

Berdir’s picture

Opened #1960032: Random failure in TimerUnitTest for that quite interesting random test fail there ;)

amateescu’s picture

FileSize
2.35 KB

That's a *very* interesting exception in EntityQueryTest. Unfortunately (for this small patch), in #32 I also included a small cleanup of two apparently harmless hook implementations: field_test_entity_bundle_create() and field_test_entity_bundle_rename() which didn't receive $entity_type as the first parameter.

Now, the exception raised in EntityQueryTest tells us that we have a hook invocation conflict. More specifically, hook_ENTITY_TYPE_create() (hook = field, ENTITY_TYPE = test_entity_bundle) conflicts with hook_entity_bundle_create() (hook = field_test). Maybe it's more clear written like this: field__test_entity_bundle__create() vs. field_test__entity_bundle_create().

There a couple of options for fixing this:

  1. fix #548470: Use something other than a single underscore for hook namespacing .. there are very slim chances of this happening
  2. finish #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest .. a little bit easier than 1)
  3. extract parts related to the EntityQueryTest from #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest into a separate, smaller patch .. even easier?
  4. revert the two lines changed in #32 and pretend it never happened :D

I think 2) is the best long-term option and this issue should be postponed on it.

Here's an interdiff that shows what happened in #32.

yched’s picture

Ouch. That's kind of frustrating.
An entity type whose name ends with '_entity_bundle' seems like a bit of an odd duck, but right, the coexistence of hook_[ENTITY_TYPE]_[OP]() & hook_entity_bundle_OP() does raise the chances of collision.

No great suggestion comes to mind (except renaming hook_[ENTITY_TYPE]_[OP]() to hook_[ENTITY_TYPE]_entity_[OP](), which I'm sure won't raise much enthusiasm...).
[edit: well, hook_[ENTITY_TYPE]_entity_[OP]() would have potential clashes with hook_entity_[OP]() anyway....
one flat string is not enough, we'd need 3D hook names - er, wait... that's namespaces...]

It seems #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest currently doesn't get rid of the 'test_entity_bundle' entity type either, I find at least an untouched
$entity = entity_create('test_entity_bundle', array(... in it.

Berdir’s picture

Status: Needs work » Needs review
FileSize
719 bytes
26.4 KB

Excuse the stupid question but what's the point of that hook implementation anyway? It does nothing and it even says so, it's also never referenced or anything.

yched’s picture

field_test_entity_bundle_create() is useless, but there is field_test_entity_bundle_rename()...
Doesn't that one clash too ?

Berdir’s picture

That one clashed with hook_$entity_type_create(), there is no hook_$entity_type_rename(). Run Entity Query and all Field API tests locally and it worked fine, we'll see.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Hah, seems that you found 5), which is even easier than all the rest. Always helps to come with a fresh mind :)

xjm’s picture

Looks like this and #1735118: Convert Field API to CMI will conflict.

yched’s picture

Yup, but #1735118: Convert Field API to CMI can live with it, no need holding that one up IMO.

webchick’s picture

Hm. Why do we want to effectively re-introduce "$op"? We spent a lot of time eradicating that from D7.

tstoeckler’s picture

+++ b/core/includes/entity.inc
@@ -86,6 +86,27 @@ function entity_get_bundles($entity_type = NULL) {
+  // Clear the cache.
+  entity_info_cache_clear();

This comment can probably be dropped, no?

+++ b/core/includes/entity.inc
@@ -86,6 +86,27 @@ function entity_get_bundles($entity_type = NULL) {
+  // Let other modules act on renaming the bundle.
+  module_invoke_all('entity_bundle_' . $hook, $entity_type, $bundle, $bundle_new);

The comment needs to be updated.

That was the worst I could find, :-) looks very good.

I disagree with webchick (*runs* :-)) that this is re-introducing $op. entity_invoke_bundle_hook($hook, ...) is just a shorthand for not having 3 functions for entity_bundle_rename(...), entity_bundle_delete(...) and entity_bundle_update(...). $op was primarily painful because you had to switch {} between completely different things in one function. This is not the case here; the hook implementors either handle (only!) renaming, updating or deleting. (Although I must admit, that writing it out, I do find calling entity_bundle_delete(...) (or a bikeshed thereof) much more appealing that entity_invoke_bundle_hook('delete', ...), but I digress.)

webchick’s picture

Right, and hook_block($op) was short-hand for not having hook_block_save(), hook_block_view(), etc. It's still a pattern we deliberately introduced in D7 to make the API more explicit.

tim.plunkett’s picture

@webchick, which part are you referring to? Yes, entity_invoke_bundle_hook() takes an $op, but it invokes "hook_entity_bundle_$op" anyway. The $op part is only exposed if you yourself are calling it, which is much more explicit.

tstoeckler’s picture

Thanks @tim.plunkett. #46 is what I was trying to say, but failed :-)

webchick’s picture

Title: Move bundle CRUD API out of Field API » Change notice: Move bundle CRUD API out of Field API
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Ok, I guess the worst we're doing here is introducing another node_invoke equivalent. While I dislike those layers of abstraction around straight-up module_invoke/module_implements() it's not really any worse than it was before.

Committed to 8.x, with the exception of removing both of those commented lines from entity_invoke_bundle_hook() identified in #44, because agreed that this was a bit much for a 2-line function. :)

This will need a change notice.

Berdir’s picture

The important part here were the added/renamed hooks. That wrapper function can easily be changed and I assume that we will inline those two lines in the config storage controller for bundles so that we can call the hook on the injected module handler (wondering about having a base storage controller for config entities which are bundles... that invokes those hooks by default).

andypost’s picture

Once #1735118: Convert Field API to CMI will be commited this hooks could be easily removed in favor of hook_entity_*

yched’s picture

@andypost : I don't see the connection with #1735118: Convert Field API to CMI. $field as ConfigEntity will deprecate the existing hook_field_[CRUD]_field() hooks in favor of hook_field_entity_[CRUD](), but that's completely unrelated with the hooks we're talking about here ?

swentel’s picture

Status: Active » Needs review

Change notice added here http://drupal.org/node/1964766 - should be fairly simple.

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

swentel’s picture

Title: Change notice: Move bundle CRUD API out of Field API » Move bundle CRUD API out of Field API
Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed

Ok then :)

Berdir’s picture

Title: Move bundle CRUD API out of Field API » Change notice: Move bundle CRUD API out of Field API
Priority: Normal » Critical
Status: Fixed » Needs review

Can we add the new hook names too? As discussed, using those helper functions is optional and just a helper, the important part are the new hooks.

swentel’s picture

Title: Change notice: Move bundle CRUD API out of Field API » Move bundle CRUD API out of Field API
Priority: Critical » Normal
Status: Needs review » Fixed

Good point, added those - should be ok now :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

xjm’s picture

Issue summary: View changes

Updated issue summary.

amateescu’s picture

Found a @todo that we forgot to take care of in this issue: #2148723: Clean up field_info_cache_clear()