barrya [barrya]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

Hi barrya!

Thanks for submitting the module, I think it could be very useful, here is what I found reviewing the code:

• Please remove License.txt file, Drupal has its own licence that modules can't override.
• In the install file you are adding files to the webform table, are you sure that this is the way to go? I would say that managing this in a sepparate table would fit better.
• You've missed the headers of the files for CVS management in drupal ($Id), please take a look to this handbook page: http://drupal.org/node/546
• Make sure you pass your code through coder module to avoid little formatting issues and coding standards http://drupal.org/project/coder
• I think that you could do a general userpoint settings for all webforms in addition of configuring it for each webform node, what do you think?

You should have a seperate DB table for your module and have a "connection" between your table and the 'webform' table.

And then clean up the webform_userpoints_configure_form_submit() and use drupal_write_record instead.

I don't think that those two issues are compulsory for getting the module commited, only a "nice to have", maybe opened as issues when the project is created

There is still a normal warning and a few minor ones when running the module through coder.module, and the schema_alter could indeed be avoided, but apart from that this looks ready to go

What do you say barrya? do you think that using a sepparate table for the module data could be done prior to commiting the version to CVS?

http://drupal.org/node/539608 reports what is considered an application blocker.
Having a separate database table is not one of them, but the module should implement hook_schema_alter().

When a module alters a database table defined from another module, it should also implement hook_schema_alter(), in order to allow the other module to correctly work.

barrya, ok then I think hook_schema_alter belongs to module file and not to install one? https://drupal.org/node/51220, note that you have documented it as hook_schema and not hook_schema_alter.

I guess that if the schema modification is done just one time, like in this case, it must be in example.install

But if you modify database tables dinamically, like CCK, it would be in example.module

Thanks juankvillegas, do you know if this is documented somewhere?
In that case, hook_schema_alter should be documented properly and we'll ready to go :)

drupal_alter('schema', $schema) is normally called inside drupal_get_schema(), after module_load_all_includes('install').

The hooks that should go in the installation file are hook_enable(), hook_disable(), hook_install(), hook_schema(), hook_uninstall(), and hook_requirements(); other hooks should go in the module file.

This is very curious, but I can't find any docs that refer on where to call the hook_schema_alter function, it would make sense to call it in the install file, but is not included in the "usual install file functions set", also would be the only alter function called in this file, bear in mind that alter functions are invoked by drupal_alter function.
It would be great to clarify and document this for the future

Thank you for your contribution! I am going to update your account.

These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.