[D7] formspree

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

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.

Automated Review

Best practice issues identified by pareview.sh

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

[No: Does not follow] the guidelines for in-project documentation and/or the README Template.

Check the README template

Code long/complex enough for review

[Yes: Follows ] the guidelines for project length and complexity.

This review uses the Project Application Review Template.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

Automated Review

There are lots of coding standard issues are there pareview.sh

Manual Review

Individual user account

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

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.

Secure code

[Yes: Meets the security requirements.]

This review uses the Project Application Review Template.

Automated Review
Passed.

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.

This review uses the Project Application Review Template.

@ginorodrigues : Looks like you forgot to change the status. Is this now RTBC after your review or are there application blockers left and this should be "needs work"?

@visabhishek thanks, you are right, I am updating now.

Once enable formspree for a form then it skipping all other form validations other than required field validation. Email validaton is getting failed in contact form.

Same validation error will happen for all forms. Please refer screenshot.

Code too short

This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project for you.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

Hi,

Please create uninstall file to delete your variables.

I need this for a site, would love to see this as a proper project.

Automated Review

pareview.sh looks good. No automated test cases were found, but this is not a requirement.

Note, that I did update the issue summary with the correct git repository URL, since it changed when you promoted your sandbox project to a full project.

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

No: Does not follow the guidelines for project length and complexity.

This has been discussed before and since we are now treating this as an application for a single project promotion, this is not a blocker.

Secure code

No: Does not meet the security requirements. See below for the issue(s) identified.

Coding style & Drupal API usage

1. (*) XSS: You are passing your settings to your JS as is. You don't filter them using any of the methods provided by the Drupal API and you are not validating them after form submission. This is a major security issue! You should really validate the email address and CSS selector(s) before use and even then filter them for XSS just as a precaution. To test, try the following:
   1. Go to your configuration form and insert <IMG SRC=/ onerror="alert(String.fromCharCode(88,83,83))"></img> into the Apply Formspree to the following elements form element.
   2. Save the form.
   3. You should see a popup displaying XSS immediately, because your JS is also added to your configuration form. Why, btw?
   4. If you navigate to any webform, you should see a popup displaying XSS. No need to submit the form first.
2. (+) As previously mentioned, implement hook_uninstall() in a formspree.install file to delete the variables you created upon uninstallation of the module using variable_del().
3. formspree_menu(): Instead of using administer site configuration permission, which is fairly broad, consider to implement hook_permission() to provide your own permission and use that. This would give administrators more fine-grained control over who could configure your module.
4. formspree_preprocess_page(): Consider implementing hook_form_FORM_ID_alter for webform forms and contact forms to only attach your JS when it is actually needed. You could then also use #attached to add your JS and JS settings, which would allow other modules to alter your modifications, if needed.
5. formspree_preprocess_page(): Why are you calling drupal_add_js() for each setting individually? You could add both settings in one call.
6. formspree_preprocess_page(): The DocBlock should read Implements hook_preprocess_HOOK() for page.tpl.php.
7. formspree.js, line 9: This line should not be needed, since the settings parameter should contain the settings already.
8. formspree.js: To make your module work with other modules, that use the AJAX API, you should pass the context parameter to your jQuery() calls.
9. formspree.js: Your validation of required fields works only for contact forms, but it is suggested on the settings page, that you also support webforms. Also, as previously said, there are cases, where your script will submit the form, even though form validation may have failed.
10. formspree.js: At a first glance, your JS looks simple enough to easily port it to Drupals default jQuery version. Consider doing that to get rid of the jQuery Update dependency.
11. formspree.js: A way better approach to implement this functionality, would be to alter the forms you want to support using form alter and add an additional submit handler, that will submit the forms to formspree using drupal_http_request(). Also, you could implement several hooks to extend the webform settings for each webform and then track in a custom SQL table, which forms should be submitted to formspree. That way, it would be much easier to make the module secure, you wont have any issues with form validation, since Drupal would only execute your submit handler, when the form is already validated and you would have much more code to show, so it wouldn't be a one-off promition. And your module would be easy to use for people who don't know anything about CSS selectors. Win win situation in my opinion ;).

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.

Revision reviewed: 468073e

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.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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