Overview

Drupal's core Field API defines a hook for field access, hook_field_access, that is restrictive as all module developers are aware. In other words, given field F, if a single module implementing hook_field_access returns a Boolean false, thus denying access to the field, Drupal will deny access to the field even if there is another module that absolutely requires access to the field.

Normally, this is acceptable but sometimes a module might want to override the grants for a field denied access by another module. Or, a module might contain logic to grant access to a field but another completely independent module has denied access. Because of the restrictive nature of the field_access function, which internally invokes hook_field_access, access will be denied.

There are numerous scenarios that are similar in nature to hook_file_download_access and hook_file_download_access_alter. For a possible scenario, read on.

Possible scenario:

Assume we have 2 modules, M1 and M2, and a field F. Both modules are configured to operate on F. M1 contains logic to deny access to F from users outside of Canada. M2 contains logic to deny access to F from users without an "Editor" role.

Given a user, U, from England, that has the "Editor" role, M1 would deny access to F - they are not from Canada - and M2 would grant access to F - they have the "Editor" role.

The end result however is that access to F will be denied because a single module, in this case M1, denied access.

The only way to overcome this problem is through a mechanism that allows M2 to override the grants of M1. This can be accomplished through another hook, hook_field_access_alter, which unfortunately is not provided by core. That's where this module comes in - it introduces that much needed hook.

It is worth noting that, following the basic implementation approach for this module - read on for details on that - it is possible to make the field_access function permissive versus restrictive instead of providing an alter hook. But to accommodate additional scenarios - read on for details on that - an alter hook is preferred.

Quick links and information:

Comments

abhijeetkalsi’s picture

abhijeetkalsi commented

Hi,

first of all there are quite a few issues to sort out such as indentation, whitespace.

You can find them all here:
http://ventral.org/pareview/httpgitdrupalorgsandbox9ee11882924git

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors.

9ee1’s picture

9ee1 commented

Thank you for taking the time to review so quickly.

It's weird, I ran the Coder module before I committed the code for the first time and it did not report any of these errors. In any case, most of the errors where whitespace errors in the inline comments, not in the code itself.

I fixed the initial errors you reported but I am still coming up with some weird errors that frankly I have no idea what they mean. For example: "Parameter comment indentation must be 2 additional spaces". I tried 2 spaces and 4 spaces and it still continues to report an error. Not sure how to fix this?

Secondly: "Return comment must be on the next line". Not sure what this means. There is a blank line between the return comment and the last parameter in the inline comments. If I remove the blank line, this error goes away and a completely new error is raised that says return comments must be preceded by a blank line. Which is right?

Also, I am not sure what the problem is with the branch that I am working under. It is not the master branch as reported by the tool. My understanding is that it is not incorrect to commit under a development branch.

Other than that, there does not seem to be any other errors.

sadashiv’s picture

sadashiv commented
Status: Needs review » Needs work

"Parameter comment indentation must be 2 additional spaces".
It should be
* An access operation.
and not
* An access operation.
One space between * and "An" on line 12.

"Return comment must be on the next line" on line 22
Should be
* @return
* A boolean true if the field access is granted. A boolean false otherwise.
and @return should not having any comment following it.

I think you should use module_invoke_all() to invoke hooks rather writing the code, refer module_invoke_all

It would be nice if you write hook_help for your module.

9ee1’s picture

9ee1 commented
Status: Needs work » Needs review

Thank your for your review.

I think your comment on the parameter comment indentation is incorrect. It is not 1 space between the "*" and the first character. It is 3. I looked at other modules and that's the way they do it.

The return value comment should also include the return value's type on the same line. Any additional comments are then started on the next line, also indented by 3 spaces.

I do believe module_invoke_all does not allow parameters to be passed by reference, that is why I chose to do it manually. One of the hooks has a parameter that is passed by reference. I could use it when invoking one of the hooks, but I chose to maintain consistency. If I am wrong about it not supporting pass by reference, please let me know and I will be happy to change it.

I am not sure how a hook_help implementation would be beneficial. This is an API only module. There is no documentation that site developers need to be aware of in order to use it. It will, in most cases, be a dependency for other modules only. Thus, only module developers need to know how it works. Do you agree?

I have also addressed all the outstanding issues reported by the automated review.

stevector’s picture

stevector
he/him
commented
Status: Needs review » Reviewed & tested by the community

On the module_invoke_all() vs. module_implements()/foreach question, I think this module is fine as is. hook_field_access itself uses the module_implements()/foreach approach. http://api.drupal.org/api/drupal/modules%21field%21field.module/function...

I agree that hook_help is unnecessary here since hook_help is entirely path based and this module has no particular path on which it acts or is configured. Though sadashiv might have just been looking for more illustration of what this module does. I suggest a simpletest that tests the functionality here. To my knowledge, simpletest coverage is not a requirement for full project status so RTBC

9ee1’s picture

9ee1 commented

Thanks for your review.

Can you please tell me what I need to do now in order to get the proper permissions to create projects?

stevector’s picture

stevector
he/him
commented

9ee1,

Only certain drupal.org users are able to promote your access. One of them should see the RTBC status on this issue and promote you.

9ee1’s picture

9ee1 commented

Thanks stevector.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

9ee1’s picture

9ee1 commented

@klausi,

I would be more than happy to review other projects. Though I am confused a bit - doesn't the "reviewed & tested by the community" status now mean that this issue has been reviewed and accepted?

jthorson’s picture

jthorson commented

9ee1:

RTBC does mean that it has been reviewed, but still needs final approval from a git administrator. Klausi uses the 'review bonus' program to prioritize which applications he looks at, and is our most active reviewer ... so participating in the review bonus program could help accelerate the reaching of that final approval step.

jthorson’s picture

jthorson commented
Status: Reviewed & tested by the community » Fixed

While there is very little code to go on here, and it does not meet the typical 'code length' standard, this module does demonstrate an in-depth understanding of Drupal's inner workings, which is further evidenced in the explanation on the module's project page. Thus, I'm ignoring that particular requirement for this case.

Thanks for your contribution, 9ee1!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Status: Fixed » Closed (fixed)

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

9ee1’s picture

9ee1 commented

Thank you for your approval. And thank you to everyone involved in earlier steps of the approval process as well.