DrupalDefaultEntityController has 7 protected properties defined. None, however, have a docblock to specify what their type is and what they do. They should.

I don't yet grok them enough to know what to put there. If I figure it out I'll see about a patch, but it would be great if someone who knows the entity system better could beat me to it. :-)

CommentFileSizeAuthor
#10 807158-morefixes.patch5.04 KBjhodgdon
#7 807158.patch1.5 KBfreelock
#5 807158.patch1.32 KBfreelock
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Crell CreditAttribution: Crell commented

Reading the code a bit more, it looks like there are also properties that are used that aren't properly defined, such as $this->cache in the constructor. That should be fixed, too. (I'm listing it here since really it's the same fix: Clean up properties in the controller class.)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

This is pretty far from the only class that needs documentation in Drupal. We have a meta-issue about that... but before we do this, we need to agree on standards, which has been languishing in the issue quee for AGES. Sigh.

See:
#718576: [Meta] API documentation for classes and interfaces is incomplete
#718596: Lacking standards for documenting classes, interfaces, methods

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Active » Postponed

This needs to be postponed until the coding standards are defined for classes. See previous comment.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Postponed » Active

Standards are now defined:
http://drupal.org/node/1354#classes

freelock’s picture

freelock
Location Seattle
CreditAttribution: freelock commented
FileSize
807158.patch1.32 KB

Attaching patch with properties documented. Also added protected cache property.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Active » Needs work

Good start... A couple of formatting issues:
- Please, blank lines before each /**
- Each summary line should be one sentence and end with . and be less than 80 characters if at all possible.
- If you have more information past the summary or past 80 characters, put additional info as paragraphs (blank line after summary line before the extra info). The @var should be last.

Oh. And after you attach a new patch, please set the status to "needs review". That will clue us in that it needs reviewing, and also clue in the test bot to verify that the patch applies and doesn't break anything.

Thanks!

freelock’s picture

freelock
Location Seattle
CreditAttribution: freelock commented
Status: Needs work » Needs review
FileSize
807158.patch1.5 KB

Updated patch... Thanks!

Status: Needs review » Needs work

The last submitted patch, 807158.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Assigned: Unassigned » jhodgdon

I'm rerolling and fixing some additional problems, such as spaces at ends of lines, additional missing doc, etc.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review
FileSize
807158-morefixes.patch5.04 KB
freelock’s picture

freelock
Location Seattle
CreditAttribution: freelock commented
Status: Needs review » Reviewed & tested by the community

Double-checked.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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