[D7] Meet Our Team

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then 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.

Hi,

please clear basic errors before you push your project which is shown in pareview result ::

Git errors:

Git default branch is not set, see the documentation on setting a default branch.
Review of the 7.x-1.x branch (commit 5467542):

ESLint has found some issues with your code (please check the JavaScript coding standards).

1. /var/www/drupal-7-pareview/pareview_temp/js/meet-our-team.js: line 22, col 10, Error - 'popupinfo' is defined but never used (no-unused-vars)

Permission Denied ::: Unable to take clone.

Hi ,

Reviewed your code manually here is some suggestion for you.
1. Please remove PHP logic from "views-view-fields--meet-our-team.tpl.php" file and preprocess function for it.
2. Add your JavaScript code inside Drupal.behaviors (Optional).
3. Use alise $ for Jquery(optional).

Thanks

HI Madan,

Automated Review:
http://pareview.sh/pareview/httpgitdrupalorgsandboxclariontechnologies26...

Please fix the issues in the pareview.

Manual Review:

I reviewed your module, no any issue found in manual code review

Thanks
Nitin

Minor coding standard issues are surely not application blockers, please do a real manual review.

Then its ok.

This issue is not fixed? See the workflow: https://www.drupal.org/node/532400

Hi madan879,

in your hook_uninstall() you are deleting a view. If this view is in default state, you don't need to delete it. If it is overwritten by the site builder you might delete his/her work since you don't know how many changes he/she made to this view or if this view was already there before the user enabled your module. I think this is not a good idea.

You have many variables so you should consider to use one of the methods suggested by Proper way to bulk-delete variables on module uninstall to be sure to delete all your variables.

Best regards

Hi madan879,

have you also read the comments on Proper way to bulk-delete variables on module uninstall? You should not delete variables directly from db.

I would suggest something like this

global $conf;
$module_string = "meet_our_team_";
$vars = array_intersect_key($conf, array_flip(preg_grep("/^{$module_string}.*/", array_keys($conf))));
foreach (array_keys($vars) as $var) {
  variable_del($var);
}

Best regards

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/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

[Yes: Meets the security requirements.]

Coding style & Drupal API usage

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.

1. I'm fairly sure that check functions were used when appropriate. At the very least, I don't see any part of the code where user-supplied strings were used (user being a visitor to the page). Could I get a confirmation for this?
2. In meet_our_team.module, the function meet_our_team_node_type_update() seems to do the same thing for each of the if conditional. It would be easier to read if the common parts were broken out into its own function.
3. The word "organization" is misspelled in your README and your project page.

This review uses the Project Application Review Template.

Overall, I don't see any major issues.

Hi @madan879,

There is a security vulnerability in your module.
Instead of print the value, use Drupal's render function in template to display fields/user entered data in template files.

There is no need to override the template for node page as you are using the default node template name format, so hook_theme can be removed.
In hook_node_type_update() function, same code is being repeated for social information, it can be moved to a separate function and change data as per parameters.
Write proper comment for meet_our_team_preprocess_views_view_fields__meet_our_team() function.

Changing the status back to "Needs work". you can change the status back to "Needs review" once the issue is fixed.

/meet-our-team is vulnerable to XSS exploits. If I enter <script>alert('XSS title');</script> as node title then I will get a nasty javascript popup on this page. You need to sanitize user provided text before printing, make sure to read https://www.drupal.org/node/28984 again.

In this case you must use $output instead of $row in views-view-field--meet-our-team--title.tpl.php or sanitize instead. The docs even say so: "$row: The raw SQL result that can be used", which is not XSS safe for printing to HTML.

Pareview.sh test still finds stuff:

Git errors:

Git default branch is not set, see the documentation on setting a default branch.
Review of the 7.x-1.x branch (commit 9d16193):

ESLint has found some issues with your code (please check the JavaScript coding standards).

/var/www/drupal-7-pareview/pareview_temp/js/meet-our-team.js: line 26, col 10, Error - 'popupinfo' is defined but never used (no-unused-vars)
1 problem

From manual check I found:

• Install file contains database queries that do not use dynamic queries, see https://www.drupal.org/dynamic-queries.
• Still found PHP logic in your template files, for instance views-view-fields--meet-our-team.tpl.php, lines 28 to 39, would advice you to check on this thoroughly.

