Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
Comment | File | Size | Author |
---|---|---|---|
#24 | entity_module_cleanup-1315340-d7-24.patch | 10.27 KB | Albert Volkman |
#24 | interdiff.txt | 2.58 KB | Albert Volkman |
#19 | entity_module_cleanup-1315340-d7-19.patch | 9.16 KB | Albert Volkman |
#13 | entity-module-cleanup-13.patch | 8.83 KB | aspilicious |
#11 | entity-module-cleanup-11.patch | 8.82 KB | aspilicious |
Comments
Comment #1
xjmI recommend maybe postponing this until #1184944: Make entities classed objects, introduce CRUD support is committed, which should hopefully be soon.
Comment #2
jhodgdonThis doesn't need postponing now. That issue is resolved (except for the change notification). Patch away!
Comment #3
xjmSince #1184944: Make entities classed objects, introduce CRUD support has been committed, it is safe to roll this patch now.
Comment #4
aspilicious CreditAttribution: aspilicious commenteddone I hope
Comment #5
xjmCutest little patch ever! (Go, us, already having cleaned it up!)
While we're changing these lines, let's use uppercase properly for "ID" and "IDs."
vid
should probably become "revision ID."Comment #6
aspilicious CreditAttribution: aspilicious commentedHmm, this one? (probably not :p)
Comment #7
jhodgdonThese list items need to end in ".":
There is one other place like this too.
Comment #8
aspilicious CreditAttribution: aspilicious commented3th try
Comment #9
aspilicious CreditAttribution: aspilicious commentedComment #10
xjmApplied the patch and found a few other things not covered:
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).Comment #11
aspilicious CreditAttribution: aspilicious commentedThnx all!
Comment #12
jhodgdonThis all looks good to me, with one exception: list formatting-- (sorry I should have mentioned this in #7):
These should start with capital letters after the :
See http://drupal.org/node/1354#lists for full list specs.
Comment #13
aspilicious CreditAttribution: aspilicious commentedIt's ok I should read the docs ;)
Comment #14
jhodgdonOK, I think it's all good now. Thanks!
Comment #15
catchCommitted/pushed to 8.x, thanks!
Moving to 7.x for whatever can be backported.
Comment #16
jhodgdonHm. 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).
Comment #17
xjmWell, 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?
Comment #18
aspilicious CreditAttribution: aspilicious commentedNot going to do this soon. THAT is a lot of work ;) (copy pasting the D8 improvements)
Comment #19
Albert Volkman CreditAttribution: Albert Volkman commentedD7 backport.
Comment #20
aspilicious CreditAttribution: aspilicious commentedI think there is A LOT more work to do. Look at the entity .inc files in the includes folder (in D7).
Comment #21
jhodgdonHmmm... 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)
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:
(needs extra line after first line description)
Get -> Gets
I guess that's it...
Comment #22
xjmWell, I think the backport of the includes A-C didn't include those changes, because the
fileslines aren't there in D8. Thence the followup issue here. :)Comment #23
jhodgdontag
Comment #24
Albert Volkman CreditAttribution: Albert Volkman commentedApplied @jhodgdon's suggestions.
Comment #25
jhodgdon#24: entity_module_cleanup-1315340-d7-24.patch queued for re-testing.
Comment #26
jhodgdonThis patch is nearly ready to go (sorry no one reviewed it sooner...). I just found:
includes/entity.inc
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!