Files: 
CommentFileSizeAuthor
#24 entity_module_cleanup-1315340-d7-24.patch10.27 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,273 pass(es).
[ View ]
#24 interdiff.txt2.58 KBAlbert Volkman
#19 entity_module_cleanup-1315340-d7-19.patch9.16 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 37,548 pass(es).
[ View ]
#13 entity-module-cleanup-13.patch8.83 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 34,609 pass(es).
[ View ]
#11 entity-module-cleanup-11.patch8.82 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 34,609 pass(es).
[ View ]
#8 entity-module-cleanup-8.patch6.89 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 34,609 pass(es).
[ View ]
#6 entity-module-cleanup-6.patch6.68 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 34,600 pass(es).
[ View ]
#4 entity-module-cleanup.patch3.69 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 34,589 pass(es).
[ View ]

Comments

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.

Status:Active» Needs review
StatusFileSize
new3.69 KB
PASSED: [[SimpleTest]]: [MySQL] 34,589 pass(es).
[ View ]

done I hope

Status:Needs review» Needs work

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."

Status:Needs work» Needs review
StatusFileSize
new6.68 KB
PASSED: [[SimpleTest]]: [MySQL] 34,600 pass(es).
[ View ]

Hmm, this one? (probably not :p)

Status:Needs review» Needs work

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.

StatusFileSize
new6.89 KB
PASSED: [[SimpleTest]]: [MySQL] 34,609 pass(es).
[ View ]

3th try

Status:Needs work» Needs review

Status:Needs review» Needs work

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).

Status:Needs work» Needs review
StatusFileSize
new8.82 KB
PASSED: [[SimpleTest]]: [MySQL] 34,609 pass(es).
[ View ]

Thnx all!

Status:Needs review» Needs work

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.

Status:Needs work» Needs review
StatusFileSize
new8.83 KB
PASSED: [[SimpleTest]]: [MySQL] 34,609 pass(es).
[ View ]

It's ok I should read the docs ;)

Status:Needs review» Reviewed & tested by the community

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

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed/pushed to 8.x, thanks!

Moving to 7.x for whatever can be backported.

Version:7.x-dev» 8.x-dev
Status:Patch (to be ported)» Fixed

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).

Title:Clean up API docs for entity moduleClean up API docs for entity.inc
Version:8.x-dev» 7.x-dev
Status:Fixed» Patch (to be ported)

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?

Assigned:aenw» Unassigned

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

Status:Patch (to be ported)» Needs review
StatusFileSize
new9.16 KB
PASSED: [[SimpleTest]]: [MySQL] 37,548 pass(es).
[ View ]

D7 backport.

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

Status:Needs review» Needs work

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. :)

Issue tags:+Novice

tag

Status:Needs work» Needs review
StatusFileSize
new2.58 KB
new10.27 KB
PASSED: [[SimpleTest]]: [MySQL] 39,273 pass(es).
[ View ]

Applied @jhodgdon's suggestions.

Status:Needs review» Fixed

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!

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