The documentation of the return value for _field_info_collate_fields is wrong: In the returned array fields holds an array of existing fields (including deleted fields), keyed by field id. fields_id, on the other hand, is an array of field ids, keyed by name (hence only containing non-deleted fields).

Though this is not a public function, the misleading documentation can seriously impede someone's understanding of how the internals work ...

CommentFileSizeAuthor
#13 998016-12-collate-fields-doc.patch1.48 KBsven.lauer
#10 998016-10-collate-fields-doc.patch1.49 KBsven.lauer
#9 998016-9-collate-fields-doc.patch1.49 KBsven.lauer
#6 998016-collate-fields-doc.patch1.5 KBsven.lauer
#4 998016-collate-field-doc.patch12.31 KBsven.lauer
#2 998016-field-doc.patch1.67 KBsven.lauer

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Title: Documentation problem with _field_info_collate_fields » _field_info_collate_fields doc has the return values backwards

Good catch! The in-code comments (and the code) agree with your interpretation. The docblock definitely needs to be fixed.

sven.lauer’s picture

sven.lauer commented
Status: Active » Needs review
StatusFileSize
new 998016-field-doc.patch1.67 KB

Here is a patch fixing the return value doc.

jhodgdon’s picture

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

There is one technical error:

a) The 'fields_ids" element in the return value should be 'field_ids'.

There are several style/writing/formatting concerns:

b) The proper spelling of the short form of identifier/identification is "ID", contrary to a bunch of existing doc in Drupal, not "id" (which is a psychological term: ego, superego, id).

c) The terminology around "active" was inconsistent. In one place, it says "active fields" and in another, it says "fields that have been marked active". It would be nice if it were consistent.

d) It uses the word "entry" in place of "element", in several spots.

sven.lauer’s picture

sven.lauer commented
Status: Needs work » Needs review
StatusFileSize
new 998016-collate-field-doc.patch12.31 KB

Uhm, okay. I just tried to stay as close to the original as possible, including talk of `entries' and such. Here is another version.

jhodgdon’s picture

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

Ummm. This patch includes a number of code changes. Maybe you uploaded the wron patch?

sven.lauer’s picture

sven.lauer commented
StatusFileSize
new 998016-collate-fields-doc.patch1.5 KB

Ups. Yes, I must have switched things around while renaming patches. This should be the right one.

sven.lauer’s picture

sven.lauer commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I really would like to see the word "element" used to refer to an element of the returned array, not "entry". As it is now, it's mixed. I realize you didn't write the original doc, but if we're doing a patch, let's make it into good, well-written, consistent doc.

Also, don't we use the word "attached", not "assigned" to refer to attaching a field to a bundle?

sven.lauer’s picture

sven.lauer commented
StatusFileSize
new 998016-9-collate-fields-doc.patch1.49 KB

My bad, I did not change all "entries" to "elements". But now.

I did not change the "assigned" to "attached", for it seems to me that that is not quite how the words are used. From http://drupal.org/node/443540#field_bundles :

A bundle is a place to attach a field. [...] A field instance is created from a field when that gets attached to the bundle.

So, a field is attached to a bundle, forming an instance (in a way), so I guess it would be technically 'wrong' to speak of a field attached to an instance. I don't like "assigned", either, though ... so I went with "instances of that field".

sven.lauer’s picture

sven.lauer commented
StatusFileSize
new 998016-10-collate-fields-doc.patch1.49 KB

Args. The "assigned" change was not in the last patch. I should not keep switching version control systems.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I like the current wording, nice work!

There is a typo near the bottom "eelement" that needs to be fixed... and a few lines that end in spaces (we don't like to have extra spaces at the end of the line -- you might try finding a setting in your editor that would either remove them automatically or at least highlight them). Other than that, I think it's ready to go.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work
sven.lauer’s picture

sven.lauer commented
StatusFileSize
new 998016-12-collate-fields-doc.patch1.48 KB

Maybe this time, I'll get it right ... that's a lot of patches for a simple issue. I am learning (also, I thought my editor was set up to remove the trailing whitespace ...).

sven.lauer’s picture

sven.lauer commented
Status: Needs work » Needs review
jhodgdon’s picture

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

6 patches for one issue is fairly normal if not minimal, and as you said, you are learning (and getting your development environment set up right, apparently). :)

If you'd like to work on more patches, you can go to http://drupal.org/project/issues/drupal and search for issues in component "documentation", preferably on Drupal 7.x. There are several. If you specifically want fairly easy issues, you can do an advanced search and look for the "Novice" tag.

Anyway, this patch looks good -- thanks!

yched’s picture

yched commented

Patch is good on my side too - thanks @sven.lauer, and thanks one more time @jhodgdon !

dries’s picture

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.