There are some places in the new entity query which is not documented perfectly, see http://drupal.org/node/1854708#comment-7069386 for example.

CommentFileSizeAuthor
#7 drupal-1918254-7.patch1.72 KBdawehner
#7 interdiff.txt836 bytesdawehner
#5 drupal-1918254-5.patch1.71 KBdawehner
#5 interdiff.txt787 bytesdawehner
#3 drupal-1918254-3.patch1.67 KBdawehner
#1 drupal-1918254-1.patch1.65 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
FileSize
drupal-1918254-1.patch1.65 KB

.

jhodgdon’s picture

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

Thanks! A few things to fix:
- Needs newline between @param and @return sections.
- SQL is an acronym and needs to be all-caps in documentation. Actually though, I think rather than calling it a "sql field" (I had to think quite a while before I figured out what that meant), maybe this documentation should refer to constructing a database table field alias or something like that? I just thought that this phrase:

"a sql field for a given field"

was confusing, given that the word "field" has two meanings only a couple of words apart.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
drupal-1918254-3.patch1.67 KB

Thanks for the feedback. Yeah I was uncomfortable with that as well.
What do you think about this, is this easier to understand?

jhodgdon’s picture

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

Much better, thanks! I still don't really understand the return value documentation though:

An expression of the field and language, so for example "base_table.id".

Maybe something more like this would be clearer?

An expression that will select the given field for the given language in a SELECT query, such as 'base_table.id'.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
interdiff.txt787 bytes
drupal-1918254-5.patch1.71 KB

Oh much much better!!

jhodgdon’s picture

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

Glad we agree on the wording. One more thing: indent two spaces instead of one on @return docs.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
interdiff.txt836 bytes
drupal-1918254-7.patch1.72 KB

Oh damn :) I'm feeling a bit like a novice starting the first contribution.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Looks good -- I'll get it committed soon (may need to wait for some Avoid Commit Conflicts issues first, but sometime soon). Thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

This didn't conflict after all, so I committed it. Thanks again!

Status: Fixed » Closed (fixed)

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