Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
documentation
Priority:
Normal
Category:
Bug report
Assigned:
Issue tags:
Reporter:
Created:
15 Feb 2012 at 11:17 UTC
Updated:
29 Jul 2014 at 20:23 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
jhodgdonGood point. The documentation does say this hook is invoked from field_attach_delete(), which is true... So it's invoked when a field is removed from an entity bundle. The documentation should say this in a clearer manual.
Probably a good Novice project...
Comment #2
joachim commented> So it's invoked when a field is removed from an entity bundle
Are you sure?
Field_attach_delete says:
> Delete field data for an existing entity. This deletes all revisions of field data for the entity.
Which means when a node is deleted and it has a field of type foo.
Comment #3
jhodgdonAh, you're right joachim, thanks! It would be invoked from deleting a node, comment, etc. with that field attached to it, to delete the data that is in that particular field instance.
I was confusing field_attach_delete() [which deletes data in a field on a particular entity object, and invokes hook_field_delete()] with field_delete_instance() [which removes a field from a bundle, and invokes hook_field_delete_instance()].
Comment #6
nmudgal commentedAttached patch contains below proposed wordings
Comment #7
jhodgdonThere are a couple of problems with this patch:
- Spelling: instanace -> instance
- Accuracy: it is called *from* field_attach_delete, not *before* field_attach_delete.
- Completeness: you left out the part that says it is called just before the data is deleted from field storage, as stated in the original text being replace by this patch.
- Also it needs some grammar cleanup. [let me know if you need assistance with that part]
Comment #8
nmudgal commentedComment #9
nmudgal commentedSo much issues :-)
Reworded to this & attached patch.
Is it looking good now, let me know if needs more cleanup or so ?
Thanks
Comment #10
jhodgdonMuch better. :)
- I would replace the "and" with a comma, and then we're done. "...defines a field, just before the..."
- Needs "a" here: "the data of a particular field instance"
Comment #11
nmudgal commentedUpdated :-)
Comment #12
jhodgdonThat looks good to me. joachim - since you reported this, any comment on the proposed change?
Comment #13
jhodgdonI think I'll tenatively set to RTBC and see if anyone objects.
Comment #14
joachim commentedI read the patch without rereading the comments above to see if I get it.
> the data of a particular field instance
That almost makes it sound like it's entity data in that field, rather than just the actual field instance that is about to be deleted. But then I'm still having breakfast -- brain not in gear yet :)
Comment #15
jhodgdonJoachim: Let us know when your breakfast is settled. :) Better wording suggestions will be cheerfully adopted.
Comment #16
webchickSounds like this still needs review yet. Though FWIW, the wording seems clear to me. :) But I don't eat breakfast. ;)
Comment #17
joachim commentedYeah, I can't figure out from that whether it's about deleting entities or deleting an instance. Admittedly it's outright pre-breakfast today.
I suggest:
This hook is invoked on the module that defines a field, just before data in this field for a particular entity is deleted from field storage in field_attach_delete().
Comment #18
nmudgal commentedWell #17 makes it more clear, re-rolled the patch.
Comment #19
nmudgal commentedComment #20
jhodgdonShould we say ... data in this field for a particular entity instance is deleted... ?
Comment #21
jhodgdonComment #22
joachim commentedEntities don't have instances, fields do.
And you can't have entity data without an instance, so it's redundant. Furthermore, I'm not sure whether the entity data is counted as being 'in' the instance or 'in' the field, since it's all in the one table. The 'instance' is more the particular configuration (field options, widget, display) for that entity bundle.
Comment #23
jhodgdonOK, my mistake. What do you call a particular entity object then?
Comment #24
joachim commentedYou mean like a node? That's just an entity.
entity <--> node as in the single piece of content data
entity type <--> 'node' as in the concept
It's all terribly confusing; even diagrams like that just add to confusion potential :/
Comment #25
jhodgdonOK, how about "for the particular entity object" then?
Comment #26
joachim commentedYup.
Comment #27
nmudgal commentedThe case is,
entity would be an instance of particular entity type & that means fields have entities not the other way round,
while it currently says that
data in the field which a entity object is carrying would be deleted.
So I think it should be:
"for the particular entity type" ??
Correct me if I am wrong :-)
Comment #28
jhodgdonUm. No, I don't think so. This hook is for responding when a single node, taxonomy term, user, etc. *object* is being deleted, and your module's field needs to do something special with some field data that it is storing outside the normal field system's storage.
So we shouldn't be talking about entity types at all. This is responding to deleting an entity object, by deleting the outside-stored information your module has for that particular field.
How about this [note that I have changed the first line too]:
---
Define custom delete behavior for this module's field data.
This hook is invoked from field_attach_delete() on the module that defines a field, during the process of deleting an entity object (node, taxonomy term, etc.). It is invoked just before the data for this field on the particular entity object is deleted from field storage. Only field modules that are storing or tracking information outside the standard field storage mechanism need to implement this hook.
---
And maybe we could have an @see link to the hook that does the converse (allows field modules to store some outside information when the node/term/etc. object is inserted or updated)? I don't know off-hand what that hook is, but there must be one.
Comment #29
nmudgal commentedAha get it, well I guess this doc needs some update too http://drupal.org/node/1261744 or may be I mis-read it.
Comment #30
jhodgdonI don't see anything particularly wrong there, except that I'm not sure it is entirely consistent in its terminology around entity types and entity objects. It's certainly easy to get confused about!
Comment #31
nmudgal commentedyeah that's what i meant, it's a bit confusing around entity types & entities.
anyways in #28, for @see link, I think it's "hook_field_insert" and "hook_field_update" ?
Comment #32
jhodgdonYes, those look like the right hooks. Dang, their docs are also very weak and have the same problems as hook_field_delete does... would someone like to expand this patch to put similar explanations in them, or at least put @see references between these three hooks?
Comment #33
nmudgal commentedRe-rolled the patch along with adding @see references for other two hooks too.
Comment #34
jhodgdonSo far, so good, thanks!
Now, can we also add similar information to the update/insert hooks to what we added to the delete hook, saying that they are useful to store information outside the standard field storage mechanism?
Comment #35
nmudgal commentedUpdated patch with similar information. Thanks.
Comment #36
jhodgdonLooks great!
One little problem: "inserted from field storage." on the insert hook -- needs to be "inserted into field storage." :)
Update hook has the same problem: updated from -> updated into.
Comment #37
nmudgal commentedYeah 'inserted from' doesn't make much sense either :-) Anyways last patch had trailing whitespaces also, so removed those too.
Thanks
Comment #38
jhodgdonNow, this looks like good documentation. Thanks!
Patch applied to both 8.x and 7.x. Committed to both.