Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

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

jhodgdon’s picture

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

xjm’s picture

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

aspilicious’s picture

Status: Active » Needs review
FileSize
3.69 KB

done I hope

xjm’s picture

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

aspilicious’s picture

Status: Needs work » Needs review
FileSize
6.68 KB

Hmm, this one? (probably not :p)

jhodgdon’s picture

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.

aspilicious’s picture

3th try

aspilicious’s picture

Status: Needs work » Needs review
xjm’s picture

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).
aspilicious’s picture

Status: Needs work » Needs review
FileSize
8.82 KB

Thnx all!

jhodgdon’s picture

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.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
8.83 KB

It's ok I should read the docs ;)

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

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.

jhodgdon’s picture

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

xjm’s picture

Title: Clean up API docs for entity module » Clean 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?

aspilicious’s picture

Assigned: aenw » Unassigned

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

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
9.16 KB

D7 backport.

aspilicious’s picture

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

jhodgdon’s picture

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

xjm’s picture

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

jhodgdon’s picture

Issue tags: +Novice

tag

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
2.58 KB
10.27 KB

Applied @jhodgdon's suggestions.

jhodgdon’s picture

jhodgdon’s picture

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.