[D7] Webform References

Hi Sumit,
Awesome module. Thanks for your contribution!
Added updated issue summary link.
Updated git access url.Please make sure to add the public accessible git repo url so that other users can access it .i.e. not include your name in the git repo url.

Will update the manual review of this module soon.

As I am not a git administrator, so I would recommend you, should review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks!
Naveen Valecha

Hi,

Nice work.
Please fix the issues provided by the PareView.sh(Automated review).
Those are very minor issues in your module and take a jiffy to fix them.
Have look to the attached file for more information.

Good Luck.

Sanat

Hey Sumit,

Good work, I dont think the issues are still fixed from Automated review, as it says " All functions should be prefixed with your module/theme name to avoid name clashes" so your function names from webform_references_node.inc would be usually like webform_references_node_reference etc. also would suggest renaming webform_references_node.inc to webform_references.node.inc

Rahul

Hi Thanks for contribution,

below is the feedback of manual review:

/**
 * Implements _webform_theme_component().
 */
function _webform_theme_node_reference() {
  return array('webform_display_node_reference_field' => array('render element' => 'element'));
}

Code comments does not match with function name, you can use "implements" in case of implementing of hook only, this function doesn't seem hook.

Hi Sumit,

If it is hook must be prefixed with module name. I would suggest to treat this as a handler and mark comments accordingly. Please see automated review also.

http://pareview.sh/pareview/httpgitdrupalorgsandboxsumitmadan2332637git

Thanks,
Rashid

I don't think we should blame Sumit for following the naming conventions of the Webform module. It might be strange choice to work with hooks that have a different structure of names than we are used to in Drupal but that is a decision that has been made earlier by the makers of the Webform module.

For me it is ok to ignore the warnings of the automated review about these function names. The rest of the code looks ok and there are comments that explain what each piece of code does.

As for the working of the module it is easy to install and use. The only thing that bugs me is what is already described as a 'known issue': if you disable the module while your have reference components in use it generates notices on several pages of the webforms that use such a component. Is there any way to prevent this or can we at least warn the site builder about the component being in use when he or she wants to disable the module?

