[D7] Field Prefix

I'm not sure I really understand the purpose of this module that can't be solved by just adding an additional field? Perhaps you can add a more detailed explanation. I downloaded the source to look through it to see if I could figure out the intended purpose but the only thing that was in the git repo was the .info file. There is no .module file. Please commit your code to the repo.

Additionally, you have two very minor errors on the pareview: http://ventral.org/pareview/httpgitdrupalorgsandboxasik1985302git

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.

Manual review:

change git clone to git clone http://git.drupal.org/sandbox/asik/1985302.git field_prefix

also use t() function with strings like

  '#title' => 'Additional description',
  '#description' => 'Additional text over the field',

must be

  '#title' => t('Additional description'),
  '#description' => t('Additional text over the field'),

separate code more often with empty line

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

manual review:

1. field_prefix_form_alter(): use hook_form_FORM_ID_alter() instead.
2. field_prefix_field_attach_form(): this is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as additional description I get a nasty javascript popup when editing content with that field. You need to sanitize user provided text before printing. Please read http://drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
3. field_prefix_field_attach_form(): You are overriding #field_prefix here - what if it has already been set by some other module?
4. 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.

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

manual review:

1. field_prefix_description_wrapper(): the actual markup should be in a theme function so that it can be overridden easily.
2. field_prefix_description_wrapper(): strip_tags() is not safe against XSS! I could not actually exploit this on my modern browsers, but you should use filter_xss() or similar functions anyway. See http://drupalscout.com/knowledge-base/drupal-text-filtering-cheat-sheet-... . You could even make a text format configurable or use filter_xss_admin().
3. field_prefix_include_css(): the optional CSS files do not exist in your CSS directory? Shouldn't you allow themes to provide their CSS overrides in their theme folder? And themes can override the CSS anyway, so what's the point?
4. field_prefix.css: file is never used?

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

append review

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

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

I see no blockers with this, but there are few things you should address, probably prior to promoting your project as one of them is regarding the name.

I have been looking long and hard at whether this is the proper way to handle adding text above the textfield in the form, but that is not the main purpose of this review. The code looks quite good and there effective use of the core Forms/Field API which is the important part; demonstrating good use of Drupal API to make a module. You could have taken a slightly different approach by adding your own custom theme hints to the form element which would allow adding an alternative to theme_form_element() which is where the final HTML for the form field is generated. This is probably a little better than appending things to #title or #field_prefix, but your method is effective in most situations. theme_form_element() or equivalent is at a much lower level than hook_field_attach_form(), and may allow for more generalisation instead of having to code for each field type. I will leave it to you to investigate.

Project page and README

Both of these are exceedingly short making it difficult for a user to assess the module's features and usage. Your application post here gives much more information so perhaps you could include some of that on the project page.

Project name

Fields already have a prefix and suffix which are quite different to what this module provides, even if it is using the field prefix to add the text. This may lead to some ambiguity for users looking for the module, or inferring what it does. Have you considered calling it something more related to its function of adding a description above the field? Field Description Prefix? Extra Field Description? Since the normal description is called "Help Text", Extra Help Text? Split Help Text? In terms of UI, it has far more to do with Help Text than a description or a prefix. In such a case, you may with to integrate much more closely with the Help Text textarea instead of making a new fieldset.

Limitations

You should note on your project page that it does not have locale/language support - You have hard coded in LANGUAGE_NONE when adding the text. Or you could add multilingual support as there should be language parameter passed in one of the objects pased to hook_field_attach_form() but I did not look too deeply. This is a fairly advanced move.

Text processing

The normal Help Text allows for a large number of HTML tags, written into the function _field_filter_xss_allowed_tags(). You have coded it so that only the anchor (a) tag is allowed which departs from what admin may expect of their Help Text. Perhaps you should be basing it on the code used for the Help Text, which calls filter_xss($string, _field_filter_xss_allowed_tags()) to sanitize the text.

Thanks for your contribution, Akopyan!

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 being an excellent project maintainer:

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

(Welcome message shamelessly stolen from klausi!)