[D7] FVL (Field Value Loader)

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 put yourself on the PAReview: review bonus high priority list. Then I'll take a look at your project right away :-)

Hi Dalay,

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

I have installed your module and everything seems to work but at first I was missing some simple usage instructions (in english). It wasn't very clear to me what I should do to get it working until I reviewed your code.

Your Demo page is having some trouble to find jQuery "Uncaught ReferenceError: jQuery is not defined".

Gr,
Samuel

Review comments:

fvl_get_field_value() does not account for node revisions, so viewing past revisions won't work with this module.

Add a configuration entry in fvl.info so users get a config link on the module page.

fvl.module:39-45 - this is unnecessary, remove all of this and replace line 47 with:

  $node->content[$field] = array(
    0 => array(
      ...
    ),
  );

which will replace all deltas anyway.

fvl.module:124 - split this line into if(user_access(...)) {...} for readability.

@Dalay Thanks for responding quickly, and apologies if I wasn't being clear.

I don't understand you. fvl_get_field_value() returns field value from current revision, only thing that we need.

My point here is that on the past revisions page for a node, e.g. http://mysite.com/node/1/revisions/1/view the user should see the field values corresponding to the selected revision. Instead, when they click 'show', they see the current field value.

I think you should either allow for previous field revisions to be displayed (since this is core functionality), or at the very least document it as a known issue on your project page and README.txt.

So do not. Otherwise, we lose all the other useful properties of the field (such as title, label_display...).

That's a very good point! Please ignore my comment.

Since you've gone to the trouble of writing a documentation page and creating a demo, I would suggest you add these to your sandbox page. You can do this by editing the page and adding them under the 'Project resources' tab.

move function fvl_admin_settings() from fvl.module to fvl.admin.inc

this has been a standard way now.:)

Manual review:

Interesting idea.
Everything works fine. But I noticed one moment with displays. Module always uses default display of field even if I changed it in settings.
See my screencast http://screencast.com/t/Qogr800spr
It's because of display parameter in

$output = field_view_field('node', $node, $field, array('label' => 'hidden'));

makes sence to add 1 more parameter into fvl_get_field_value function like $display.

Hi Dalay,

you've built a very interessting module. I like the idea.

i tested your module and it didn't work for me.

In your file fvl.module on line 98 there is:
$display = $instance['display'][$view_mode];

In $instance['display'] i have 2 view modes: default and teaser.
I am on my node so my view mode should be 'default'.
Now when i print out the variable $view_mode i get the string 'full'.
Yes my default view mode is the full version.

You have to look up if $view_mode ('full' in my example) is default.

Hi dalay,

Thanks for bring to the valuable module to the our(drupal) community.

Module is working fine without any issues.Could you please add some user friendly info for which was highlighted in the attached image.

Sorry for my poor english ( I am not native speaker)

Thanks,
-Raj.

Hi Dalay,

Thanks for taking my feedback.

In the permission description, you gave like "Permission to view data from field body".

As per your module, we have an option control the view of the fields other than title of any content type right?

Could you please give easily understandable permission description message.(Permission to view the fields(Except title) for any content type like page or article)

Thanks,
-Raj.

manual review:

1. Looks like your module only works with nodes and not other entity types, why? Please add that limitation to project page and README.txt.
2. fvl_node_view(): it does not make sense to display the show link at all if the user does not have access to the field?
3. fvl_node_view(): shouldn't the link have a nofollow attribute for search engines to indicate that they should not follow the link?
4. I would rather implement this functionality with a custom field formatter. That way you could make it work for any entity type and it would be independent of nodes.
5. fvl_get_field_value(): this is vulnerable to access bypass attacks. Scenario: I have a node with a body field that is FVL enabled. The permission to FVL view the body field is granted to the anonymous user. The node gets unpublished or protected by other node access modules, anonymous users cannot access the node page anymore. The path /fvl/1/1/body/full/ajax/ is still accessible to anonymous users, thereby allowing them to bypass the access control and retrieve the field body value of an otherwise access protected node. This is a security blocker. You must use node access checks (or entity_access() checks from Entity API if you expand this to more than nodes). And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

PAReview.sh comes back clean, code appears properly formatted, and module does what developer says it is supposed to do. Changing status to reviewed & tested.

Thanks for your contribution, Dalay!

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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

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.