Problem

  • Tons of duplicate API documentation.

Proposed solution

  1. Remove hook_node_load(), hook_comment_load(), etc.pp. entirely.
  2. Add generic hook_ENTITY_TYPE_load() API docs to entity.api.php instead.
CommentFileSizeAuthor
#1 entity.api-docs.1.patch41.31 KBsun

Comments

sun’s picture

Status: Active » Needs review
StatusFileSize
new41.31 KB

Wowza. Tons is understated.

moshe weitzman’s picture

Nice! We really should link have some kind of way to link from deprecated API functions to new ones (i.e. help people who go searching for hook_user_load())

sun’s picture

jhodgdon’s picture

Component: entity system » documentation
Status: Needs review » Needs work

Good idea, but this patch isn't quite there...

a) The entity.api.php file should be under modules/system, by convention -- that is where all of the other generic api.php files are, and it seems silly to have one in includes when the rest are in modules/system. Either that or move all the others to includes. Hmmm... I guess this one was already in the wrong place. Uck. Maybe that is a separate issue...

b) I think this documentation needs more information or at least consistency. For instance, the load hook:

/**
+ * Act on entities when loaded.
+ *
+ * This is a generic load hook called for all entity types loaded via the
+ * entity API.
+ *

This one has an extra line; the other hooks do not have a similar line... This line really doesn't tell us anything useful that the first line doesn't tell us. It will be really difficult to use api.drupal.org to find out where these hooks are invoked (since there is no module_invoke_all('ENTITY_TYPE_load') call), so I think each hook with ENTITY_TYPE in the name needs to have a line saying where it's invoked. And you can get rid of the meaningless line saying "this is a generic load hook".

c) These parameters:

+ * @param array $entities
+ *   The entities keyed by entity ID.

What are the array elements? This should be documented (I think maybe they are EntityInterface objects?).

c) It would also help if the docs first lines were more consistent. Some say "Act on entities" and some say "Act on an entity", and there are two that say just "Act on entities being inserted" (but presumably they are different hooks?). I guess it's that there are hook_entity_* and hook_ENTITY_TYPE_* right? Well, they should not have the same first lines.

d) I also think we need a topic that describes the order of invocation of the hooks in different operations. For the node hooks, we had:
http://api.drupal.org/api/drupal/modules!node!node.api.php/group/node_ap...
which is presumably going away. This is rather complex and I think this type of topic needs to be made for entities.

e) There is a LOT of information being lost in this proposed patch. For instance, hook_comment_view_alter had a bunch of documentation that is just being thrown out. This information needs to either be added to the generic entity type hook docs (if appropriate), or maintained as specific hook documentation. We shouldn't get rid of the documentation we had if it is still accurate and still useful.

jhodgdon’s picture

this is not really a coding standards issue and we do not use the "documentation" tag in core issues

berdir’s picture

In another issue, I suggested to keep those existing explicit hooks but just make them a single line and add a @see hook_entity_info(). Would have the advantage that we could keep the existing implements ... and all other references.

Which is something this patch is also missing. If we really want to remove them completely, we also have to update all docblocks of all those implementations and document them like we document hook_form_FORM_ID_alter()?

jhodgdon’s picture

Yes, if we adopt the idea proposed here (eliminate the specific hooks), then we would also need to change all the documentation references to each of the removed hooks. Ideally that would be in the same documentation patch -- or perhaps one per entity? (one patch for user, one for node, etc.). And I really don't want to lose any information we have in docs now -- if there is entity-specific information, we should retain the entity-specific docs... which would be a good argument for keeping the specific-entity hook stubs (they could have some specific information in there if appropriate).... well I don't know what's best... as long as it's all consistent, that would be good.

plach’s picture

@#4/a:

Now that we have the Entity module again we could simply move entity.api.php back there, I guess.

jhodgdon’s picture

Issue summary: View changes
Status: Needs work » Closed (cannot reproduce)

This was taken care of on other issues.