The $field paramater to field_access() is pretty confusing. Some people think it's a field name, others think it's the whole field structure array. Getting this wrong leads to silently causing access bypass bugs. For example, I just found one over at #1393194: Access bypass in field_collection.

http://api.drupal.org/api/drupal/modules--field--field.module/function/f... just says:

"$field The field on which the operation is to be performed."

It'd sure be nice if a) that had a type and b) explained it's the field structure definition, not just a field name.

Not sure if "documentation bug" is the right issue tag for stuff like this anymore...

CommentFileSizeAuthor
#18 1393234-field-access-docs-2.patch894 bytesNiklas Fiekas
#14 1393234-field-access-docs.patch1.01 KBNiklas Fiekas
#6 1393234-6.field_access_api_docs.patch1.03 KBdww
#3 1393234-3.field_access_api_docs.patch858 bytesdww
#1 1393234-1.field_access_api_docs.patch829 bytesdww
#1 1393234-1.field_access_api_docs.d7.patch809 bytesdww
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Assigned: Unassigned » dww
Status: Active » Needs review
FileSize
1393234-1.field_access_api_docs.d7.patch809 bytes
1393234-1.field_access_api_docs.patch829 bytes

Patches for D7 and D8.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Status: Needs review » Needs work 
+++ b/core/modules/field/field.moduleundefined
@@ -975,13 +975,14 @@ function field_has_data($field) {
  *   - "edit"
  *   - "view"

These should be single quoted strings. See below.

+++ b/core/modules/field/field.moduleundefined
@@ -975,13 +975,14 @@ function field_has_data($field) {
+ *   be performed. @see field_info_field().

Per http://drupal.org/node/1354 @see should be at the end of doc blocks. But because it's in the context of this parameter, so maybe just something like: "See field_info_field()." The function name would be linked automatically anyway.

+++ b/core/modules/field/field.moduleundefined
@@ -975,13 +975,14 @@ function field_has_data($field) {
  *   The type of $entity; e.g., 'node' or 'user'.

For reference: Single quotes here are correct.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Needs work » Needs review
FileSize
1393234-3.field_access_api_docs.patch858 bytes

Thanks for the review.

The "view" etc are already in there, not changed by my patch. But sure, we can fix that while we're at it.

I suspected the inline @see was going to be a "problem", but fine, changed it to See field_info_field().

Let's finish bikeshedding the D8 version before we bother with the D7 backport.

Cheers,
-Derek

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Status: Needs work » Needs review
Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Status: Needs review » Needs work

Whoops, then I've also been reviewing the context of that hunk. But yes, why not fix it. Very good. When we're doing that, we could just as well add a newline between the @param's and the @return. After that, this should be good to go.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
FileSize
1393234-6.field_access_api_docs.patch1.03 KB

Newline added.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Status: Needs review » Reviewed & tested by the community

Very good.

xjm’s picture

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

Actually, while we're changing it, I'm fairly sure the array keys are not supposed to be quoted at all. See:
http://drupal.org/node/1354#lists
#1333534: Further cleanup for documentation in core/includes files starting with A-G

xjm’s picture

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

And actually I'm dumb because these are values, not keys. Disregard #8. :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/field/field.moduleundefined
@@ -975,18 +975,20 @@ function field_has_data($field) {
+ *   The full field structure array for the field on which the operation is to

Why not use @see?

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented

dww had a @see that, but I said he shouldn't. When reviewing that I looked at http://drupal.org/node/1354. It says, that there should be no inline @see's. If we put the @see at the end of the docblock (as recommended) it doesn't have the effect we want. So ... if we can use an inline @see there, we should, but ...

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Reviewed & tested by the community

OK that makes sense, if we need it in context we shouldn't use @see, back to RTBC :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.x.

This probably needs backport?

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Status: Patch (to be ported) » Needs review
Issue tags: +Needs backport to D7
FileSize
1393234-field-access-docs.patch1.01 KB

Simple backport.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Hum, realize I'm late to the party on this, but would it be better to say why we need to see field_info_field() explicitly?

xjm’s picture

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

Eh, it's clear. :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

I don't think we put data types in parameters in D7.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Status: Needs work » Needs review
FileSize
1393234-field-access-docs-2.patch894 bytes

Oh ...

xjm’s picture

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

Right, forgot about that, sorry. #18 looks fine.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

Re: #17-19: grep says otherwise. There are ~60 files in core that put something between @param and the $name (total ~350 times). I just thought that improvement was incomplete and on-going. I prefer #14 (equivalent to #6 that's in D8). But if there's some conscious policy to eradicate this from D7 I'd love to know the more appropriate issue for me to oppose such a policy. ;)

Either #14 or #18 are better than what we have now, and RTBC. I'll let webchick decide. ;)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Well, we decided in the original issue to add the param docs in d7 d8 only. See http://drupal.org/node/1354#functions. There's also a longer explanation in the original issue.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

From the linked page on documentation standards:

Data types on param/return
Note: This section of the standard was adopted as of Drupal 8. In previous versions, data type specification was recommended in the case of non-obvious types or specific classes/interfaces only.

I'd say this would be a case of "non-obvious types", and therefore, worthy of including the types. Honestly, I really don't see the harm in providing this info if we've got it. But, I don't know what "the original issue" is, so I can't read the longer explanation.

Anyway, either #14 or #18 are an improvement. I just think #14 is more of an improvement. ;)

Cheers,
-Derek

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

@dww: Sorry, thought I'd linked it. See webchick's post at: #1296842-29: Clarify scope of PHP coding standards for core (major version) and contrib code.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed #18 to 7.x. Thanks!

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