Manual Reviews:
I found one issue.
Please use t() function in $info['description].

function webform_references_system_info_alter(&$info, $file, $type) {
  if ($type == 'module' && $file->name == 'webform_references') {
    $result = db_select('webform_component', 'wc')
      ->fields('wc', array('name'))
      ->condition('wc.type', 'node_reference')
      ->execute()->fetchAll();
    if (!empty($result)) {
      $info['description'] = "Defines a webform component type for referencing the nodes. <span class='error'>(Warning: Don't disable/uninstall the module because the node_reference type component is already in use.)</span>";
    }
  }
}

Hi Sumit,

This seems like an interesting extension to Webform. I would like to know if it is possible to reference to entities, instead of nodes (or is this future work?). My general remarks:

1. When no node (references) are available, the form element is still shown. This is especially frustrating when the component is made required (i.e., you cannot submit the form).
2. The wrapper CSS classes is not applied. In the _webform_render_node_reference() you should add to the associative array $form_item '#theme_wrappers' => array('webform_element').
3. When the title display is set to inline, the form element disappears. In the _webform_render_node_reference() you should add to the associative array $form_item '#theme_wrappers' => array('webform_element').
4. Unpublished nodes are available in the select element, but not in the autocomplete element.

Below you can find (the rest of) my review using the Project Application Review Template.

Let me know if you need some clarification on my feedback.

Automated Review

function webform_validate_node_reference($element, $form_state) {

I understand the naming convention imposed by Webform, however, I think that this function should be prefixed with your module name. If not, could you explain why?

Manual Review

Individual user account

Follows the guidelines for individual user accounts.

No duplication

Does not cause module duplication and/or fragmentation. However, the functionality of your module has (some) similarity with the tutorial: Providing dynamic select options to Webform. I think it is good idea to post a comment explaining and linking to your project.

Master Branch

Follows the guidelines for master branch.

Licensing

Follows the licensing requirements.

3rd party assets/code

Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Does not follow the guidelines for in-project documentation and/or the README Template completely.

1. I encourage you to use the prefered format that is explained on the bottom README Template (e.g., "Headings underlined with ===/--- to the length of the heading, followed by a newline").
2. Extend the summary (synopsis on project page) with a typical example that this module could solve for the user.
3. Explain that the 'Node Reference' component is created by your module.
4. List the dependencies of Webform: ctools and Views (including links to project pages).
5. Since your module is for Drupal 7, I think you should use this link: https://www.drupal.org/documentation/install/modules-themes/modules-7 in the installation section.
6. Add a configuration section, explaining how to configure the webform component.

Code long/complex enough for review

Follows the guidelines for project length and complexity.

Secure code

Meets the security requirements. However, see some issues below.

Coding style & Drupal API usage

1. I recommend to apply the API documentation and comment standards (for hook_menu() page router callback functions, Themeable functions, ...). Extending the documentation with @param, @return and @see would be helpful to understand your functions more easily.
2. If I understand it correctly, you are using local memory to store a potential references in webform_references_potential_references(). I don't know if it is useful, but you may want to integrate a second layer/level cache by using Drupal's cache_set() and cache_get() (for more information see A Beginner's Guide to Caching Data in Drupal 7).
3. * check_plain() should be used when outputting/using user-provided information, instead of applying it in _webform_references_potential_references_standard() you should use it in functions such as webform_references_autocomplete().
4. + Applying check_plain() (through array_map()) is not needed in _webform_edit_node_reference(), since #default_value element and #options element (in Drupal 7) of Form API (FAPI) is always sanitized.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Hello Sumit,

Nice to see that my feedback was useful, however, some changes you made introduced (minor) issues according to PAReview.

I understand that Webform introduces 'conflicts' with Drupal's naming convention. However, the

function webform_validate_node_reference($element, $form_state) {

should be

function webform_references_validate_node_reference($element, $form_state) {.

I've just tested locally by changing the function name and everything still works as expected. Please let me know if I'm wrong! As far as I understand this validation callback function is (currently) only attached to your form items and not called by other modules (i.e., Webform itself).

Kind regards,

Niels

Ok! Marking this project RTBC.

Assigning to myself for next full review.

Automated Review

Review of the 7.x-1.x branch (commit f5f4186):

• ./webform_references.node.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming
  

  function _webform_defaults_node_reference() {
  function _webform_edit_node_reference(array $component) {
  function _webform_render_node_reference(array $component, array $value = NULL, $filter = TRUE) {
  function _webform_submit_node_reference(array $component, $value) {
  function _webform_theme_node_reference() {
  function _webform_display_node_reference(array $component, array $value, $format = 'html') {
  function _webform_csv_headers_node_reference(array $component, array $export_options) {
  function _webform_csv_data_node_reference(array $component, array $export_options, array $value) {
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

(*) Without seeing the context of where/how _webform_references_get_node_list() is being called, this needs a ->addTag('node_access'), otherwise
there is a node access bypass problem. Also webform_references_validate_node_reference(), and possibly _webform_references_get_node_title(). In general, all
db_select() against {node} should have this tag unless you have already established that the current user has access to the node. Even revealing the title of the
node would be considered a security issue.

Coding style & Drupal API usage

Will this work with both Webform 3 and 4? If not, specify a version in the .info dependency.

Use NODE_PUBLISHED instead of the actual constant value.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Manual Review

Review of the 7.x-1.x branch (commit a6c88dcb):

Looked at changes. Since appears _webform_references_get_node_title() after a reference has been established, and is an
internal helper function, the tag isn't needed here.

(*) Sorry I missed this, but you have one more security problem. theme_webform_display_node_reference_field() plains
the $element['#value'], and passes it to _webform_references_get_node_title(), which uses it as the $nid in the query. When you don't
use the link option, the title is not plained. See https://www.drupal.org/node/28984. When you make a link, everything
is secure because l() does a check_plain() on the title internally.

I read `git diff a6c88dcb..HEAD` and my blocking issue has been addressed. Assigning to @heddn for a second look, if he has time. Otherwise, I will give this a few days at RTBC for any objections.

About a week at RTBC w/o objections, so...

Thanks for your contribution, sumitmadan!

I updated your account so you can 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 stay 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.