[D7] Webform Matrix Component

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxcpsinghal1904116git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Manual Review:

1. Title Modified.
2. Use t() function in 'webform_matrix_component.api.php' file at line number 48.
3. Use t() function in 'matrix.inc' file at line number 154, 164, and similar other lines.

Hi @cpsinghal:

Review of the 7.x-2.x branch:

• Remove all backup files from your repository:
  

  ./webform_matrix_component.api.php~
  

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

  function _webform_defaults_matrix() {
  function _webform_theme_matrix() {
  function _webform_edit_matrix($component) {
  function _webform_render_matrix($component, $value = NULL, $filter = TRUE) {
  function _webform_submit_matrix($component, $value) {
  function _webform_display_matrix($component, $value, $format = 'html') {
  function _webform_analysis_matrix($component, $sids = array()) {
  function _webform_table_matrix($component, $value) {
  function _webform_csv_headers_matrix($component, $export_options) {
  function _webform_csv_data_matrix($component, $export_options, $value) {
  

Hi cpsinghal,
I tested your module. It seems to work fine, however I have a problem with your component position in the form.
Please see file attached. As you can see, "cognome" (=lastname) is shown after field "matrix", but "matrix" has weight 2 and "cognome" has weight 1, so the order seems to be incorrect...
You should fix this, if this depends on your module.

Cheers

manual review of the 7.x-2.x branch:

1. _webform_render_matrix(): this is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as label name for a label element I get a nasty javascript popup when rendering the form. You need to sanitize user provided input before printing into #markup. The #title is safe as that seems to be filtered in theme_form_element_label(). Please read http://drupal.org/node/28984 again.
2. Another exploit when I enter script tags as user in text fields. When an admin views the submission page she will get XSSed. Not sure where that is caused, maybe in _webform_analysis_matrix() or _webform_table_matrix()?
3. "form_error($element, t('!name field is required.', array('!name' => check_plain($element['#title']))));": use the "@" placeholder with t() then you don't need the extra check_plain() call, see the documentation of t().
4. _webform_matrix_component_textfield_Numeric(): why do you need that validator? Is there nothing like that in webform core? Please add a comment.

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

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

Hello cpsinghal,

A few comments:

1) Function names should be lower case. See lines 20 and 33 in webform_matrix_component.api.php. I don't see any calls to these functions, are they necessary? Perhaps they're hooked into from somewhere else that I'm missing. In the case of webform_render_matrix_textfield_Alphabet, it also has no code.

2) This module is still vulnerable to XSS. If I add

Review of the 7.x-2.x branch:

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/webform_matrix_component.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   5 | WARNING | @author tags are not usually used in Drupal, because over time
     |         | multiple contributors will touch the code anyway
  --------------------------------------------------------------------------------
  

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:

1. "'textfield' => 'Textfield',": all user facing text should run through t() for translation.
2. "t('Option keys must be less than 128 characters. The following keys exceed this limit:') . theme('item_list', $long_keys)": do not concatenate dynamic variables into t() strings, use placeholder with t() instead. Also elsewhere.

But otherwise looks RTBC to me now, I could not exploit any XSS anymore. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to patrickd as he might have time to take a final look at this.

Please don't use commit messages like "some changes", these don't help to understand what the actual code changes were.
Instead use the commit message to explain what you have done. It's also good to commit regularly, so you keep your commit messages small.

I recommend to only use module_load_include() when you don't know the exact path because it's not your own module/theme
from: module_load_include('inc', 'webform_matrix_component', '/components/matrix');
to: require_once dirname(__FILE__) . '/components/matrix/webform_matrix_component.inc';

For other developers you code is very hard to read, that's because you have many blocks of code without any inline commenting.
To understand your module you have to explain what you do in a code block and why you did it this certain way.

Beside that the code looks fine and I could not find any critical blockers.

Thanks for your contribution!

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.

Here: http://drupal.org/node/1068944 are the instructions for creating new releases, I recommend following them step by step

Hi,

This is a great module when you need repetitive fields in a form.
I have few queries:
1. I see that 'File' is not a sub component for matrix component, so are there any developments going to have this feature or should I add this in sub component's list programmatically?
2. Once you add a new row in the form, I don't see the button or icon to delete any row if not needed. Do I need to also add this feature through code?
3. Or are there any other contrib modules where I could see these features?

Please help me on these queries.
Thanks!

I have encountered an error in my first use of this module. Any suggestions as to hwo to trecity or get around this issue? I'd really like to have this working in my webforms.

Error: Call to undefined function webform_filter_xss() in .../sites/all/modules/webform_matrix_component/components/matrix.inc on line 393

Thanks in advance