Nice concept + comments and Readme.txt look fine, helping out the user in the end.

Hey madan879,

Still there is some PHP logic found in template file, Is there is now way to add this logic to preprocess function and render variable to .tpl file only.

Thanks,

Hi @madan879

Ran code sniffer: And found these small indentation issue which are quite important according to Drupal coding practices:

FILE: .../meet_our_team/themes/node--meet_our_team.tpl.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
113 | ERROR | [x] Spaces must be used to indent lines; tabs are not
| | allowed
118 | ERROR | [x] Spaces must be used to indent lines; tabs are not
| | allowed
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: .../meet_our_team/themes/views-view-fields--meet-our-team.tpl.php
----------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
----------------------------------------------------------------------
39 | ERROR | [x] Spaces must be used to indent lines; tabs are not
| | allowed
46 | ERROR | [x] Spaces must be used for alignment; tabs are not
| | allowed
49 | ERROR | [x] Spaces must be used for alignment; tabs are not
| | allowed
55 | ERROR | [x] Spaces must be used for alignment; tabs are not
| | allowed
95 | ERROR | [x] Spaces must be used for alignment; tabs are not
| | allowed
----------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Other than this I could not find any issue in this. Looks fine.

@lhuria94: I think you forgot to change the status. Is this now RTBC after your review or are there any application blockers left?

@klausi - Sorry, I forgot to do that. Some minor fixes, No Application blockers. This is RTBC according to my review.
@madan879 - Thanks for making those changes.

Git errors:

• Git default branch is not set, see the documentation on setting a default branch.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: ...klausi/pareview_temp/themes/views-view-fields--meet-our-team.tpl.php
  --------------------------------------------------------------------------
  FOUND 3 ERRORS AFFECTING 3 LINES
  --------------------------------------------------------------------------
   46 | ERROR | [x] Closing brace indented incorrectly; expected 6 spaces,
      |       |     found 5
   67 | ERROR | [x] Closing brace indented incorrectly; expected 5 spaces,
      |       |     found 6
   89 | ERROR | [x] Closing brace indented incorrectly; expected 8 spaces,
      |       |     found 9
  --------------------------------------------------------------------------
  PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------
  

• 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:

1. project page is a bit short, see https://www.drupal.org/node/997024
2. meet_our_team_node_type_insert(): why are you creating the fields in a node insert hook? Shouldn't that be done in when the admin form is saved? Why are you doing it again here?
3. meet_our_team_preprocess_views_view_fields__meet_our_team(): do not assemble URL paths yourself, use url() instead. Also elsewhere. And in node--meet_our_team.tpl.php.
4. views-view-fields--meet-our-team.tpl.php: usually check_plain() should not be used in a template. It should be done when you prepare the variables or pass them to theme(), for example in meet_our_team_preprocess_views_view_fields__meet_our_team(). That way if a theme overrides the template they cannot forget to run check_plain() and you avoid potential security issues in custom themes.
5. views-view-field--meet-our-team--title.tpl.php: same here: check_plain() usage, why can't you use $output? Please add a comment.
6. node--meet_our_team.tpl.php: "Back to team": all user facing text must run through t() for translation.
7. Notice: Undefined index: und in include() (line 85 of themes/node--meet_our_team.tpl.php). I think this happens when there is no image for the team member. Make sure to enable the display of PHP notices in your dev environment.

Although you should definitely fix those issues they are not critical application blockers.

Thanks for your contribution, Madan!

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.

Set the Default git branch:-
Edit your project
Click the "Default branch" tab
Select the desired branch
Click Save

In meet-our-team.js popupinfo is defined but never used
(no unused vars)

Hm, because the sandbox does not belong to your account it seems you cannot promote it.

Did that for you: https://www.drupal.org/project/meet_our_team

Now that this experimental project has been promoted, you'll need to update the URL of your remote repository or reclone it.

The git instructions are here: https://www.drupal.org/project/meet_our_team/git-instructions

Ah, you need to use the project short name in the git URL. Filed #2726599: Sandbox repositories are occasionally not fully promoted & Git events are sometimes not recorded.