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
Comment | File | Size | Author |
---|---|---|---|
#37 | entity-bundle-crud-1374116-37.patch | 26.4 KB | Berdir |
#37 | entity-bundle-crud-1374116-37-interdiff.txt | 719 bytes | Berdir |
#35 | interdiff.txt | 2.35 KB | amateescu |
#32 | 1374116-32.patch | 26.5 KB | amateescu |
#28 | 1374116-28.patch | 25.41 KB | amateescu |
Comments
Comment #1
yched CreditAttribution: yched commentedproper tag
Comment #1.0
yched CreditAttribution: yched commentedfixed typo, rephrased a bit
Comment #2
andypostBig +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
Comment #3
fagoI'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.
Comment #4
yched CreditAttribution: yched commentedIt'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.
Comment #5
fagoYou 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.
Comment #6
fagooh, there is already #1380720: Rename entity "bundle" to "subtype" in the UI text and help
Comment #7
catchYeah 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.
Comment #8
indytechcook CreditAttribution: indytechcook commented+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
Comment #9
sunComment #9.0
suntypo
Comment #10
yched CreditAttribution: yched commentedSo #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 ?
Comment #11
swentel CreditAttribution: swentel commentedI will kill this over the weekend.
Comment #12
swentel CreditAttribution: swentel commentedLet's see
Comment #13
amateescu CreditAttribution: amateescu commentedSo.. I guess you didn't like my idea of having entity bundles managed by a config entity? :/
Comment #14
swentel CreditAttribution: swentel commentedIt 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 :)
Comment #15
amateescu CreditAttribution: amateescu commentedOk then, let's review this one.
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().
Same here.
Maybe we should renamed these to entity_bundle_*()?
This means that entity.module would still have a hard dependency on field.module. Aren't we trying to get away from that? :)
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.
Comment #16
yched CreditAttribution: yched commentedThanks for kicking this @swentel !
This is looking good. Just one point :
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.
Comment #17
swentel CreditAttribution: swentel commentedYeah, 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
Comment #18
swentel CreditAttribution: swentel commentedFixed 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.
Comment #19
amateescu CreditAttribution: amateescu commentedLooks great to me now :)
Comment #20
BerdirLooks good to me as well.
Comment #21
sunAwesome.
Comment #22
BerdirTagging.
Comment #23
xjmOh, thank you thank you thank you thank you. This has driven me CRAZY for a very long time.
Comment #24
xjmCrossposts, crossposts....
Comment #25
fagoGreat work! Here a review:
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.
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?
Same here.
Same here.
Comment #26
swentel CreditAttribution: swentel commentedI 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.Comment #27
amateescu CreditAttribution: amateescu commentedI like entity_invoke_hook_bundle() as well (or maybe entity_invoke_bundle_hook() ?) and unifying those three functions makes sense.
Comment #28
amateescu CreditAttribution: amateescu commented@Berdir confirmed in IRC that he prefers entity_invoke_bundle_hook() too, so here's anther one.
Comment #29
BerdirI 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.
Comment #30
Berdir#28: 1374116-28.patch queued for re-testing.
Comment #32
amateescu CreditAttribution: amateescu commentedRerolled for #1950326: Entity displays are not renamed/deleted when a bundle is renamed/deleted..
Comment #34
BerdirOpened #1960032: Random failure in TimerUnitTest for that quite interesting random test fail there ;)
Comment #35
amateescu CreditAttribution: amateescu commentedThat'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()
andfield_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:
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.
Comment #36
yched CreditAttribution: yched commentedOuch. 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.Comment #37
BerdirExcuse 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.
Comment #38
yched CreditAttribution: yched commentedfield_test_entity_bundle_create() is useless, but there is field_test_entity_bundle_rename()...
Doesn't that one clash too ?
Comment #39
BerdirThat 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.
Comment #40
amateescu CreditAttribution: amateescu commentedHah, seems that you found 5), which is even easier than all the rest. Always helps to come with a fresh mind :)
Comment #41
xjmLooks like this and #1735118: Convert Field API to CMI will conflict.
Comment #42
yched CreditAttribution: yched commentedYup, but #1735118: Convert Field API to CMI can live with it, no need holding that one up IMO.
Comment #43
webchickHm. Why do we want to effectively re-introduce "$op"? We spent a lot of time eradicating that from D7.
Comment #44
tstoecklerThis comment can probably be dropped, no?
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.)
Comment #45
webchickRight, 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.
Comment #46
tim.plunkett@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.
Comment #47
tstoecklerThanks @tim.plunkett. #46 is what I was trying to say, but failed :-)
Comment #48
webchickOk, 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.
Comment #49
BerdirThe 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).
Comment #50
andypostOnce #1735118: Convert Field API to CMI will be commited this hooks could be easily removed in favor of
hook_entity_*
Comment #51
yched CreditAttribution: yched commented@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 ?
Comment #52
swentel CreditAttribution: swentel commentedChange notice added here http://drupal.org/node/1964766 - should be fairly simple.
Comment #53
pfrenssenLooks good!
Comment #54
swentel CreditAttribution: swentel commentedOk then :)
Comment #55
BerdirCan 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.
Comment #56
swentel CreditAttribution: swentel commentedGood point, added those - should be ok now :)
Comment #58
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.
Comment #58.0
xjmUpdated issue summary.
Comment #59
amateescu CreditAttribution: amateescu commentedFound a @todo that we forgot to take care of in this issue: #2148723: Clean up field_info_cache_clear()