Clean up API docs for entity.inc

I recommend maybe postponing this until #1184944: Make entities classed objects, introduce CRUD support is committed, which should hopefully be soon.

This doesn't need postponing now. That issue is resolved (except for the change notification). Patch away!

Since #1184944: Make entities classed objects, introduce CRUD support has been committed, it is safe to roll this patch now.

done I hope

Cutest little patch ever! (Go, us, already having cleaned it up!)

+++ b/core/modules/entity/entity.moduleundefined
@@ -122,12 +122,13 @@
- * Helper function to extract id, vid, and bundle name from an entity.
+ * Extracts id, vid, and bundle name from an entity.

@@ -159,7 +160,7 @@
- * Helper function to assemble an object structure with initial ids.
+ * Assembles an object structure with initial ids.

While we're changing these lines, let's use uppercase properly for "ID" and "IDs." vid should probably become "revision ID."

Hmm, this one? (probably not :p)

These list items need to end in ".":

 * @return
  *   A numerically indexed array (not a hash table) containing these
  *   elements:
- *   0: primary id of the entity
- *   1: revision id of the entity, or NULL if $entity_type is not versioned
+ *   0: primary ID of the entity
+ *   1: revision ID of the entity, or NULL if $entity_type is not versioned
  *   2: bundle name of the entity

There is one other place like this too.

3th try

Applied the patch and found a few other things not covered:

• Line 594 of entity.query.inc needs wrapping.
• entity_hook_crud_test.test needs an @file.
• entity_hook_crud_test.module needs an @file (and possibly to replace all those weird // headers with defgroups or just remove them).
• EntityMalformedException docblock needs a verb ("defines" or something).

Thnx all!

This all looks good to me, with one exception: list formatting-- (sorry I should have mentioned this in #7):

+ *   0: primary ID of the entity.
+ *   1: revision ID of the entity, or NULL if $entity_type is not versioned.
+ *   2: bundle name of the entity.

These should start with capital letters after the :

See http://drupal.org/node/1354#lists for full list specs.

It's ok I should read the docs ;)

OK, I think it's all good now. Thanks!

Committed/pushed to 8.x, thanks!

Moving to 7.x for whatever can be backported.

Hm. There isn't actually an entity module in Drupal 7, so I think this is not portable (the include files would go in a different patch port).

Well, since the patch here has a lot of the needed changes and the include doesn't exist in D8, can we repurpose this issue for that?

Not going to do this soon. THAT is a lot of work ;) (copy pasting the D8 improvements)

D7 backport.

I think there is A LOT more work to do. Look at the entity .inc files in the includes folder (in D7).

Hmmm... First of all, I was going to say that changes to common.inc should not be covered here, but I see that the "includes A-C" issue is already closed, so that is fine. We must have missed a couple of items.

So, on to the review!

a)

  * @return
  *   A numerically indexed array (not a hash table) containing these
  *   elements:
- *   0: primary id of the entity
- *   1: revision id of the entity, or NULL if $entity_type is not versioned
- *   2: bundle name of the entity
+ *   - 0: Primary ID of the entity.
+ *   - 1: Revision ID of the entity, or NULL if $entity_type is not versioned.
+ *   - 2: Bundle name of the entity.
  */
 function entity_extract_ids($entity_type, $entity) {
   $info = entity_get_info($entity_type);
@@ -7520,9 +7521,10 @@ function entity_extract_ids($entity_type, $entity) {
  * @param $ids
  *   A numerically indexed array, as returned by entity_extract_ids(),
  *   containing these elements:
- *   0: primary id of the entity
- *   1: revision id of the entity, or NULL if $entity_type is not versioned
- *   2: bundle name of the entity, or NULL if $entity_type has no bundles
+ *   - 0: Primary ID of the entity.
+ *   - 1: Revision ID of the entity, or NULL if $entity_type is not versioned.
+ *   - 2: Bundle name of the entity, or NULL if $entity_type has no bundles.
+ *

It seems like maybe we shouldn't document this return value twice. Maybe the extra information that is in the second function's doc should be moved to the first one, and then just referenced?

b) The rest of the patch looks fine, but when I looked at entity.inc I saw some other things that weren't in the patch and need fixing. Such as:

  /**
   * Attaches data to entities upon loading.
   * This will attach fields, if the entity is fieldable. It calls

(needs extra line after first line description)

  /**
   * Get the total number of results and initialize a pager for the query.
   *
   * This query can be detected by checking for ($this->pager && $this->count),
   * which allows a driver to return 0 from the count query and disable
   * the pager.
   */
  function initializePager() {
 

Get -> Gets

I guess that's it...

Well, I think the backport of the includes A-C didn't include those changes, because the files lines aren't there in D8. Thence the followup issue here. :)

tag

Applied @jhodgdon's suggestions.

#24: entity_module_cleanup-1315340-d7-24.patch queued for re-testing.

This patch is nearly ready to go (sorry no one reviewed it sooner...). I just found:

includes/entity.inc

/**
-   * Get the total number of results and initialize a pager for the query.
+   * Gets the total number of results and initialize a pager for the query.

initialize -> initializes

Since this was the only problem and this has been around for so long, I just made that one small change and committed it. Thanks!