[D7] Form default button

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

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.

Kindly pin-down all items from here:
http://pareview.sh/pareview/httpgitdrupalorgsandboxmiroslavbanov2290641git
1. Please remove the master branch but making sure you have the correct branch setup first take a look https://www.drupal.org/node/1127732
2. README.txt https://www.drupal.org/node/447604 and follow this README.txt standard template
3. Please take some time to make your project page follow tips for a great project page.

cheers,
-Albert

The changes are looking good.
1. Cleaned PAReview
2. Checked
3. README.txt could have TOC and Maintainer section but all good.
4. Project page has the image now and looking good.

Currently you have 1 Project review: Review Bonus, just reminding you that adding another two will speed-up further reviews and hopefully to full project soon.

Goodluck Mate!

Hi MiroslavBanov,

First of all thank you for your contribution.

Automated Review

No issues found

Manual Review

A description of your module for my own understanding: Your module adds a 'fake' button to the beginning of the form that will be triggered upon 'enter' (default browser behaviour). If javascript is disabled it will just reload the page, since the browser by default will trigger a click on the first button it finds, which is the newly added but hidden button. If javascript is enabled it will either ignore the 'enter' click with a preventDefault() or trigger another button that was set in the modules UI page.

1. You choose to make a 'progressively enhanced' solution that doesn't require javascript for disarming the default 'enter' on forms. You do this by adding a hidden button to the form. If you want to trigger an alternative button on the form you do need Javascript. Technically your module is partly able to work without javascript. If you accept the requirement of javascript for your module, your module could be reduced to half the size and hidden elements wouldn't be necessary. A simple preventDefault() would suffice. I think the virtue of writing a 'progressive enhancement' solution that (only partly) works without javascript is somewhat canceled out by the complexity of your solution. I therefore would stick to a javascript solution to disarm the enter button, since it is a UI functionality and wouldn't affect the inner workings even if javascript was disabled. Would like to hear your opinion.
2. **You change the forms by calling hook_form_alter. The hidden button is generated by a new object defined in FormDefaultButtonElement.php. As much as I like OOP, I think this is a bit of an overkill for this purpose since your object is not actually doing anything that can't be done within the same hook_form_alter function. I think it would be better just to add the hidden button within the hook_form_alter, above all for readability and sticking to the 'drupal way'.
3. I don't think formDefaultButtonElement.php should be loaded on every page, only on forms that need it. Better would be to put it in an .inc file, remove it from the .info file and call it when needed using module_load_include.
4. In FormDefaultButtonElement.php you use a form array named $warapper. Does this stand for anything? Better to use $build or $form.
5. You can enter any type of PHP as a form_id. I must admit that my knowledge is limited about how and where to apply sanitize functions to avoid people from entering php and possibily execute malicious code, but I feel like you need some security measure. Maybe some other reviewer can help you (and me) with that.
6. Would like to see some information about how the module works and what it does 'technically' on your project page and in the README.txt

** Important issue

Hi MiroslabBonav,

Thanks for your quick reply.

1. i tried your module with JavaScript disabled and it did trigger the first button on enter, which just reloads the page. But about a full JavaScript solution,i haven't looked at it in detail but a quick browse led me to this forum topic http://stackoverflow.com/questions/895171/prevent-users-from-submitting-...
2. ok, still not entirely convinced about the use of that class but like hear other reviewers opinions.maybe I'm simply mistaken
3. see 3
4. in any case it should be wrapper, not wawrapper (see extra a)

Again thanks for your feedback, good luck!

Works correctly on my site. Pareview.sh reports no problems and I see no conflicts with the checklist: https://www.drupal.org/node/1587704. I'm switching status to "Reviewed & tested by the community" but I do have some comments for consideration:

1) The install file pre-populates the form_ids of existing node types when the module is installed. But it left me a bit confused because I saw some of the expected form_ids but not all. Forms that are not for node types are missing. This was overcome by the box that let me add the form id I wanted. But I had to search to find the correct form_id. Perhaps the list of forms on the configure page should be empty at first and the site admin can add them as needed. Or, perhaps there should be instructions that indicate that only forms for node types are automatically added. Also, to help the user, perhaps you could add the form_id as text at the bottom of the form if the user has a given permission. This way the user doesn't have to dig around to find the form_id and it wouldn't show up for other users looking at the form.

2) I like a bit more inline documentation, especially if there are control structures (if, while, etc.) Just one liner's explaining what is going on so I don't have to study the code so closely.

3) In regards to the discussion on whether to use a class.... I do agree that it is probably not necessary since it is really only used by one function and the code from it could be placed in that function. But that's just my preference.

Automated Review

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

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

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: Does not cause module duplication and 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

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

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

(*) form_default_button_settings() is XSS prone. Enter <script>alert('XSS')</script> as the form ID, and click save. When the page refreshes you will get the alert.

Coding style & Drupal API usage

The config path should be in the .info file.

A hook_help() would be nice.

Do you need to module_implements_alter to make sure your form alter runs last?

The labels in $button_default_action_options() need to be translated. Remove the commented out option for release.

For form.form-default-button-wrapper, why can't you just use the standard .element-invisible class? If not, comment needed.

The form_default_button_form_alter() is a simple, static query. You can use db_query() w/ a placeholder here. Also elsewhere.

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.

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

manual review:
form_default_button_settings_submit(): check_plain() is wrong here: "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." from https://www.drupal.org/node/28984 . Instead, you should use check_plain() when you display the form_id. Even better in your case: create a validation callback to make sure that a form ID can only have alphanumeric characters and underscores, see https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...

Setting this back to needs work because it is important that you understand where to use sanitisation functions and where not. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: I found some snippets than implements similar funcionality to include on the template but no module doing it for all forms.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: I think it follows the licensing requirements

3rd party code

Yes: I didn't found any 3rd party code so it's ok.

README.txt

Yes: It's not exacly the template, but it's very similar and clear.

Code long/complex enough for review

Yes: All code files (all but README.txt) has 438 lines of code in addition.

Secure code

Yes. : I checked al SQL querys and all of then use de database API. Didn't found any cross site, scripting possibility.

In addition I must say that the module works fine.

Assigning to myself for next review, which will likely be tomorrow morning.

Automated Review

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

• 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: Does not cause module duplication and 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 the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes.

Coding style & Drupal API usage

I think your JS behavior may need to use the context that is passed in and/or the .once() functionality to prevent it from running more than once when AJAX events happen.

Still think a hook_help() would be nice.

Evelement level validation would be better for the form_id, rather than a global validator.

I don't really like your FormDefaultButtonElement() class. It doesn't really do much on its own without the method parameters that get passed in. It almost seems like the ClassesWithoutOo antipattern.

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.

Blocking issues from my previous review have been addressed, and blocking issue from #18 has been addressed via validating the form_id (and improper check_plain has been removed). Explicitly tested both the form_id textfield and configuration textfields for XSS.

Not seeing anything to prevent RBTC. Assinging to @heddn for a second look.

1. Nit: Usually array elements are on their own line if they line wrap.
   

   function form_default_button_install() {
     $form_ids = array(
       'contact_site_form', 'contact_personal_form',
       'user_register_form', 'user_pass', 'user_login', 'user_login_block',
     );

2. Nit: No need to wrap the variable {$form_html_id} in brackets.
   

   function _form_default_button_setting($form_html_id, $button_html_id, $form_settings) {
   ...
     return array(
       "{$form_html_id}" => array(
         'action' => $form_settings->default_action,
         'target' => isset($configuration['click_selector']) ? $configuration['click_selector'] : '',
         'button_id' => $button_html_id,
       ),
     );

3. Nit: spelling in comment: Attachemnts
   

   function _form_default_button_element($form_html_id, $form_settings) {
   ...
     // Attachemnts.

4. (*) Theme implementations should be placed in either the .module file or a 'file' directive given them. Currently this only works because the only place that the module uses the theme function is also in the admin.inc. If it were used anywhere else, it would silently fail and makes for a very difficult troubleshooting experience. Marking as Needs Work since theme functions are a Drupal API concept that should be thoroughly understood. In fact, after looking at this more closely I'm not really sure if theme_table couldn't be used entirely and the theme callback disappear.
   

   function form_default_button_theme() {
     return array(
       'form_default_button_forms_table' => array(
         'render element' => 'form',
         'file' => 'form_default_button.admin.inc',
       ),
     );
   }
   

Plenty of custom template-reviews in this issue so I won't repeat that. These are some random thoughts and feedback, none of which are show-stoppers, mostly nitpicks. Not like the security issue mpdonadio found in #16, which is a rather important one to "get".

• The module works as advertised and I like the idea. But I'm torn about the implementation. An additional "hidden" button? It sounds hack-ish, though many solutions to make things work across browsers are. I'm still wondering if this shouldn't be solved entirely in js, or using the States API in some clever way.
• I miss some comparison with other modules or discussions around this problem. A quick search gives me some ideas revolving around a few lines of JS that hinders default behavior of input elements on enter keypress (event.keyCode == 13).
• Is there a real need for an additional permission for this? Would it not suffice to use one of the existing permissions from system.module, say administer site configuration? I for one think the permissions page is cluttered enough.
• Code looks well structured with reasonable .inc files, but is there really need for the form_default_button.theme.inc? Isn't it enough to include the theme within the form_default_button.admin.inc file? It's not like anyone ever will override this theme, realistically, so if the .admin.inc is included, so will the .theme.inc.
• In form_default_button.admin.inc:22:
  

    $button_conf_all = db_query('SELECT * FROM {form_default_buttons} WHERE 1 ORDER BY form_id ASC');
  

  Is the WHERE 1 necessary? Am I missing something here?

• In form_default_button.admin.inc:26:
  

      $form['forms']["existing-$button_conf->form_id"]['form_id_value'] = array(
  

  "existing-$button_conf->form_id" doesn't read well, and is repeated several times down the form. I'd extract this to a separate var before to improve readability:

      $existing_form = 'existing-' . $button_conf->form_id;
      $form['forms'][$existing_form]['form_id_value'] = array(
  

@MiroslavBanov: My only close to show-stopping issue was the solution's architecture, and your points are valid. I've used the few lines of javascript myself on an other project, but will consider this solution for later. The rest is just nitpicks or conflicting with other reviews. Seems fine by me, so marking as rtbc.

Come to think about it. The exact solution of the problem should be documented and your points about conflicting event handling should be addressed on the project homepage and README.txt. This makes this module more interesting for site builders to investigate upon.

Some clarification about the theme comment in #2290647-27: [D7] Form default button. The theme definition could have stayed in the admin.inc (although a little strange). The important thing was to list the 'file' in the theme definition.

@kaare, your questions about use #2290647-29: [D7] Form default button and follow-up in #2290647-32: [D7] Form default button did a good job of describing what I was mulling over in my head. The addition of the use case for why to use this module are extremely helpful and make this module way more compelling. Updates to the README and project page are indeed a good thing.

However, seeing no further blockers...
Thanks for your contribution, MiroslavBanov !

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.

Let's keep this as fixed, it will auto-close in 2 weeks.